Google: What to look for in a code review

Google, основываясь на многолетнем опыте, представил свои стандарты того, как лучше всего проводить code review. Все вместе они представляют собой один законченный документ, разбитый на множество отдельных разделов. Это перевод стандартов по проведению Code Review от Google. Вот ссылка на оригинал!

Терминология:

CL — расшифровывается как «список изменений» (“ChangeList”), что означает одно автономное изменение, которое было отправлено в систему управления версиями или подвергается проверке кода. Другие организации часто называют это «изменением» или «патчем».

На что нужно обращать внимание в Code Review

Дизайн

Самая важная вещь в code review — это общий дизайн CL. Для его оценки стоит ответить на ряд вопросов.

• Имеет ли смысл взаимодействие различных частей кода в CL?
• Это изменение относится к вашей кодовой базе или к библиотеке?
• Хорошо ли он интегрируется с остальной частью вашей системы?
• Подходящее ли сейчас время, чтобы добавить эту функциональность?

Функциональность

Этот CL выполняет то, что задумал разработчик? Хороши ли изменения CL для пользователей? «Пользователи» — это и конечные пользователи, на которых влияют изменения, и разработчики, которым придется «использовать» этот код в будущем.

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

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

Еще один момент, когда необходимо подумать о функциональности во время code review — параллельное программирование, которое теоретически может привести к взаимоблокировкам. Подобные проблемы трудно обнаружить, просто запустив код. Обычно требуется, чтобы кто-то (разработчик / ревьюер) тщательно продумал такие моменты.

Cложность

CL сложнее, чем он был запланирован? Функции слишком сложны?

«Слишком сложный» в данном случае означает «не может быть быстро понят другими разработчиками, которые читают этот код». Это также может означать «разработчики могут внести ошибки, когда пытаются вызвать или изменить этот код».

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

Тесты

Запросите интеграционные или сквозные тесты в зависимости от изменений в коде. В общем случае тесты должны быть добавлены в тот же CL, что и рабочий код, если только CL не обрабатывает аварийную ситуацию.

Убедитесь, что тесты в CL правильные, разумные и полезные. Тесты сами не тестируются, и мы редко пишем тесты для самих тестов — ревьюер должен убедиться, что тесты верны.

Будут ли тесты проваливаться при ошибках в коде? Если код был изменен под тестами, не начнут ли тесты давать ложные срабатывания? Выполняет ли каждый тест простые и полезные действия? Правильно ли разделены тесты между различными методами испытаний?

Помните, что тесты также являются кодом, который необходимо поддерживать.

Наименования

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

Комментарии

Написал ли разработчик четкие комментарии на понятном языке? Все ли комментарии действительно необходимы?

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

Также может быть полезно взглянуть на комментарии, которые были до этого CL. Может быть, есть TODO, который можно удалить, или предупреждающий комментарий и т.д.

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

Стиль

Убедитесь, что CL придерживается соответствующих руководств (принятых в вашем проекте или компании) по стилю.

Если вы хотите улучшить какую-то деталь, которой нет в руководстве по стилю, добавьте к комментарию префикс «Nit:», чтобы разработчик знал, что по вашему мнению, можно улучшить в коде. Не блокируйте отправку CL, основываясь только на личных предпочтениях стиля.

Документация

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

Каждая строчка кода

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

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

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

Контекст

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

Также полезно подумать о CL в контексте системы в целом. Улучшает ли этот CL исправность кода системы или делает всю систему более сложной? Не принимайте CL, который ухудшает работоспособность кода системы. Большинство систем становятся сложными из-за множества небольших изменений, поэтому важно предотвратить даже небольшие сложности в новых CL.

Этикет

Если вы видите что-то классное в CL, сообщите об этом разработчику. Обзоры кода чаще фокусируются на ошибках, но они также должны и поощрять передовую практику. Иногда с точки зрения наставничества даже более ценно сказать разработчику, что он сделал правильно, чем сказать ему, что он сделал неверно.

Выводы

В процессе code review вы должны убедиться, что:

• Код хорошо продуман.
• Функциональность хороша для пользователей кода.
• Любые изменения в интерфейсе разумны и выглядят уместно.
• Любое параллельное программирование выполняется безопасно.
• Код не сложнее, чем должен быть.
• Разработчик не должен реализовывать вещи, которые возможно могут понадобиться в будущем, но он не знает, нужны ли они сейчас.
• Код имеет соответствующие модульные тесты.
• Тесты хорошо продуманы.
• Разработчик использовал четкие имена для всего.
• Комментарии понятны и полезны, и отвечают на вопрос “почему”, а не “что”.
• Код надлежащим образом документирован.
• Код соответствует принятым на вашем проекте руководствам по стилю.