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.
Аппрув - это подпись
Формулировка, которую я говорю новым лидам: поставив аппрув, ты стал соавтором. Не «глянул по диагонали», а «согласен, что это можно выкатывать». Дисциплинирует лучше любого регламента и заодно снимает с автора одиночную ответственность за инцидент - разбираться потом будем вдвоём.
Побочные эффекты, ради которых всё и делается
Джун за три месяца ревью узнаёт о кодовой базе больше, чем за год самостоятельного ковыряния. Бас-фактор падает - каждый кусок кода видели минимум двое. Стандарты перестают быть мёртвым файлом в вики, потому что обсуждаются на конкретных примерах.
Если у тебя ревью висит по два дня - начни не с регламентов, а с размера пул-реквестов. Обычно этого достаточно.
Ссылки
- Code Review Developer Guide от Google
- Про скорость ревью и как писать комментарии
- SmartBear: цифры по объёму и скорости просмотра
- Conventional Comments - если хочется формализовать префиксы