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

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

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

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

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

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


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

Мотивация

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

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

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

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

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

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

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

Подробнее