code-review-that-works.md — vim

Code review, который не бесит

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


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

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

Маленький PR

Самое важное. Есть классические данные по инспекциям кода (исследование SmartBear на базе Cisco): эффективность поиска дефектов резко падает после 200-400 строк за один просмотр, а комфортная скорость — до 300-500 строк в час.

Что это значит на практике: PR на полторы тысячи строк не ревьюится. Он аппрувится. Разница принципиальная, и её стоит проговорить вслух в команде, потому что многие искренне думают, что «посмотрели».

Мой порог — 400 строк диффа. Больше — прошу разбить, и это не вкусовщина, а прямое следствие цифр выше.

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

Быстрый ответ важнее идеального

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

У нас нормой было четыре часа внутри рабочего дня. Занят — скажи сразу, а не молчи.

Разделяй «блокирует» и «мнение»

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

[blocker]  - без исправления не мержим (баг, дыра, сломанный контракт)
[should]   - надо поправить, можно следующим PR
[nit]      - придирка, автор решает сам
[question] - я не понял, объясни

И главное правило: [nit] не может блокировать мерж. Если у тебя одни ниты - ставь аппрув и оставляй комментарии.

Комментируй код, а не человека

Техническое содержание одинаковое, исход разговора - разный:

Ты опять не проверил null
   → Здесь user может быть null после logout - упадёт на user.name.
     Предлагаю optional chaining

Это плохой код
   → Этот watch срабатывает на каждый рендер: объект в зависимостях
     пересоздаётся. Может, следить за конкретным полем?

Во втором варианте есть что не так, когда сломается и что делать. В первом - только оценка. С лидов я это спрашиваю строже, чем с остальных: тон ревью в команде задаёт тот, кого читают все.

Чек-лист, чтобы не скатиться в стилистику

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

  • граничные случаи: пустой массив, null, ошибка сети, двойной клик;
  • безопасность: что попадает в логи, что уходит в URL, проверяются ли права не только на фронте;
  • контракт: не сломает ли это других потребителей компонента или API;
  • читаемость: пойму ли я это через полгода без автора;
  • есть ли тест на исправленный баг;
  • производительность: запрос в цикле, лишний ререндер, тяжёлое в главном потоке.

Спор о пробелах на ревью - это не про культуру команды, это просто признак ненастроенного prettier.

Аппрув - это подпись

Формулировка, которую я говорю новым лидам: поставив аппрув, ты стал соавтором. Не «глянул по диагонали», а «согласен, что это можно выкатывать». Дисциплинирует лучше любого регламента и заодно снимает с автора одиночную ответственность за инцидент - разбираться потом будем вдвоём.

Побочные эффекты, ради которых всё и делается

Джун за три месяца ревью узнаёт о кодовой базе больше, чем за год самостоятельного ковыряния. Бас-фактор падает - каждый кусок кода видели минимум двое. Стандарты перестают быть мёртвым файлом в вики, потому что обсуждаются на конкретных примерах.

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

Ссылки