Чек-лист код-ревью: что реально ловит баги, а что — ритуал

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

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

Эта статья разбирает типовой чек-лист код-ревью по пунктам — какие из них стоит держать в голове у человека, а какие давно пора делегировать линтеру, статическому анализатору и CI.

Зачем вообще нужен чек-лист код-ревью

Чек-лист код-ревью — это согласованный список критериев, по которым команда оценивает pull request перед слиянием в основную ветку. Его цель не в том, чтобы формально закрыть процедуру, а в том, чтобы сделать ревью воспроизводимым: качество проверки не должно зависеть от того, насколько внимателен конкретный ревьюер сегодня и сколько у него осталось времени до встречи.

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

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

Какие пункты чек-листа реально ловят баги

Логика и граничные случаи — что проверить

Логика и граничные случаи — это ядро код-ревью, то, что линтер в принципе не способен проверить, потому что для этого нужно понимать замысел задачи, а не синтаксис кода. Ревьюер читает не «правильный ли это код PHP», а «делает ли этот код то, что должен, во всех ситуациях, которые реально случатся».

Практический набор вопросов для этого пункта: что произойдёт, если входная коллекция пустая? Что если число равно нулю или отрицательное там, где ожидался положительный id? Обрабатывается ли случай, когда внешний API вернул не те данные, которые описаны в документации? Эти вопросы редко приходят в голову автору кода — он пишет для «счастливого пути», где всё работает как задумано, а ревьюер как раз и нужен для взгляда со стороны, свободного от этого туннельного зрения.

Обработка ошибок и то, что происходит, когда что-то пошло не так

Обработка ошибок — отдельная категория от общей логики, потому что код почти всегда пишут и тестируют на успешном сценарии, а ошибки обнаруживаются уже в проде. Ревьюер проверяет не «есть ли try/catch», а что именно происходит после перехвата: ошибка молча проглатывается и теряется, или она логируется с достаточным контекстом, чтобы потом можно было понять причину?

Частый пропуск на ревью — пустой catch-блок, который делает исключение полностью невидимым: код не падает, но и не работает правильно, а диагностировать проблему потом почти невозможно, потому что в логах нет ни единого следа. Второй частый пропуск — слишком широкий catch, который ловит любое исключение подряд, включая те, что должны были остановить выполнение, а не быть тихо проигнорированными.

Безопасность: ввод данных, права доступа, секреты в коде

Безопасность — пункт, где цена пропуска на ревью несопоставима с ценой самой проверки: несколько секунд внимания против потенциальной утечки данных или компрометации сервера. Три базовых вопроса покрывают большую часть практических рисков: валидируется ли и экранируется ли ввод от пользователя перед использованием в запросе к базе или выводом на страницу; проверяется ли, что пользователь действительно имеет право на операцию, которую выполняет (а не только то, что он авторизован вообще); не попал ли в дифф секрет — токен, пароль, ключ API — захардкоженный прямо в коде.

Последний пункт стоит проверять в первую очередь, потому что его легче всего упустить: разработчик временно вставляет реальный ключ для локальной отладки, забывает его убрать перед коммитом, и теперь он лежит в истории git — удалить его из diff недостаточно, если коммит уже запушен.

Тестируемость — не «есть ли тесты», а «проверяют ли они то, что нужно»

Вопрос «покрыт ли код тестами» на ревью задают почти всегда, но это неправильный вопрос — он легко превращается в формальную галочку: тесты есть, значит, пункт закрыт. Правильный вопрос — проверяют ли эти тесты именно граничные случаи и реальные сценарии отказа, а не просто повторяют happy path, который и так работает без всяких тестов.

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

Производительность — когда это действительно важно на ревью, а когда рано

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

Стоит целенаправленно искать конкретные паттерны, которые почти всегда становятся проблемой при росте данных: запрос к базе внутри цикла (N+1-проблема) — классика, которая незаметна на тестовых данных из десяти записей и превращается в секунды задержки на проде с десятью тысячами; отсутствие индекса на поле, по которому фильтруется большая таблица; загрузка всей коллекции в память там, где можно обрабатывать её порциями или потоком. Остальные вопросы производительности — микрооптимизации на уровне отдельных операций — на ревью почти никогда не окупают потраченное на обсуждение время.

Что из чек-листа — ритуал, а не защита от багов

Эта часть чек-листа не экономит команде ни одного инцидента в проде, но стабильно отнимает время ревьюера — и именно поэтому она заслуживает отдельного разбора, а не молчаливого включения в общий список «на всякий случай».

Ритуал в оформлении кода

• Споры об отступах и форматировании. Если в проекте нет настроенного автоформаттера (Prettier, php-cs-fixer, аналог для своего стека), ревьюер тратит время на то, что решается конфигурационным файлом за один раз, а не повторяющимся обсуждением в каждом PR.
• Споры о «магических числах» и именовании переменных на уровне вкуса. Вынести 86400 в константу SECONDS_IN_DAY — разумно, это снимает двусмысленность. А спор «назвать переменную items или itemList» — это уже вкусовщина, которая не влияет на читаемость заметно и чаще отражает личные предпочтения ревьюера, чем объективную проблему кода.

Ритуал в самом процессе ревью

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

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

Что проверять автоматически, а что — человеку

Практичный способ перестроить чек-лист — явно развести каждую строку на «это должен ловить инструмент» и «это может оценить только человек, понимающий контекст задачи».

Таблица работает как практичный фильтр при пересмотре собственного чек-листа: если строка целиком попадает в левый столбец «автоматизируется» — её место в конфиге CI, а не в голове ревьюера, отвлекающей внимание от строк из правого столбца.

Документация и код-ревью: что фиксировать после слияния

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

Если решение в PR нетривиально — выбран конкретный подход вместо очевидной альтернативы, обойдено ограничение внешнего API, принят сознательный компромисс по производительности — полезно зафиксировать «почему» прямо рядом с кодом: в комментарии, в описании PR или в краткой заметке в документации проекта. Без этого следующий ревьюер или разработчик видит только «что», но не «почему», и рискует откатить осознанное решение как случайную ошибку.

Как внедрить чек-лист без лишней бюрократии

Пошаговый план

1. Начать с 3-5 пунктов, которые реально ловят баги в вашем проекте — не копировать чужой список на 30 строк целиком, а взять то, что уже вызывало инциденты.
2. Автоматизировать повторяющиеся замечания линтером и SAST-сканером в CI — если один и тот же комментарий ревьюер пишет третий раз подряд, это сигнал перенести проверку в инструмент.
3. Пересматривать чек-лист раз в квартал, а не оставлять его неизменным годами — новые типы ошибок появляются вместе с новыми частями системы.
4. Не превращать ревью в блокер релиза на формальных основаниях — разделять «блокирующие» и «необязательные» замечания явно, например меткой nit для последних.
5. Фиксировать метрику не «число замечаний на PR», а «число багов, пойманных на ревью до прода» — первая метрика поощряет придирки, вторая — реальную пользу процесса.
6. Делиться найденными на ревью паттернами с командой — если один и тот же баг повторяется у разных авторов, это повод для короткой заметки или пункта в чек-листе, а не для молчаливого исправления раз за разом.

Если захочется пойти дальше самого процесса ревью и разобрать конкретные антипаттерны в структуре кода, которые регулярно всплывают в таких проверках, — отдельный разбор есть в каталоге code smell для начинающих разработчиков: там собраны конкретные примеры длинного метода, класса-бога и дублирования с техниками исправления.

Частые вопросы

Сколько строк кода можно просматривать за один ревью?

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

Чем код-ревью отличается от статического анализа и линтера?

Линтер и статический анализ проверяют формальные правила — стиль, типы, известные паттерны уязвимостей — без понимания бизнес-задачи. Код-ревью человеком добавляет то, что инструмент не видит: соответствует ли решение задаче, не упущен ли граничный случай, оправдан ли выбранный подход в контексте остальной системы. Это разные уровни проверки, а не конкурирующие инструменты.

Нужно ли ревьюить свой собственный код перед PR?

Да, и это один из самых недооценённых шагов. Self-review прямо в интерфейсе PR до назначения ревьюера часто находит опечатки, забытые отладочные строки и логические нестыковки — то, что автор не видел в редакторе, но замечает, читая собственный дифф со стороны. Это экономит время ревьюера и ускоряет весь цикл.

Как не превращать ревью в конфликт между разработчиками?

Комментарий формулируют как вопрос или предложение к коду, а не оценку автора: «что будет, если входной список окажется пустым?» работает лучше, чем «тут ошибка». Полезно явно помечать необязательные придирки меткой вроде nit, чтобы автор понимал, что не обязан исправлять это немедленно для одобрения PR.

Что делать, если ревьюер и автор PR не согласны?

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

Как часто пересматривать сам чек-лист ревью?

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

Нужен ли отдельный чек-лист для джуниоров?

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

Сравнение типовых пунктов чек-листа код-ревью и практик проведения ревью опирается на разборы отрасли, включая материал tproger.ru о чек-листах для код-ревью.