Чек-лист код-ревью: что проверять, кроме того, работает ли код

Ревью, которое сводится к «выглядит норм, мержим», не ловит ничего, кроме совсем очевидных опечаток. Рабочий чек-лист для PR держит внимание на пяти зонах: корректность, читаемость, тесты, безопасность и производительность, и у каждой есть конкретные вопросы, а не общее пожелание посмотреть внимательнее.

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

Зачем чек-лист, если можно просто «посмотреть код»

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

• Чек-лист снижает зависимость качества ревью от настроения и усталости ревьюера
• Чек-лист одинаков для всей команды, значит автор PR знает, чего ждать
• Чек-лист можно пройти за фиксированное время, а не бесконечно вчитываться

Корректность: логика и граничные случаи

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

• Что происходит на пустом входе: пустой массив, null, строка нулевой длины
• Что происходит на границе: первый и последний элемент, ноль, отрицательное число
• Может ли этот код выполниться параллельно для одних и тех же данных

Полезная техника: прочитать описание PR и мысленно составить три-четыре входных примера, которые автор не упомянул явно. Если код справляется с ними без правок, корректность подтверждена. Если нет, лучше указать на это сразу, а не оставить на откуп автоматическим тестам, которых для этого случая может и не быть.

Читаемость и структура

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

Хороший сигнал при чтении: если для объяснения, что делает функция, нужно больше одного предложения с союзом «и», скорее всего, она делает слишком много и стоит её разделить ещё до мержа, а не откладывать рефакторинг на потом, когда функция обрастёт новыми вызовами.

Тесты: что должно быть покрыто

Наличие тестов само по себе ничего не гарантирует, если они проверяют только счастливый путь. Хороший вопрос на ревью не «есть ли тесты», а «что из описанных в PR граничных случаев покрыто тестом, а что осталось на словах».

• Граничные случаи из описания PR отражены в тестах, а не только в комментариях
• Тест падает, если убрать проверку из кода (быстрая ручная проверка)
• Тест не завязан на детали реализации, которые могут поменяться без изменения поведения

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

Безопасность и производительность

Эти два пункта чаще всего пропускают, потому что они не видны на тестовых данных и небольшом объёме трафика. Безопасность, это в первую очередь вопрос, откуда пришли данные и что с ними делают: прямая вставка в SQL-запрос или вывод без экранирования, классические источники уязвимостей.

// Плохо: пользовательский ввод прямо в запросе
$result = $db->query("SELECT * FROM users WHERE email = '{$email}'");

// Хорошо: параметризованный запрос
$stmt = $db->prepare('SELECT * FROM users WHERE email = ?');
$stmt->execute([$email]);

Та же осторожность нужна с выводом данных пользователя на страницу: значение, вставленное в HTML без экранирования, открывает путь для межсайтового скриптинга точно так же, как неэкранированная строка открывает путь для SQL-инъекции. Оба случая объединяет одна проверка: данные извне никогда не считаются безопасными по умолчанию, пока явно не прошли через подготовленный запрос или функцию экранирования.

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

Как писать комментарии к ревью, чтобы их не игнорировали

Комментарий «так не делай» без объяснения причины воспринимается как личная претензия и провоцирует защитную реакцию. Комментарий с конкретным вопросом или альтернативой воспринимается как часть работы над кодом, а не оценка автора.

1. Отделяйте блокирующие замечания от необязательных пожеланий, явно помечая их
2. Объясняйте почему, а не только что не так
3. Предлагайте конкретную альтернативу, если она есть, а не только критику
4. Хвалите удачные решения в том же PR, не только указывайте на проблемы

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

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

Со временем команда, которая пользуется одним и тем же чек-листом, вырабатывает общий язык для обсуждения кода: фраза «это выглядит как N+1» или «а что на пустом входе» понимается одинаково всеми, без долгих объяснений с нуля в каждом PR. Это сокращает время обсуждения заметнее, чем любая попытка ускорить ревью за счёт менее внимательного чтения.