Ревью кода: успешный опыт

В Интернете найдется масса информации по ревью кода:

• как ревью чужого кода влияет на культуру компании
• о регламентированных проверках безопасности
• сокращенные руководства
• длинные списки рекомендаций
• ревью кода с межличностной точки зрения
• зачем вообще нужен ревью кода
• проверенные методы
• еще про проверенные методы
• статистика: насколько ревью кода помогает вылавливать баги
• примеры ошибок, допускаемых при ревью кода

Да, конечно же, есть и книги на эту тему. Словом, в этой статье изложено, как ревью кода организован в компании Palantir. В тех организациях, чья культура не приемлет подобной коллегиальной оценки, возможно, полезно будет сначала ознакомиться с блестящим эссе Карла Уиджерса  (Karl E. Wiegers) «Ревью кода с человеческим лицом», а затем попытаться следовать этому руководству.

Этот текст взят из рекомендаций по повышению качества,  составленным на основе работы с Baseline, нашим инструментом для контроля качества кода на Java. В нем рассмотрены следующие темы:

•         Зачем, что и когда мы пытаемся достичь при ревью кода
  
•         Подготовка кода к ревью
  
•         Выполнение ревью кода
  
•         Примеры ревью кода
  

Переведено в Alconost


Комикс XKCD ‘Качество кода’, скопировано по лицензии CC BY-NC 2.5

Мотивация

Мы занимаемся ревью кода (РК) для повышения качества кода и хотим таким образом положительно повлиять на командную и корпоративную культуру.  Например:

• Авторы кода мотивированы, поскольку знают, что их запрос на внесение изменений будет рассматривать коллектив рецензентов: автор старается подчистить шероховатости, следовать плану действий и вообще улучшить весь коммит. Когда коллеги признают твой профессионализм по части написания кода – для многих программистов это повод собой гордиться.
  
• Обмен знаниями помогает команде разработчиков сразу в нескольких отношениях:
  

— При РК явно сообщается, какая часть функционала была добавлена/изменена/удалена, чтобы в дальнейшем члены команды могли опираться на уже выполненную работу.

— Автор кода может использовать прием или алгоритм, на котором рецензенты сами чему-то научатся. В целом ревью кода помогает повысить планку качества в рамках всей организации.

— Рецензенты могут знать о приемах программирования или о самой базе кода нечто такое, что поможет улучшить или консолидировать внесенные изменения; например, кто-нибудь может в то же самое время разрабатывать или дорабатывать точно такую же возможность.  

— Положительный контакт и коммуникация усиливают социальные связи между членами команды.

• Благодаря согласованности в базе кода сам код гораздо проще читать и понимать. Так легче предотвращать баги и способствовать сотрудничеству между оседлыми и кочевыми программистами.
  
• Автору кода сложно судить об удобочитаемости собственного творения, а рецензенту – как раз легко, ведь он не видит, в каком контексте этот код находится. Удобочитаемый код проще многократно использовать, в нем меньше багов, и он долговечнее.
  
• Случайные ошибки (напр., опечатки), а также структурные ошибки (напр., неисполняемый код, ошибки в логике или алгоритмах, проблемы  с производительностью и архитектурой) зачастую гораздо проще выловить придирчивому рецензенту, который смотрит на код со стороны. По данным исследований, даже краткое и неофициальное ревью кода значительно влияет на качество кода и встречаемость багов.
  
• Требования соответствия и нормативные базы зачастую требуют обязательных проверок. Ревью кода – отличный способ избежать распространенных проблем с безопасностью. Если требования к безопасности в данной среде или применительно к данной возможности повышены, то ревью будет рекомендован (или даже затребован) сатрапами из регулирующих органов (хороший пример — руководство OWASP).

На что обращать внимание

Однозначно верного ответа на этот вопрос не существует, и в каждой команде разработчиков требуется согласовать собственный подход. В некоторых коллективах предпочитают проверять любое изменение, вносимое в основную ветку разработки, а в других учитывается «порог тривиальности», по превышении которого проверка является обязательной. В данном случае приходится выдерживать баланс между эффективным использованием времени программиста (как автора, так и рецензента кода) и поддержанием качества кода. В некоторых строгих контекстах порой требуется проверять в коде все, даже самые тривиальные изменения.

Правила ревью кода одинаковы для всех: даже если вы самый старший разработчик в команде, это еще не означает, что ваш код не требует ревью. Даже если (бывает и так) код безупречен, ревью открывает возможности для менторства и сотрудничества, и, как минимум, помогает взглянуть на базу кода с разных точек зрения.

Когда проверять

Ревью кода должно происходить после успешного прохождения автоматических проверок (тесты, оформление, другие этапы непрерывной интеграции), но до слияния нового кода с главной веткой репозитория. Обычно мы не прибегаем к формальной проверке суммарных изменений, внесенных в код с момента последнего релиза.

При сложных изменениях, которые должны добавляться в главную ветку кода единым блоком, но настолько обширных, что их фактически нельзя охватить за одну проверку, попробуйте поэтапное ревью. Создаем первичную ветку feature/big-feature и ряд вторичных (напр., feature/big-feature-api, feature/big-feature-testing, т.д.), в каждой из которых инкапсулировано подмножество общего функционала, и каждая такая ветка сверяется с основной веткой feature/big-feature. После того как все вторичные ветки будут слиты в feature/big-feature, создаем ревью кода для вливания последней в главную ветку.

Подготовка кода к ревью

Автор кода обязан предоставлять код на ревью в удобоваримом виде, чтобы не отбирать у рецензентов лишнего времени и не демотивировать их:

• Область применения и размер. Изменения должны относиться к узкой, четко определенной, самодостаточной области применения, полностью покрываемой при ревью. Например, изменения могут быть связаны с реализацией некоторой возможности или с исправлением ошибки. Краткие изменения предпочтительнее длинных. Если на ревью подается материал, включающий изменения в более чем ~5 файлах, либо требует более 1–2 дней на запись, либо на само ревью может уйти более 20 минут, попробуйте разбить материал на несколько самодостаточных фрагментов и проверить каждый отдельно. Например, разработчик может отправить изменение, определяющее API для новой возможности в терминах интерфейсов и документации, а затем добавить и второе изменение, в котором описаны реализации для этих интерфейсов.
  
• Присылать нужно только полные, самостоятельно проверенные (воспользуйтесь diff) и самостоятельно протестированные материалы. Чтобы сэкономить время рецензента, тестируйте отправляемые изменения (т.е. запускайте тестовый набор) и убедитесь, что код проходит все сборки, а также все тесты и все этапы проверки качества, как локально, так и на серверах непрерывной интеграции, и только потом подбирайте рецензентов.
  
• Рефакторинг изменений не должен сказываться на поведении и наоборот; изменения, влияющие на поведение, не должны быть сопряжены с рефакторингом или переформатированием кода. На то есть ряд веских причин:  
  

— Изменения, связанные с рефакторингом, обычно затрагивают множество строк кода во многих файлах – следовательно, этот код будет проверяться не так внимательно. Незапланированные изменения в поведении могут просочиться в базу кода, и никто этого даже не заметит.

— Крупные изменения, связанные с рефакторингом, нарушают механизмы перемещения, отбора и другую «магию», связанную с контролем исходников. Очень сложно отменить такое изменение поведения, которое было внесено после завершения рефакторинга, охватившего весь репозиторий.

— Дорогое рабочее время, которое человек тратит на рецензирование кода, должно уходить на проверку программной логики, а не на споры о стиле, синтаксисе или форматировании кода. Такие вопросы мы предпочитаем решать при помощи автоматических инструментов, в частности, Checkstyle, TSLint, Baseline, Prettier и т.д.

Коммит-сообщения

Далее приведен пример грамотного коммит-сообщения, которое выдержано в соответствии с широко применяемым стандартом:

Краткое (до 80 символов) описание изменений
Более детальный, поясняющий текст, если он требуется. Ограничьтесь
примерно 72 символами. В некоторых контекстах первая строка
рассматривается как тема электронного письма, а остальной
текст — как тело письма. Важна пустая строка, отделяющая краткое
описание от тела письма (если тело письма присутствует);
в противном случае такие инструменты, как rebase, могут воспринять
написанное некорректно.
Пишите коммит-сообщение в повелительном наклонении: «Исправить ошибку», а не «Исправление ошибки» и не «исправляет ошибку». Такое соглашение соблюдается и в коммит-сообщениях, генерируемых автоматически, например, командами git merge и git revert.	
Дальнейшие абзацы идут после пустых строк.
Также допустимы маркированные списки.

Старайтесь описать и изменения, вносимые при коммите, и как именно эти изменения делаются:

> ПЛОХО. Не делать так.
Еще раз запустить компиляцию.
> Хорошо.
Добавить jcsv-зависимость для исправления компиляции в IntelliJ

Поиск рецензентов

Обычно автор коммита подыскивает одного-двух рецензентов, знакомых с базой кода. Зачастую в таком качестве выступают руководитель проекта или старший инженер. Владельцу проекта целесообразно подписываться на собственный проект, чтобы получать уведомления о новых ревью кода. При участии троих и более рецензентов ревью кода зачастую получается непродуктивным или контрпродуктивным, поскольку разные рецензенты могут предлагать взаимно противоречивые изменения. Это может свидетельствовать о фундаментальных разногласиях по поводу верной реализации, и такие проблемы должны решаться не во время ревью кода, а на расширенном собрании, в котором лично или в режиме видеоконференции участвуют все заинтересованные стороны.

Выполнение ревью кода

Ревью кода – это точка синхронизации между разными членами команды, поэтому потенциально она может стопорить работу. Поэтому ревью кода должно быть оперативным (занимать порядка нескольких часов, а не дней), а члены и руководители команды должны осознавать, сколько времени потребуется на проверку, и соответствующим образом приоритезировать выделяемое на нее время. Если вам кажется, что вы не успеете закончить ревью вовремя, сразу скажите об этом автору коммита, чтобы он успел найти вам замену.

Ревью должно быть довольно основательным, чтобы рецензент мог объяснить изменение другому разработчику с достаточной детализацией. Так мы добьемся того, чтобы подробности о базе кода были известны хотя бы двоим, а не одному.

Вы как рецензент должны настойчиво придерживаться стандартов качества кода и держать его на высоком уровне. Ревью кода – скорее искусство, чем наука. Научиться этому можно только на практике. Опытный рецензент должен попробовать сначала подпустить к изменениям менее опытных коллег и позволить им сделать ревью первыми. Если автор следовал вышеизложенным правилам (особенно тем, что касаются самопроверки и предварительного запуска кода), то вот на что должен обратить внимание рецензент при ревью:

Цель

• Позволяет ли код достичь цели, поставленной автором? У любого изменения должна быть конкретная причина (новая возможность, рефакторинг, исправление ошибки, т.д). В самом ли деле предложенный код приводит нас к этой цели?
  
• Задавать вопросы. Функции и классы должны быть обоснованы. Если их назначение рецензенту непонятно, возможно, это означает, что код следует переписать либо подкрепить комментариями или тестами.
  

Реализация

• Подумайте, как бы решили эту задачу вы. Если иначе – то почему? Обрабатывает ли ваш код больше (пограничных) случаев? Может быть, он короче/проще/чище/быстрее/безопаснее, а функционально не хуже? Может быть, вы уловили какую-то глубинную закономерность, не охватываемую имеющимся кодом?
  
• Видите ли вы потенциал для создания полезных абстракций? Если код частично дублируется, это зачастую означает, что из него можно извлечь более абстрактный или общий элемент функционала, который затем получится переиспользовать в иных контекстах.
  
• Мыслите как оппонент, однако, оставайтесь любезны. Старайтесь «подловить» авторов, срезающих углы или упускающих конкретные случаи, подбрасывая им проблемные конфигурации/вводимые данные, которые ломают их код.
  
• Подумайте о библиотеках или о готовом рабочем коде. Если кто-то заново реализует уже имеющуюся функциональность – это бывает не только потому, что он не знает о существовании уже готового решения. Иногда код или функциональность переизобретается намеренно – напр., чтобы избежать зависимостей. В таком случае это нужно четко прокомментировать в коде. Предоставляется ли уже данная функциональность в какой-нибудь существующей библиотеке?
  
• Соответствует ли вносимое изменение стандартным паттернам? В сложившихся базах кода часто прослеживаются закономерности, связанные с соглашениями об именовании, декомпозиции программной логики, определениями типов данных и т.д. Обычно желательно, чтобы все изменения реализовывались в соответствии с существующими закономерностями.
  
• Добавляются ли при изменении зависимости, возникающие во время компиляции или во время исполнения (особенно между подпроектами)? Мы стремимся к слабой связанности кода наших продуктов, то есть, стараемся допускать минимум зависимостей. Изменения, связанные с зависимостями и системой сборки, должны проверяться особенно тщательно.
  

Удобочитаемость и стиль

• Подумайте, каково вам читать этот код. Удается ли достаточно быстро схватывать его концепции? Разумно ли он уложен, легко ли следить за названиями переменных и методов? Удается ли прослеживать код на протяжении нескольких файлов или функций? Покоробило ли вас где-нибудь непоследовательное именование?
  
• Соответствует ли код известным рекомендациям по программированию и стилю? Не выбивается ли код из всего проекта по стилю, соглашениям об именованиях API, т.д.? Как было указано выше, мы стараемся улаживать стилистические споры при помощи автоматических инструментов.
  
• Есть ли в коде списки задач (TODO)? Списки задач просто накапливаются в коде и со временем превращаются в балласт. Обяжите автора отправлять тикет на GitHub Issues или JIRA и прикреплять к TODO номер задачи. В предлагаемом изменении не должно быть закомментированного кода.
  

Удобство поддержки

• Читайте тесты. Если тестов в коде нет, а они там должны быть – попросите автора написать несколько. По-настоящему не тестируемые возможности встречаются редко, а не протестированные реализации возможностей – к сожалению, часто. Сами проверяйте тесты: покрывают ли они интересные случаи? Удобно ли их читать? Снижается ли после этой проверки общее покрытие тестами? Подумайте, каким образом может сбоить этот код. Стандарты оформления тестов и основного кода зачастую отличаются, но и стандарты тестов тоже важны.
  
• Возникает ли при ревью этого фрагмента риск нарушить тестовый код, обкаточный код или интеграционные тесты? Зачастую такое ревью не делается перед коммитом/слиянием, но, если такие случаи окажутся запущенными, то пострадают все. Обращайте внимание на следующие вещи: удаление тестовых утилит или режимов, изменение конфигурации, изменения в компоновке/структуре артефакта.
  
• Нарушает ли это изменение обратную совместимость? Если так, то можно ли вносить это изменение в релиз на данном этапе, либо следует отложить его на более поздний релиз? К нарушениям могут относиться изменения базы данных или ее схемы, изменения публичных API, изменения потока задач на уровне пользователя и т.д.
  
• Требуются ли в этом коде интеграционные тесты? Иногда код не удается как следует проверить при помощи одних лишь модульных тестов, особенно если он взаимодействует со внешними системами или конфигурацией.
  
• Оставляйте отзывы к документации по этому коду, к комментариям и коммит-сообщениям. Пространные комментарии засоряют код, а скупые коммит-сообщения запутывают тех, кому впоследствии придется работать с кодом. Так бывает не всегда, но качественные комментарии и коммит-сообщения со временем обязательно сослужат вам хорошую службу. (Вспомните, когда вам доводилось видеть великолепный или просто ужасный комментарий или коммит-сообщение.)
  
• Обновлялась ли внешняя документация? Если по вашему проекту ведутся файлы README, CHANGELOG или другая документация – то обновлена ли она с учетом внесенных изменений? Устаревшая документация может быть пагубнее, чем вообще никакой, и дороже выйдет исправить ошибки в будущем, чем внести изменения сейчас.
  

Не забывайте похвалить лаконичный/удобочитаемый/эффективный/красивый код. Напротив, забраковать или отклонить код, предложенный на ревью — не грубость. Если изменение избыточно или несущественно – отклоните его, пояснив причину. Если изменение кажется вам неприемлемым из-за одной или нескольких фатальных ошибок – забракуйте его, опять же, аргументированно. Иногда наилучший исход ревью кода формулируется «давайте сделаем это совершенно иначе» или «давайте вообще не будем этого делать».

Уважайте рецензируемого. Хотя, позиция оппонента здравая, не вы реализовали эту возможность, и не можете все за него решать. Если не удается прийти с рецензируемым к общему мнению по поводу кода, устройте консультацию в реальном времени и выслушайте мнение третьего человека.

Безопасность

Убедитесь, что на всех конечных точках API выполняется адекватная авторизация и аутентификация, соответствующая правилам, принятым в остальной базе кода. Ищите другие распространенные изъяны, например: слабую конфигурацию, вредоносный пользовательский ввод, недостающие записи в логах, т.д. Если сомневаетесь – покажите рецензируемый код специалисту по безопасности.

Комментарии: краткие, вежливые, побудительные

Рецензия должна быть краткой, выдержанной в нейтральном стиле. Критикуйте код, а не автора. Если что-то непонятно – попросите пояснить, а не считайте, что все дело в невежестве автора. Избегайте притяжательных местоимений, особенно в оценочных суждениях: «мой код работал до ваших изменений», «в вашем методе есть баг» и т.д. Не рубите сплеча: «это вообще не будет работать», «результат всегда неверен».

Старайтесь разграничивать рекомендации (напр., «Рекомендация: извлеките метод, так код станет понятнее»), необходимые изменения (напр., «Добавить Override») и мнения, требующие разъяснения или обсуждения (напр., «В самом ли деле это поведение верное? Если так — пожалуйста, добавьте комментарий, поясняющий логику»). Попробуйте поставить ссылки или указатели на подробное описание проблемы.

Закончив ревью кода, укажите, насколько подробной реакции на ваши комментарии вы ожидаете от автора, хотели бы вы повторить ревью после того, как изменения будут внесены, например: «Можете смело заливать код в главную ветку после этих небольших исправлений» или «Пожалуйста, учтите мои замечания и дайте мне знать, когда я смогу снова посмотреть код».

Реакция на рецензию

Ревью кода, в частности, призвано улучшить предложенный автором запрос на изменения; поэтому не принимайте в штыки рекомендации рецензента, отнеситесь к ним серьезно, даже если не согласны с ними. Отвечайте на любой комментарий, даже на обычные «ACK» (одобрено) или «done» (готово). Объясните, почему приняли то или иное решение, зачем у вас в коде те или иные функции, т.д. Если не можете прийти к общему мнению с рецензентом, устройте консультацию в реальном времени и выслушайте мнение стороннего специалиста.

Исправления должны фиксироваться в той же ветке, но в отдельном коммите. Если склеивать коммиты на этапе рецензирования, то рецензенту будет сложнее отслеживать изменения.

Политика слияния кода в разных командах отличается. В некоторых компаниях слияние кода доверяют только владельцу проекта, в других это может делать и участник после того, как его код будет просмотрен и одобрен.

Ревью кода тет-а-тет

Как правило, для ревью кода отлично подходят асинхронные diff-подобные инструменты, например, Reviewable, Gerrit или GitHub. Если речь идет о сложном ревью или о ревью, участники которого сильно отличаются по уровню опыта или профессионализма, ревью бывает эффективнее проводить лично — либо сидя вдвоем перед одним монитором или проектором, либо удаленно, через видеоконференцию или инструменты для совместного просмотра экрана.

Примеры

В следующих примерах комментарии рецензента в листингах вводятся при помощи

//R: ...

Противоречивое именование

class MyClass {
  private int countTotalPageVisits; //R: называйте переменные единообразно
  private int uniqueUsersCount;
}

Несогласованные сигнатуры методов

interface MyInterface {
  /** Возвращает {@link Optional#empty}, если s не удается извлечь. */
  public Optional<String> extractString(String s);
  /** Возвращает null, если {@code s} не удается переписать. */
  //R: следует гармонизировать возвращаемые значения: также применяйте здесь // Optional<>
  public String rewriteString(String s);
}

Использование библиотеки

//R: удалите и замените на MapJoiner из библиотеки Guava
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);

Личные предпочтения

int dayCount; //R: нет: я предпочитаю numFoo а не fooCount; на ваш вкус, но это название должно быть единообразным во всем проекте

Баги

//R: Количество итераций здесь numIterations+1 – так и должно быть?
//  Если нет – давайте попробуем изменить семантику numIterations?
for (int i = 0; i <= numIterations; ++i) {
  ...
}

Архитектурные соображения

otherService.call(); //R: Думаю, нужно обойтись без зависимости от OtherService. Можем обсудить это лично?

О переводчике

Перевод статьи выполнен в Alconost.

Alconost занимается локализацией игр, приложений и сайтов на 70 языков. Переводчики-носители языка, лингвистическое тестирование, облачная платформа с API, непрерывная локализация, менеджеры проектов 24/7, любые форматы строковых ресурсов.

Мы также делаем рекламные и обучающие видеоролики — для сайтов, продающие, имиджевые, рекламные, обучающие, тизеры, эксплейнеры, трейлеры для Google Play и App Store.

Подробнее