Код-ревью — одна из немногих практик, которая одновременно ловит дефекты, распространяет знание о системе и удерживает единый стиль. И одна из немногих, которая при плохой организации портит отношения в команде и растягивает выкат на дни.
Разница между этими двумя состояниями — не в людях, а в четырёх-пяти явно записанных правилах. Как проходить ревью со стороны автора — отдельная тема; здесь речь о том, что настраивает лид.
Три вещи, которые ревью не должно делать
Начать полезно с вычитания. Ревью не должно:
Проверять форматирование. Отступы, порядок импортов, кавычки — работа автоформатирования и линтера на входе. Каждое замечание про пробел — это потраченное внимание, которое не досталось логике.
Ловить то, что ловят тесты и анализаторы. Пустой блок перехвата ошибки, неиспользуемая переменная, потенциальный null — задача статического анализа. Человек читает то, что машина не умеет: правильно ли решена задача, не сломается ли это в проде, поймёт ли это следующий.
Быть местом первого знакомства с задачей. Если ревьюер впервые видит, что вообще делается, спор будет не о коде, а о самом решении — и это уже дорого. Крупное решение обсуждают до кода: в ADR или коротким разбором дизайна.
Всё, что можно снять автоматикой, стоит снять автоматикой. Именно это делает исполняемый стандарт: правила лежат в проверках, а не в голове самого дотошного участника.
Что ревью должно делать
Остаётся три вопроса, ради которых и собираются:
- Решает ли изменение заявленную задачу — целиком и то самое.
- Не сломает ли оно то, что уже работает — совместимость, миграции, поведение под нагрузкой, поведение при ошибках.
- Сможет ли это поддерживать другой человек — понятны ли имена, не спрятана ли сложность, есть ли тесты на неочевидное.
Если замечание не относится ни к одному из трёх — это, скорее всего, вкусовщина, и с ней отдельный разговор ниже.
Нормы, которые стоит записать
Устные договорённости живут до первого конфликта. Записанные — это то, на что можно сослаться, не переходя на личности.
Размер изменения. Верхняя граница — примерно 400 строк содержательных правок. Выше этого качество ревью падает обвально: читающий перестаёт видеть детали и начинает просматривать. Большое изменение автор режет сам — по слоям, по шагам, по «сначала подготовка, потом суть».
Срок ответа. Например: изменение, открытое до обеда, получает первый ответ в тот же день. Не «идеальный разбор», а первый ответ. Задержка в ревью — самый дешёвый способ растянуть выкат и самый незаметный: она нигде не видна.
Число ревьюеров. Один по умолчанию, два — на изменения в критичных местах (деньги, доступ, миграции). Требование «одобрили все» превращается в ожидание отпусков.
Что блокирует слияние. Только две вещи: дефект и нарушение записанного стандарта. Всё остальное — предложение, которое автор волен не принять. Это правило снимает большую часть конфликтов, потому что переводит спор из «кто главнее» в «покажи правило».
Форма замечания. О коде, а не об авторе. «Здесь при пустом списке будет исключение» вместо «ты не подумал о пустом списке». Разница кажется косметической ровно до того момента, когда в команде появляется человек, который перестал открывать свои изменения на ревью.
Спор о вкусах: как гасить
Спор в ревью почти всегда одного типа: два опытных инженера по-разному видят «как правильно», и оба правы. Часовая переписка в комментариях не сходится никогда, потому что предмета спора нет — есть две допустимые нормы.
Рабочее правило: спор о вкусах не решается в ревью — он выносится в стандарт. Три шага:
- Изменение сливается по текущим правилам (если это не дефект).
- Вопрос уходит в обсуждение стандарта — отдельно, спокойно, вне контекста конкретного изменения.
- Что бы ни решили, решение записывается и по возможности переносится в автоматическую проверку. Дальше это уже не вкус, а правило, и следующий такой спор занимает ноль минут.
Побочный эффект приятный: за полгода такой практики стандарт наполняется реальными решениями команды, а не переписанными из книжки.
Отдельный случай — когда один участник систематически доминирует в тредах: длинные разборы, переписывание чужого кода в комментариях, блокировка по вкусу. Это не проблема ревью, это разговор один на один, с конкретными примерами и явной договорённостью, что считается блокирующим. Общий канал для такого разговора не годится.
Когда ревью стало узким местом
Признаки: изменения висят днями, автор переключается на другую задачу и теряет контекст, к моменту ответа он уже не помнит, зачем так написал, а список открытых изменений растёт.
Что помогает, по убыванию отдачи:
- Уменьшить изменения. Половина проблемы со скоростью — это размер. Мелкое изменение читают за десять минут между делом, крупное откладывают «на когда будет время», а времени не бывает.
- Убрать из ревью автоматизируемое. Каждое правило, переехавшее в проверку, снимает несколько замечаний с каждого изменения.
- Расширить круг ревьюеров. Если весь код смотрит один человек, у команды не ревью, а очередь к нему. Дежурство по ревью на неделю распределяет нагрузку и заодно распределяет знание о системе.
- Разделить ревью по глубине. Не всё требует одинакового внимания: правка текста в интерфейсе и изменение схемы базы — разные весовые категории. Явно договоритесь, что смотрится бегло, а что — подробно.
- Иногда — заменить ревью. Парное программирование, о котором говорит XP, решает те же три вопроса синхронно и без ожидания. Это не для каждой задачи, но для сложной и незнакомой часто быстрее.
Как измерить, что стало лучше
Три числа, которые видно в любом инструменте и которые не требуют отдельного учёта:
- время до первого ответа — главный показатель уважения к чужому времени;
- время от открытия до слияния — сколько работа лежит готовой, но невыкаченной;
- средний размер изменения — предсказывает оба предыдущих лучше всего.
Превращать их в личные показатели не стоит: любой такой показатель немедленно начинают набивать. Это градусник для процесса, а не оценка людей.
Коротко
- Ревью не занимается форматированием, тем, что ловят анализаторы, и обсуждением ещё не принятого решения.
- Три вопроса ревью: решает ли задачу, не сломает ли работающее, сможет ли поддерживать другой.
- Записанные нормы: размер изменения, срок первого ответа, число ревьюеров, что блокирует слияние, форма замечания.
- Блокируют только дефект и нарушение стандарта; остальное — предложение.
- Спор о вкусах выносится в стандарт, а не решается в переписке под изменением; решение переносится в автоматическую проверку.
- Узкое место чинится размером изменений, автоматикой и распределением ревьюеров, а не призывом «ревьюить быстрее».
Что почитать дальше
- Pull request и код-ревью — та же практика со стороны автора изменения.
- Исполняемый инженерный стандарт — как превратить договорённости команды в проверки.
- Extreme Programming — парное программирование и коллективное владение кодом.