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

В новом переводе от команды Spring АйО эксперты сообщества подробно и на собственном опыте рассказали про нюансы код-ревью.

За последние два года код-ревью стало значительно важнее. Генерация кода с помощью LLM теперь проста, но проверка кода по-прежнему так же сложна.

Многие инженеры-программисты теперь тратят столько же (или даже больше) времени на проверку кода, сгенерированного их собственными ИИ-инструментами, сколько на ревью кода своих коллег.

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

Не ограничивайтесь только просмотром diff

Самая большая ошибка, которую я наблюдаю, — это ревью, сосредоточенное исключительно на diff.

Большинство самых ценных комментариев при код-ревью почти не связаны с самим diff, а основаны на вашем понимании остальной части системы.

Например, один из самых очевидно полезных комментариев — «этот метод не нужно добавлять здесь, он уже есть в другом месте». Сам diff не даст вам такой комментарий. Вы должны заранее знать другие части кодовой базы, о которых автор diff`а может не знать.

Аналогично, замечания вроде «этот код лучше перенести в другой файл» очень важны для поддержания качества кодовой базы в долгосрочной перспективе. Главный принцип при работе с крупными кодовыми базами — это консистентность (подробнее об этом я пишу в тексте «Ошибки, которые инженеры совершают в больших устоявшихся кодовых базах»). Очевидно, по одному только diff судить о консистентности всего предполагаемого изменения просто невозможно.

Рассматривать только diff гораздо проще, чем анализировать, как изменения вписываются в кодовую базу в целом. Вы можете быстро пробежать diff и оставить построчные комментарии (например: «переименуй эту переменную» или «эта функция должна быть устроена иначе»). Такие комментарии могут быть даже полезны! Но, ограничиваясь только таким подходом, вы упускаете значительную часть пользы от код-ревью.

Не оставляйте слишком много комментариев

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

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

Из этого правила есть как минимум одно исключение. Когда вы нанимаете нового инженера в команду, может быть полезно оставить множество стилистических комментариев, чтобы помочь ему разобраться в специфике «код стайла», принятого в конкретной кодовой базе. Но даже в этом случае стоит помнить, что любые важные замечания, скорее всего, затеряются среди этих мелочей. Возможно, даже лучше будет оставить общий комментарий вроде «здесь мы не используем ранние выходы (early return) из функций», чем делать отдельное замечание к каждому случаю early return в diff.

Не проводите ревью через призму «как бы я это написал»

Одна из причин, по которой инженеры оставляют слишком много комментариев, заключается в следующем подходе к ревью:

1. Посмотреть на фрагмент diff

2. Спросить себя: «А как бы я написал этот код?»

3. Оставить комментарий по каждому расхождению между своим вариантом и фактическим кодом

Такой подход легко приводит к сотне замечаний в pull request: бесконечный поток комментариев вроде «я бы выполнил эти две операции в другом порядке» или «я бы немного иначе структурировал эту функцию» и так далее.

Я не говорю, что такие мелкие замечания всегда плохи. Иногда порядок операций действительно имеет значение, или функции действительно плохо структурированы. Но одна из моих ключевых позиций в инженерии программного обеспечения — это то, что практически любую задачу можно решить несколькими допустимыми способами, и выбор подхода часто зависит от личных предпочтений.

Как рецензент, вы должны уметь одобрять решения, отличные от ваших собственных, без комментариев — при условии, что оба варианта допустимы. В противном случае вы ставите своих коллег в неудобное положение. Они либо согласятся со всеми вашими замечаниями, чтобы избежать конфликта (что приведёт к напрасной потере времени и фактически говорит о вас, как об обладателе "синдрома вахтера"), либо будут спорить по каждому пустяку, что займёт ещё больше времени. Код-ревью — это не момент, когда вы должны навязывать коллегам свои личные предпочтения.

Если вы не хотите, чтобы изменение было замержено, оставьте блокирующее ревью

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

Если вы хотите заблокировать изменение — оставьте блокирующее ревью. Многие инженеры считают невежливым оставлять блокирующий отзыв, даже если видят серьёзные проблемы, и вместо этого просто пишут комментарии, описывая эти проблемы. Не делайте так. Это формирует культуру, в которой никто не понимает, можно мержить изменения или нет. Одобрение должно означать: «Я не против, если ты замержишь, даже проигнорировав мои комментарии». Просто комментарии без статуса — «Я не против, если ты замержишь после одобрения другого участника, даже если проигнорируешь мои замечания». Если вас расстроит, что изменение замержили — вы должны оставить блокирующее ревью. Тогда автор изменений будет точно знать, можно мержить или нет, и ему не придётся бегать по всем комментаторам, чтобы получить их неформальное одобрение.

Большинство ревью должно завершаться одобрением

Начну с оговорки: многое зависит от типа кодовой базы, о которой идёт речь. Например, я считаю вполне допустимым, если pull request’ы в такие проекты, как SQLite, чаще всего получают блокирующее ревью. Но в стандартной SaaS-кодовой базе, где команды активно разрабатывают новые функции, подавляющее большинство ревью должно завершаться одобрением. Я подробно разбираю различие между этими двумя типами кодовых баз в тексте Pure and Impure Engineering.

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

У инженеров в этой “платформенной” команде может быть даже другое название должности, например SRE, или они вообще могут принадлежать к другой организационной структуре. Их мотивация, таким образом, не совпадает с мотивацией продуктовых команд, которым они номинально «помогают».

Предположим, что продуктовая команда хочет обновить публичные маршруты ingress, чтобы запустить важный проект. Но команде edge-сети этот проект не интересен — он не влияет ни на их задачи, ни на показатели их руководителя. Единственное, что может повлиять на их оценку, — это проблемы в продакшене, вызванные изменениями. А значит, их мотивация — блокировать любые потенциально рискованные изменения как можно дольше. Для команды, которая готова принять определённый риск ради выпуска фичи, это может быть чрезвычайно раздражающим.

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

Многим инженерам — и я не исключение — нравится оставлять блокирующие ревью, по тем же причинам, по которым людям вообще нравится быть «стражами порядка». Кажется, что ты в одиночку оберегаешь качество кодовой базы или предотвращаешь будущий инцидент в продакшене. Это также возможность потешить одно из распространённых инженерных тщеславий — продемонстрировать своё техническое превосходство над менее опытным коллегой. Ах, ты не знал, что твой код вызовет N+1-запрос? А я знал. Как хорошо, что я внимательно всё прочитал, не так ли?

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

Заключительные мысли

Я вполне уверен, что многие инженеры не согласятся с большинством (а может, и со всеми) пунктами в этом посте. И это нормально! У меня также есть много убеждений насчёт код-ревью, которые, на мой взгляд, очевидно верны, но я бы не стал включать их сюда.

Из моего опыта, стоит придерживаться следующих подходов (ниже, в конце статьи, приведен пример чеклиста Code Review по мнению эксперта сообщества Spring АйО Евгения Сулейманова):

• Обращать внимание на то, какой код не был написан в PR, а не только на diff

• Оставлять небольшое количество продуманных комментариев, а не наспех писать построчные замечания, которых в итоге накапливается сотня

• Смотреть на код через призму «будет ли это работать», а не «точно ли это так, как сделал бы я»

• Если вы не хотите, чтобы изменение было замержено — оставьте блокирующее ревью

• Если нет очень серьёзных проблем — аппрувьте изменения.

Всё это в целом применимо и к ревью кода, сгенерированного агентными LLM-системами. Они особенно склонны упускать важный код, который стоило бы написать; им также сложно справляться со ста комментариями сразу; у них есть свой собственный стиль. Единственное исключение — принцип «старайтесь аппрувить изменения». К LLM это не относится. Генерируемые ИИ PR’ы мо��но и нужно блокировать столько, сколько потребуется.

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

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

P.S.: Этот пост получил в основном положительные отзывы на lobste.rs и Hacker News. Нескольким людям не понравился пример с camelCase против snake_case — они считают, что такие вещи должна отслеживать автоматизация. Возможно, так, но принцип сохраняется и для менее очевидных случаев, которые нельзя так просто автоматизировать — например, «всегда логировать с определёнными тегами перед операциями записи». В этой ветке комментариев ведётся интересное обсуждение норм вокруг блокирующих ревью. Наконец, самый популярный комментарий на lobste.rs гласит, что я неправильно интерпретирую гайд Google, перефразируя его как «склоняйтесь к одобрению». На мой взгляд, принцип действительно направлен на то, чтобы убедить умных, придирчивых инженеров чаще одобрять изменения — но, да, это уже моя интерпретация.

Присоединяйтесь к русскоязычному сообществу разработчиков на Spring Boot в телеграм — Spring АйО, чтобы быть в курсе последних новостей из мира разработки на Spring Boot и всего, что с ним связано.