Введение: принципы code review

Вы получите рабочую процедуру ревью: в каком порядке читать дифф, какие вопросы задавать на каждом проходе, как помечать замечания по важности и как решить, ставить ли approve. К концу урока вы разберёте настоящий PR и увидите, какие четыре комментария оставил бы опытный ревьюер и в какой формулировке.

Code review — проверка изменений другим разработчиком до попадания в основную ветку. Ценность у него двойная: находит проблемы и распространяет знание о кодовой базе. Второе часто важнее первого — через полгода в команде не остаётся кода, который видел только один человек.

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

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

Проверьте себя. Возьмите последний инцидент на проде и честно ответьте: поймало бы его ревью или нет.

Частая ошибка. Считать ревью заменой тестам и требовать от него гарантий, которых оно не даёт.

Главная ошибка новичка — читать дифф сверху вниз в том порядке, в котором его показал GitHub, то есть по алфавиту имён файлов. Так вы разбираете детали реализации раньше, чем поняли замысел, и тратите внимание на код, который, возможно, вообще не должен существовать.

Читайте в четыре прохода, от замысла к деталям.

Проход 1: контекст, без кода. Прочитайте описание PR и связанный тикет. Ответьте себе на вопрос: какую задачу автор решает и совпадает ли она с той, что в тикете. Посмотрите список изменённых файлов — уже он говорит многое. Если в PR «исправить опечатку в письме» затронуты миграции, что-то не так.

Проход 2: замысел. Найдите точку входа изменения — новый эндпоинт, обработчик, публичный метод — и проследите основной путь. Вопрос этого прохода: правильное ли решение выбрано в принципе. Если ответ «нет», остальные проходы не нужны — обсуждать отступы в коде, который надо выкинуть, бессмысленно.

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

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

Стилевые придирки в этот список не входят вообще: их работа — линтера, а не человека. Если у вас нет линтера и вы обсуждаете кавычки руками, это проблема инфраструктуры, а не автора PR.

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

Частая ошибка. Начинать с первого файла в алфавитном порядке и оставить первые пять комментариев про именование переменных.

Автор PR не умеет читать мысли. Комментарий «здесь лучше использовать словарь» может означать «я не пущу это в мердж» и «просто мысль вслух» — и автор потратит полдня, угадывая, какое из двух.

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

Выглядит это так:

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

Проверьте себя. Перечитайте свои последние десять комментариев в PR и проставьте им уровни задним числом. Сколько из них автор мог понять неправильно?

Частая ошибка. Оставить двадцать замечаний без приоритетов и удивиться, что автор исправил самое неважное, а blocker пропустил.

Финальное действие — тоже сообщение, и оно должно быть честным.

Approve — если blocker'ов нет. Наличие nit-ов не повод держать PR: поставьте approve и напишите «мелочи на твоё усмотрение». Иначе автор ждёт вас лишние сутки ради переименования переменной.

Request changes — если есть blocker. Обязательно с указанием, что именно нужно изменить, чтобы получить approve. «Мне не нравится» — не запрос изменений.

Комментарий без вердикта — когда вы не компетентны в этой части системы или не успели посмотреть всё. Так и напишите: «посмотрел только слой API, миграции пусть глянет кто-то из платформы». Это честнее, чем approve всего PR по одной трети.

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

Частая ошибка. LGTM через две минуты после открытия PR на 900 строк. Это не одобрение, это отказ от ревью — но выглядит как гарантия качества, поэтому вреднее молчания.

Автор — разработчик второго года, задача — добавить выгрузку отчёта. Дифф на 34 строки, CI зелёный.

Пройдём по нашим четырём проходам.

Проход 1 (контекст). Эндпоинт принимает user_id параметром запроса. Кто угодно может подставить чужой — а в описании PR про это ни слова. Первый вопрос: как проверяется, что запрашивающий имеет право на эти данные.

Проход 2 (замысел). Синхронная выгрузка отчёта за произвольный период в HTTP-ответе. За год данных это может быть сотня тысяч строк и таймаут. Решение спорное само по себе, но менять архитектуру в этом PR, возможно, не нужно — достаточно ограничить период.

Проход 3 (корректность). len(get_items(o.id)) равен нулю для заказа без позиций — деление на ноль. Плюс get_items вызывается дважды на каждый заказ, и оба раза внутри цикла: классический N+1.

Проход 4 (сопровождаемость). Тестов на новое поведение нет вообще, хотя CI зелёный — он просто не знает, что здесь что-то появилось.

Итоговые комментарии:

Обратите внимание, чего в комментариях нет: замечаний про именование o, i, rows. Они бы утонули среди трёх blocker'ов и только размыли сигнал. Именование обсудим следующим PR, когда дыра в авторизации будет закрыта.

Чем закончилось. Автор перенёс user_id в зависимость от сессии, перешёл на параметры запроса, добавил ранний выход для пустых позиций и один агрегирующий запрос вместо цикла. Лимит периода вынесли в отдельный тикет — это оказалось продуктовое решение, а не техническое.

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

Героическое ревью. PR на 2000 строк, одобренный за десять минут. Внимание ревьюера кончается примерно на четырёхсотой строке диффа, дальше он листает. Лекарство на стороне автора: PR больше 400 строк надо разбивать. Лекарство на стороне ревьюера: честно написать «PR слишком велик, чтобы я мог за него отвечать, давай разобьём».

Пассивная агрессия. «Интересное решение… а ты тесты запускал?» Вопрос, который на самом деле утверждение, — худший формат замечания: автор считывает насмешку и защищается вместо того, чтобы исправлять. Пишите прямо: «на этом пути нет теста, добавь, пожалуйста».

Ревью личности вместо кода. «Ты опять забыл обработку ошибок» и «здесь нет обработки ошибок для сетевого запроса» описывают один факт, но первое приглашает к обороне, а второе — к правке.

Проверьте себя. Найдите в истории репозитория самый большой смерженный PR и посчитайте, сколько в нём комментариев на сто строк. Сравните с обычным PR.

Частая ошибка. Списывать поверхностное ревью на невнимательность конкретного человека, когда причина в размере PR.

Процедура из этого урока — каркас. Следующие темы наполняют проходы 3 и 4 содержанием: что именно искать в логике, безопасности, производительности, тестах и документации.

Потренироваться в поиске проблем прямо сейчас: Code Review Python и Code Review React — фрагменты кода с несколькими настоящими проблемами и ложными следами. Написать полноценное ревью свободным текстом — Арена.