Топ 7 правил полезного код-ревью
Код-ревью или заметно улучшает код и команду, или превращается в формальность с одним комментарием про лишний пробел. Семь правил, которые отличают первое от второго.
Зачем это нужно на самом деле
Ловля ошибок — только одна из целей, и не главная. Тесты и статический анализ находят баги дешевле и быстрее, чем человек, читающий диф.
Главное, что даёт ревью, — распространение знания. После него в команде минимум два человека понимают, как работает эта часть системы. Это страховка от ситуации «уволился единственный, кто разбирался в биллинге», и способ выровнять подходы: как у нас принято обрабатывать ошибки, куда класть бизнес-логику, что считается достаточным покрытием тестами.
Второй эффект — дисциплина автора. Человек, который знает, что его код прочитают, пишет иначе: убирает отладочные строки, придумывает нормальные имена, дописывает тест. Значительная часть пользы возникает ещё до того, как ревьюер открыл вкладку.
И честная оговорка. Ревью стоит времени двух человек и замедляет доставку изменений. Плохо организованное ревью замедляет её сильно, а пользы даёт мало — поэтому правила ниже в основном про то, как не потратить это время впустую.
Топ 7 правил
Маленькие изменения
правило, от которого зависят все остальные
Внимательно прочитать можно ограниченный объём кода. Дальше начинается имитация: человек листает диф, видит стену незнакомого текста и пишет комментарий про форматирование, потому что суть охватить уже не получается. Отсюда знакомая закономерность: изменение на двадцать строк собирает пять содержательных замечаний, изменение на тысячу — одно «выглядит нормально».
Практический ориентир. Столько, сколько читается вдумчиво за полчаса. Если ревьюеру нужно два подхода — изменение стоило разбить.
Как дробить. Отдельно переименования и перекладывание кода, отдельно изменение поведения. Смешивать их — самый надёжный способ спрятать баг: среди трёхсот строк механических правок одна содержательная не видна никому.
Если задача большая. Разбейте её на цепочку изменений, каждое из которых само по себе осмысленно и не ломает систему. Это дороже для автора и в разы дешевле для команды.
- Резко повышает качество замечаний
- Ревью занимает минуты, а не полдня
- Мелкие изменения реже конфликтуют при слиянии
- Требует дисциплины и умения планировать работу кусками
- Цепочки зависимых веток неудобны в большинстве инструментов
- Не всякую задачу вообще получается разрезать
главное правило · оценка 9,5
Автоматика до человека
человек не должен работать линтером
Отступы, кавычки, порядок импортов, неиспользуемые переменные, очевидные подозрительные конструкции — всё это находит машина. Если такие замечания появляются в ревью, значит, у вас не настроены инструменты, и вы тратите на это время живых людей.
Минимальный набор. Форматер, который приводит код к единому виду автоматически. Линтер с согласованным набором правил. Тесты и сборка, запускающиеся до ревью. Всё это должно проходить в автоматической проверке, а не запускаться вручную по памяти.
Побочный эффект — конец споров о вкусах. Когда форматирование делает машина, обсуждать его бессмысленно. Команда один раз договаривается о настройках и больше к этому не возвращается.
Что остаётся человеку. Правильность логики и крайние случаи. Понятность имён и структуры. Безопасность и работа с чужими данными. Соответствие тому, как принято в проекте. Ровно то, чего машина не умеет.
- Убирает из ревью весь шум разом
- Настраивается один раз на весь проект
- Прекращает споры о вкусовщине
- Внедрение в старый проект даёт огромную разовую правку
- Слишком строгий линтер начинает мешать и его учатся обходить
- Медленная автоматическая проверка сама становится узким местом
фундамент · оценка 9,4
Автор объясняет контекст
ревью начинается не с кода, а с описания
Ревьюер не сидел с вами на обсуждении задачи и не помнит, почему выбран именно такой подход. Описание должно отвечать на три вопроса: что меняется, зачем это нужно и как вы это проверили. Плюс ссылка на задачу и, если менялся интерфейс, картинка или короткая запись экрана.
Самый недооценённый приём. Прочитайте собственный диф перед тем, как звать людей. Половина будущих комментариев снимается прямо там: забытая отладочная строка, закомментированный кусок, файл, попавший случайно, кусок, который вы сами уже не понимаете.
Комментарии в диффе от автора. Пояснить неочевидное место прямо в обсуждении — вежливо и экономит круг вопросов. Если объяснение нужно и через полгода — ему место в коде или в документации, а не в комментарии к изменению.
- Экономит целый круг вопросов и ответов
- Заставляет автора самого перечитать изменения
- Описание остаётся в истории проекта и помогает потом
- Тратит время автора, особенно на мелких правках
- Шаблон описания легко превращается в формальность
на авторе · оценка 9,3
Разделять «это блокирует» и «на подумать»
без этого автор либо чинит всё подряд, либо игнорирует всё подряд
Комментарии бывают разного веса: «здесь падает на пустом списке» и «мне кажется, тут лучше подошло бы другое имя» — не одно и то же. Если их не различать, автор вынужден угадывать, что обязательно, а что мнение.
Что работает. Явные пометки в начале комментария: обязательное к исправлению, желательное, необязательное замечание, просто вопрос. Достаточно трёх-четырёх слов-меток, о которых команда договорилась один раз.
Нормальный исход ревью — одобрение с замечаниями. Не всё нужно проверять повторно. Если остались только мелочи, доверьтесь автору: он поправит и вольёт сам. Иначе ревью превращается в бесконечный пинг-понг из-за запятых.
Требуйте обоснования у блокирующих замечаний. «Так не надо делать» — не аргумент. «Так не надо делать, потому что при повторной отправке формы создастся два заказа» — аргумент.
- Автор сразу понимает, что делать
- Убирает лишние круги проверки
- Дисциплинирует ревьюера: блокировать надо уметь объяснить
- Нужна общекомандная договорённость, в одиночку не работает
- Пометкой «необязательно» иногда прикрывают важное замечание
на команде · оценка 9,2
Комментарий про код, а не про человека
от тона зависит, будут ли вас вообще звать на ревью
«Ты не подумал про пустой список» и «здесь на пустом списке будет ошибка» описывают одно и то же, но читаются по-разному. Первое — про автора, второе — про код. Второе исправят молча, первое обсудят в курилке.
Вопрос сильнее приговора. «Почему здесь именно так?» часто выясняет, что автор знал что-то, чего не знали вы. А если не знал — он сам придёт к нужному выводу, и это дешевле спора.
Предлагайте вариант. Замечание без альтернативы оставляет автора в тупике. Даже приблизительный набросок «можно было бы вынести это в отдельную функцию и вызвать из обоих мест» переводит разговор в конструктив.
Про асимметрию. Комментарий опытного разработчика новичок читает как приказ, даже если это была мысль вслух. Поэтому старшим в команде особенно полезны пометки веса и явные слова «это не блокирует».
И да, хвалить нужно. Одна строчка «хорошее решение, я бы не додумался» стоит недорого и заметно меняет отношение к ревью в команде.
- Ревью перестаёт восприниматься как экзамен
- Замечания реже вызывают споры
- Ничего не стоит и не требует внедрения
- В письменном виде тон легко читается холоднее, чем задумано
- Вежливость иногда размывает суть замечания
на ревьюере · оценка 9,1
Отвечать быстро
ревью, висящее третий день, вредит больше, чем его отсутствие
Пока изменение ждёт, автор либо простаивает, либо начинает следующую задачу поверх непринятой ветки. Основная ветка тем временем уходит вперёд, и к моменту слияния появляются конфликты, которых не было бы вчера.
Разумный ориентир. Посмотреть в течение рабочего дня. Не мгновенно — прерывания дороги, и выдёргивать человека из работы ради каждого изменения хуже, чем задержка на несколько часов.
Что помогает. Пара выделенных окон в дне под ревью — например, после утренней встречи и перед концом дня. И договорённость, что мелкие изменения смотрят сразу: на диф в двадцать строк уходит меньше времени, чем на переключение контекста.
Если посмотреть сегодня не получится — напишите об этом. Одна строка «возьму завтра утром» решает проблему неопределённости; автор просто спланирует день иначе.
- Меньше простоя и меньше конфликтов при слиянии
- Изменения не копятся стопкой к концу недели
- Требует договорённости, а не инструментов
- Прерывания вредят собственной работе ревьюера
- Спешка снижает качество чтения — быстро не значит бегло
на команде · оценка 9,0
Два круга спора — переходите в разговор
переписка отлично фиксирует и очень плохо убеждает
Если в ветке комментариев уже десяток сообщений и стороны повторяют аргументы, обсуждение зашло в тупик. Пять минут голосом решают то, на что в переписке уходит полдня и запас доброжелательности.
Обязательное продолжение. Итог разговора запишите обратно в обсуждение: к чему пришли и почему. Иначе через месяц спор повторится с теми же аргументами, а решение останется в голове двоих.
Проверьте уровень спора. Если разногласие про архитектуру, а не про конкретные строки, ревью — неподходящее место: подход надо было обсуждать до того, как код написан. Правильный выход — вливать текущее, если оно не вредит, и выносить архитектурный вопрос отдельно.
Когда не договорились. Нужен заранее оговорённый способ решить: правило проекта, слово ответственного за эту часть системы или короткое обсуждение командой. Побеждать не должен тот, кто упрямее.
- Экономит часы и нервы на затяжных обсуждениях
- Голосом быстро выясняется, что спорили о разном
- Записанный итог закрывает вопрос надолго
- Плохо работает при разных часовых поясах
- Устные договорённости теряются, если поленились записать
- Разговор — тоже прерывание, и не бесплатное
по ситуации · оценка 8,8
Сравнение по главному
| Правило | Кого касается | Эффект | Внедрить |
|---|---|---|---|
| Маленькие изменения | автор | высокий | привычка |
| Автоматика до человека | команда | высокий | день настройки |
| Контекст в описании | автор | высокий | сразу |
| Пометки веса замечаний | команда | средний | сразу |
| Тон комментариев | ревьюер | высокий | привычка |
| Скорость ответа | команда | средний | договориться |
| Спор переводить в разговор | оба | средний | сразу |
На что смотреть, когда открыл диф
Сначала целиком, потом построчно. Первый проход — понять, что вообще происходит и туда ли автор пошёл. Замечания по строкам, написанные до того, как поняли замысел, обычно приходится отзывать.
Крайние случаи. Пустой список, отсутствующее значение, ноль, отрицательное число, очень длинная строка, повторный вызов. Здесь живёт большая часть настоящих багов.
Ошибки и отказы. Что произойдёт, если чужой сервис не ответит или база вернёт ошибку. Проглоченное исключение без записи в лог — почти всегда замечание.
Данные и безопасность. Проверка входных данных, права доступа, отсутствие секретов в коде, персональные данные в логах.
Имена и структура. Понятно ли будет через полгода человеку, который этого кода не писал. Про это у нас есть отдельный разбор с конкретными приёмами.
Обратная совместимость. Изменения в схеме базы, в формате ответов, в очередях. Что будет в момент выкладки, когда старая и новая версии работают одновременно.
И чего делать не надо. Переписывать чужое решение на своё, если оно просто другое, а не хуже. Требовать изменений вне задачи. Обсуждать то, что должна проверять автоматика.
Частые вопросы
Нужно ли ревью в команде из двух человек?
Да, и особенно там: в маленькой команде цена «уникального знания в одной голове» максимальна. Формальности можно сократить до минимума — иногда достаточно короткого разбора экрана вдвоём. Главное, чтобы про каждое существенное изменение знал не один человек.
Сколько ревьюеров назначать?
Обычно хватает одного, знакомого с этой частью системы. Второй имеет смысл, когда изменение затрагивает чужую зону или несёт заметный риск. Назначать всю команду бесполезно: при общей ответственности изменение просто дольше висит, а читают его невнимательнее.
Что делать, если не согласен с более опытным коллегой?
Спросить о причине, а не спорить о выводе: часто за замечанием стоит история, которую вы не застали. Если аргумент не убедил, скажите об этом прямо и предложите обсудить голосом. Опыт — не аргумент сам по себе, но и упрямство тоже; заранее оговорённый способ разрешать разногласия экономит всем время.
Можно ли вливать без ревью?
В аварии — да, когда прод лежит и счёт идёт на минуты. Но с двумя условиями: изменение минимальное и разбор проводится сразу после, а не «когда-нибудь». Если исключение случается каждую неделю, дело не в авариях, а в процессе.
Итог
Если менять что-то одно — меняйте размер изменений. Маленькие дифы автоматически чинят и качество замечаний, и скорость ответа, и настроение участников. Всё остальное поверх этого работает, а без этого — нет.
Второе по важности — вынести всё механическое в автоматику, чтобы люди обсуждали смысл, а не отступы. А дальше достаточно двух договорённостей: помечать вес замечаний и не спорить в переписке дольше двух кругов.
Комментарии
0Будьте первым, кто ответит автору.