Лучшие практики в код ревью. Главные ошибки и как их избежать

Меня зовут Вячеслав Аксёнов, я бэкенд разработчик, с опытом более 5 лет. За свою карьеру я помогал строить сложные распределенные системы в крупнейших российских финтех компаниях, продуктовых европейских компаниях и сейчас выстраиваю процессы разработки в продуктовом стартапе в США.

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

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

Что такое код ревью и пулл реквест

Процесс командной разработки зачастую включает в себя работу несколькими людьми над одним репозитием. В таком процессе необходимо следить за тем, чтобы репозиторий был максимально надежным и имел только качественный код, который был бы хорошо читаемым и содержал бы минимальное количество багов. Для выполнения этих требований существует процесс код ревью – участник команды выставляет часть кода на проверку, которую проводят другие члены команды и либо оставляют комментарии с пометкой “требуется доработка”, либо ставят галочку “одобрено”. Код ревью важен для команд с разработчиками любого уровня, поскольку все члены команды хотя бы верхнеуровнево начинают быть в курсе что именно добавляет в репозиторий другой разработчик.

Чтобы было удобно проводить код ревью, во всех современных системах репозиториев есть механизм “Пулл реквеста” или “merge request” (в gitlab), что по сути одно и то же.

Как интегрировано в жизнь разработчика

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

Какие ошибки могут быть со стороны выставляющего пулл реквест

Создавать пулл реквест, содержащий несколько тикетов сразу

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

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

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

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

Как не допустить такой ошибки: неукоснительно соблюдать правило – в одном пулл реквесте максимум один тикет.

Слишком большой для одного тикета

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

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

А уж если пулл реквест оказался настолько запутанным, что никто из членов команды не может разобраться что именно и как там написано, то точно НЕ СТОИТ устраивать созвон на всю команду с демо своего пулл реквеста. Потому что только время свое и всех участников команды потратите, а к облегчению задачи едва ли приблизитесь

Как не допустить такой ошибки:

• Во-первых, оценить какие именно изменения были проведены в пулл реквесте. Где добавились новые модели, где добавились новые клиенты или интегарации, где рефакторинг зачесался.
• Во-вторых, по возможности разделить огромный пулл реквест на пулл реквесты поменьше. Главное чтобы каждый был емким и завершенным. Добавили модели – отлично. Добавили внешнюю интеграцию + тесты – супер.

Главная идея – создавать завершенные пулл реквесты, которые будет возможно проверять в один присест. Такие ревью будут проходить гораздо проще + будут приближать вас к завершению задачи.

Добавлять закомментированный код

Иногда кажется, что “да ладно, закомментирую эту функцию вместо того чтобы удалить”. Но это является ошибкой поскольку комментарии в коде катастрофически уменьшают читаемость кода. И каждый пулл реквест должен по возможности следовать правилу “сделай репозиторий лучше, чем он был до”. А добавление комментариев в пулл реквесте только множит хаос в репозитории. Если вы боитесь удалить что-то неиспользуемое, то держите в голове, что git – это машина времени для кода, в любой момент можно поднять историю и посмотреть как изменялся файл и что из него удалялось, а что добавлялось.

А если вы добавляете НОВЫЙ закомментированный код, то вероятно вы опасаетесь что он работает некорректно. Этого нельзя делать категорически. Закоментированный нестабильный код – это мусор.

t Как не допустить такой ошибки: не добавляйте закомментированный код в пулл реквест. Никакой. Вообще. Единственным исключением может быть описание какого-то блока бизнес логики, если у вас такая практика закреплена в команде. Либо комментарий с указанием “todo + номер задачи с описанием что должно быть здесь доработано”.

Если же есть что-то неочевидное в коде, то указать на это комментарием в самом пулл реквесте.

Запрашивать ревью пулл реквеста, который имеет сломанные тесты

Как правило, все команды имеют настроенные CI скрипты (Continious Integration пайплайн), которые выполняют запуск тестов на вашей ветке чтобы убедится что код внутри правильно работает. Бывает такое, что во время реализации новой фичи вы либо написали тесты, которые не работают, либо каким-то образом сломали старые тесты. И этот самый CI пайплайн падает с ошибкой в одном из ваших тестов.

Кажется, что в такой ситуации можно написать примечания к пулл реквесту мол “не обращайте внимание, потом поправлю”. Но это ошибка – поскольку пулл реквест должен быть “завершенным” и ни в коем случае не сломать основную ветку проекта. А запрос ревью на пулл реквест, который заведомо “сломает” ветку, крайне снижает уровень доверия к такому пулл реквесту и скептицизм взлетает до совсем высокого уровня. Такой пулл реквест можно сравнить с отвлечением команды от работы. Поскольку текущее ревью явно будет не последним и тем самым обесценит время, затраченное на него.

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

Запрашивать код ревью на код, не покрытый тестами

Когда разрабатываешь новую бизнес фичу может показаться, что нет смысла тратить время и писать тесты. Ведь “там же все и так очевидно” или потому что “лень”. Но уверяю вас, добавление бизнес логики без тестов в репозиторию подобно бомбе замедленного действия. Будь вы хоть сколько опытным разработчикм, никогда не допускающим ошибок, наличие тестов позволит найти ошибку в будущем, если кто-то другой сломает ваш код. Пулл реквест, где код не покрывается тестами является незаврешенным и даже более опасным, чем иметь неработающие тесты. Поскольку в таком случае CI пайплайн будет “зеленым” и не подсветит ошибок.

Как не допустить такой ошибки: всегда проверять, что ваш пулл реквест содержит тесты на тот код, который вы написали или исправили. Не допускать отсутствия тестов.

Иметь в одном пулл реквесте код от нескольких разработчиков

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

Как не допустить такой ошибки: либо разделять свой пулл реквест и просить коллегу создать свой пулл реквест отдельно, либо перенимать его код и применять к своей ветке как есть, либо с исправлениями. Но не допускать смежного авторства в одном пулл реквесте.

Какие ошибки могут быть со стороны принимающего пулл реквест

Придирки к мелочам и требовать переделать с таким же подходом, но “по-другому”

Бывает такое, что открываешь пулл реквест на проверку и видишь, что решение отличается от того, которое представляется в голове. И в такой ситуации иногда можно воспользоваться правом “проверяющего” и поставить “требуются изменения” с указанием своего видения решения.

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

Если разработчик – новичок или не так опытен, то имеет смысл более внимательно относиться к его реквестам. Но даже в этой ситуации код ревью не должно быть процессом, где код причесывается под видение одного человека, даже если он “самый главный” в команде. Поскольку репозиторий – сущность, с которой работает команда, а не единственный человек.

Быть токсичным

День оказался неприятным, кто-то нагрубил, погода плохая, а тут еще пулл реквест этот, а в нем еще и ошибки какие-то. Бывает, что очень хочется написать “ну и чушь ты здесь понаделал”. И если взять во внимание, что вы – одна команда, которая работает над общим проектом, то становится очевидно, что токсичность даже одного члена команды может ухудшить атмосферу уважения во всем коллективе.

Токсичность – это непрофессионализм.

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

Признаком профессионала будет оставление на каждую ошибку емкого комментария с описанием почему так делать не надо и как делать надо. Можно оставить ссылку на статью по теме с комментарием. Любое требование изменения должно быть аргументированным. И быть написано максимально спокойным или дружеским тоном.

Пытаться найти максимальное количество ошибок

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

Однако код ревью не является соревнованием по поиску недоработок. Любой код не идеален по своей сути. И требование код ревью – не допустить опасного или вредного кода в репозиторий. Но не искать где можно придраться к код стайлу или неверному отступу.

Следует указывать только на критические функциональные ошибки, либо нарушение договоренностей внутри команды.

Откладывать пулл реквест в долгий ящик

Вот это денек выдался – столько задач, столько созвонов, а еще эти код ревью поназапрашивали. Ничего не случится если завтра проверю, ну или послезавтра.

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

Как правило внутри команды заводится четкая практика – например, код ревью проводится строго с утра первым делом. Такая практика делает срок получения комментариев по пулл реквесту предсказуемым.

Выводы

Хороший пулл реквест – это какой?

1. Имеет понятное название + содержит ссылку или номер задачи.
2. Имеет тесты на все изменения.
3. Имеет пройденные проверки CI пайплайна.
4. Достаточно небольшой чтобы его можно было проверить в один заход.
5. Имеет только одного автора.

Хорошее ревью – это какое?

1. Своевременное.
2. Без токсичности, вежливое и хорошо читаемое.
3. Без излишней придирчивости к каждой строчке. Подсвечивает явные ошибки или потенциальные проблемы.
4. Не содержит требований “вам кажется, что можно написать по другому”. Если есть предложения – выдвигать их, но не требовать чтобы разработчик их выполнил.

Короткое дополнение для джунов

1. Если вас строго проверяют – это может быть как синдром вахтера, так и искреннее желание помочь вам развиться. Внимательно относитесь ко всем комментариям.
2. Пулл реквесты и код ревью это не страшно. У всех бывают тяжелые код ревью и легкие. Это необходимый процесс чтобы код в репозитории был чистым и работал.