На что обращать внимание при код-ревью

Часть публичной документации Google по инженерным практикам, написанная для ревьюеров. В ней перечислены аспекты, которые ревьюер должен проверять, в примерном порядке их важности. Документ рассчитан на чтение вместе с их страницей Standard of Code Review. Около десятка заголовков несут в своей последовательности чёткий посыл: архитектура и дизайн идут первыми, а стиль оформления - ближе к концу.

Архитектура, функциональность, сложность

Архитектура (design) названа самым важным пунктом ревью. Разумно ли взаимодействуют части изменения, место ли этому коду в вашей кодовой базе или в отдельной библиотеке, как он стыкуется с тем, что уже написано, и подходящий ли сейчас момент, чтобы вообще это добавлять. Ничего из этого не видно при построчном чтении diff'а.

Функциональность делится на два вопроса: делает ли изменение то, что планировал автор, и несёт ли задуманное пользу тем, кто будет этим кодом пользоваться. Под пользователями понимаются как конечные пользователи продукта, так и разработчики, которые будут вызывать этот код в дальнейшем. От ревьюера не требуют перепроверять всё руками - предполагается, что это сделал автор, - но требуют думать о краевых случаях и искать потенциальные баги при чтении. Реальный запуск кода оправдан в двух ситуациях. Изменения, видимые пользователю (особенно в UI), тяжело оценить по diff'у, поэтому, если накатить патч неудобно, стоит попросить автора показать демо. Вторая ситуация - параллельное программирование: deadlock'и и race condition'ы обычно не выявляются простым запуском, тут требуется вдумчивый логический анализ. В документе есть полезное замечание: это само по себе повод избегать моделей конкурентности, допускающих гонки и взаимные блокировки, поскольку они сильно удорожают ревью и понимание кода.

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

Тесты, именование, комментарии

Тесты должны идти в том же изменении, что и рабочий код, за исключением аварийных ситуаций. Задача ревьюера - проверить, что тесты корректны, осмысленны и полезны, поскольку сами себя тесты не проверяют, а тесты на тесты никто не пишет. Конкретные вопросы: упадут ли они при поломке кода, не начнут ли выдавать ложные срабатывания при изменении лежащего под ними кода, делает ли каждый тест простые и полезные проверки, аккуратно ли распределены сценарии по отдельным тестовым методам. Тестовый код тоже нужно поддерживать, поэтому сложность в нём не становится бесплатной оттого, что он не попадает в итоговый бинарник.

Хорошее имя достаточно длинное, чтобы объяснить суть или действие сущности, не становясь при этом громоздким. Комментарии должны в основном объяснять, зачем этот код существует, а не что он делает: если требуется объяснять, что он делает, код нужно упростить; признанные исключения - регулярные выражения и сложные алгоритмы. Полезная привычка - перечитывать комментарии, которые уже были в кодовой базе: среди них может быть TODO, ставший ненужным благодаря этому изменению, или комментарий, предостерегающий именно от того, что делает автор. Документация к классу, модулю или функции рассматривается отдельно от комментариев: она фиксирует назначение, предполагаемое использование и поведение. Если изменение затрагивает сборку, тестирование, использование или релиз кода, вместе с ним обновляются README и справочная документация, а удаление кода требует удаления соответствующей документации.

Стиль, единообразие и две привычки напоследок

Если гайдлайн по стилю требует определённого подхода, требование соблюдается безоговорочно. Если он лишь рекомендует, решение принимается по ситуации: сопоставляются рекомендация и единообразие с окружающим кодом, с приоритетом у гайдлайна, если только локальное расхождение не будет сбивать с толку. Когда применимого правила нет вовсе, ориентируйтесь на существующий код, а для наведения порядка заводите отдельный баг с TODO, вместо того чтобы переписывать всё по ходу дела. Личные предпочтения, отсутствующие в гайде, помечаются префиксом "Nit:" и никогда не блокируют отправку кода. Масштабное переформатирование выносится в отдельное изменение, изолированно от функциональных правок, чтобы diff'ы, слияния и откаты оставались читаемыми.

Завершают документ два тезиса. Просматривайте каждую назначенную вам строку: беглый просмотр допустим для файлов с данными, сгенерированного кода и больших структур данных, но не для написанной человеком функции в надежде, что "там всё нормально". Если код слишком трудно читать и это тормозит ревью, это не ваша личная проблема, а замечание к коду: скажите об этом прямо и подождите разъяснений от автора, ведь другие разработчики споткнутся ровно о то же самое. Привлекайте профильных ревьюеров по приватности, безопасности, конкурентности, доступности или интернационализации, если сами в этом не специализируетесь. Когда изменение делят несколько ревьюеров, укажите в комментарии, какую именно часть вы проверили.

И смотрите на изменение в контексте. Инструмент ревью показывает лишь несколько строк вокруг diff'а, но четыре добавленные строки могут оказаться внутри метода на пятьдесят строк, который давно пора разбить, чего сам по себе diff не покажет. На уровне всей системы этот же принцип превращается в единственное категоричное правило документа: не принимайте изменения, ухудшающие состояние (code health) системы, ведь системы становятся сложными из-за множества мелких правок, каждая из которых по отдельности казалась приемлемой. Последний раздел призывает ревьюеров отмечать удачные решения: похвала за то, что сделано хорошо, с точки зрения менторства часто даёт больше, чем очередное исправление.

Стоящие за этим предпосылки

Чек-лист рассчитан на блокирующее ревью перед слиянием, выполняемое человеком, у которого есть на это время. Большинство требований - соответствие архитектуре, отсутствие избыточного усложнения, реальная работоспособность тестов - нацелены вовсе не на поиск багов. Это совпадает с выводами из code-review-knowledge-transfer: менее 15% комментариев на ревью касаются ошибок, а главный результат процесса - обмен знаниями. Требование "просматривать каждую строку" сталкивается с ограничениями пропускной способности из code-review-throughput-limits, как только объём кода растёт. В этом заключается вся сложность из reviewing-ai-code, и именно поэтому в code-review-principal-agent утверждается, что код, сгенерированный агентами, ломает сам процесс ревью, а не просто раздувает его. Подходы вроде ship-show-ask и stop-using-pull-requests выступают ответом, снижающим количество работы, которую вообще приходится прогонять через этот чек-лист.