Ошибки, которые инженеры совершают на код-ревью

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

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

Не проверяйте только diff

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

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

Точно так же комментарии типа «этот код, вероятно, должен находиться в этом другом файле» очень полезны для поддержания долгосрочного качества кодовой базы. Главная ценность при работе с большими кодовыми базами — это последовательность (я пишу об этом подробнее в статье Mistakes engineers make in large established codebases). Конечно, вы не можете судить о последовательности только по диффу.

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

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

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

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

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

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

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

1. Смотрят на кусок диффа.
2. Спрашивают себя: «как бы я это написал, если бы я писал этот код?»
3. Оставляют комментарий к каждому различию между тем, как бы они это написали, и фактическим диффом.

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

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

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

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

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

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

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

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

Если куча PR блокируется, это обычно признак того, что происходит слишком много гейткипинга (gatekeeping). Одна динамика, которую я часто наблюдал, — это когда одна команда владеет узким местом для функций многих других команд — например, возможно, они владеют конфигурацией пограничной сети (edge network), где должны быть определены новые публичные маршруты, или структурой базы данных, которую новые функции должны будут изменить. Эта команда, как правило, больше ориентирована на надежность, чем типичная продуктовая команда. Инженеры в этой команде могут иметь другое название, например SRE, или даже принадлежать к другой организации. Таким образом, их стимулы не совпадают с продуктовыми командами, которые они номинально поддерживают.

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

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

Для многих инженеров — включая меня — приятно оставить блокирующее ревью по тем же причинам, по которым приятно заниматься гейткипингом в целом. Кажется, что вы в одиночку защищаете качество кодовой базы или предотвращаете какой-то инцидент в продакшене. Это также способ потакать распространенному пороку среди инженеров: флексить (демонстрировать) свои собственные технические знания перед менее компетентным инженером. О, похоже, ты не знал, что твой код вызвал бы N+1 запрос! Ну, а я знал об этом. Разве тебе не повезло, что я нашел время прочитать твой код?

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

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

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

По моему опыту, хорошая идея:

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

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

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

UPD: Этот пост получил несколько в основном положительных комментариев как на lobste.rs, так и на Hacker News. Некоторым людям не понравился пример «camel case vs snake case», потому что они считали, что это должно отлавливаться инструментами — справедливо, но принцип верен для изменений, которые не так легко отловить инструментами, например, «логировать с определенными тегами перед операциями записи». Эта цепочка комментариев представляет собой интересное обсуждение норм вокруг оставления блокирующих ревью. Наконец, топовый комментарий на lobste.rs считает, что я искажаю рекомендации Google, перефразируя их как «склонность к одобрению». Мне кажется совершенно ясным, что принцип Google направлен на то, чтобы убедить умных, придирчивых инженеров, что они должны одобрять больше изменений — но это определенно моя интерпретация.