Как проводить code review: порядок проверки и чек-лист

Хорошее code review начинается не с первой изменённой строки. Сначала ревьюер выясняет, какую задачу решает pull request, затем проверяет самые дорогие риски и только после этого переходит к поддерживаемости и стилю. Такой порядок помогает не потратить всё время на нейминг, пропустив ошибку в авторизации или потерю данных.

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

Сначала поймите задачу, а не код

Перед чтением diff ответьте на четыре вопроса:

1. Какое поведение должно измениться?
2. Какие сценарии обязаны остаться прежними?
3. Где проходит граница изменения: API, база данных, интерфейс, фоновая задача?
4. Как автор проверил результат?

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

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

Оцените размер и форму pull request

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

Полезно мысленно разделить diff на части:

• публичные контракты и точки входа;
• основная бизнес-логика;
• работа с данными и внешними системами;
• обработка ошибок;
• тесты и документация.

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

Проверяйте риски в правильном порядке

Универсального чек-листа для всех технологий нет, но порядок приоритетов довольно стабилен.

1. Корректность

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

Задавайте к каждой ветке простой вопрос: «Что должно быть истинно, чтобы мы сюда попали, и что получит пользователь на выходе?» Это часто обнаруживает проблему быстрее, чем чтение каждой строки по отдельности.

2. Данные и конкурентность

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

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

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

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

Дальше смотрите на инъекции, раскрытие чувствительных данных, небезопасные редиректы, хранение секретов и сообщения об ошибках. Если проблема позволяет получить чужие данные или выполнить чужое действие, это blocker, а не «желательное улучшение».

4. Надёжность и обработка ошибок

Что произойдёт, если база временно недоступна, внешний API вернёт неожиданный ответ, а фоновая задача запустится повторно? Хороший код не обязан скрывать любой сбой, но должен переводить его в предусмотренное состояние и оставлять достаточно данных для диагностики.

Проверьте таймауты, повторные попытки, освобождение ресурсов и отсутствие слишком широких except/catch, которые превращают реальную ошибку в тихую потерю результата.

5. Производительность

Оптимизировать всё заранее не нужно. Ищите изменения сложности и операции, стоимость которых растёт вместе с объёмом данных: N+1 запросы, загрузку всей таблицы в память, повторные вычисления, сетевые вызовы внутри цикла.

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

6. Тесты

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

Проверьте:

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

7. Поддерживаемость

Только после рисков переходите к структуре. Смотрите на слишком длинные функции, неявные зависимости, дублирование бизнес-правил и названия, которые скрывают смысл.

Не каждое несовпадение с вашим привычным стилем требует комментария. Если вариант автора читаем, соответствует правилам проекта и не создаёт риск, его можно оставить.

Как отличить баг от субъективного замечания

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

Субъективное замечание звучит иначе: «я бы написал это через другой паттерн» или «мне больше нравится такое имя». Прежде чем отправить комментарий, попробуйте закончить фразу: «Если оставить код так, то…». Если конкретного продолжения нет, вероятно, это nit или личное предпочтение.

На живой тренировке code review этот навык проверяется отдельно: в PR есть не только реальные дефекты, но и подозрительные корректные места. Лишние замечания снижают точность ревью так же, как пропущенные проблемы — полноту.

Как не пропустить проблему в большом PR

Используйте несколько коротких проходов вместо одного длинного:

1. Контракт и архитектурная форма изменения.
2. Корректность, данные и безопасность.
3. Ошибки, производительность и тесты.
4. Поддерживаемость и итоговое решение.

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

Если PR слишком велик для надёжной проверки, нормальное замечание — предложить разделение. Это не отказ ревьюить, а управление риском: небольшой независимый diff проще понять, проверить и откатить.

Как завершить ревью

Перед Request changes или Approve перечитайте свои комментарии и расставьте критичность:

Не блокируйте PR из-за nit-комментариев. Если обязательные проблемы исправлены, а оставшиеся замечания не меняют безопасность и контракт, ревью можно одобрить с необязательными предложениями.

Чек-лист перед отправкой ревью

• Я понимаю задачу и ожидаемое поведение.
• Я проверил корректность и граничные случаи.
• Я посмотрел на права доступа, данные и конкурентность.
• Я оценил обработку сбоев и стоимость операций.
• Тесты подтверждают контракт и падают без изменения.
• Каждый мой комментарий объясняет последствие.
• Критичность соответствует реальному риску.
• Итоговое решение не блокирует PR из-за личного вкуса.

Чек-лист помогает не забывать направления, но навык появляется только в практике. Для быстрого разогрева подойдут задачи на поиск ошибок в коде, а для полного цикла — тренажёр code review для собеседования, где нужно написать замечания своими словами, ответить автору и принять решение по pull request.