Практика код-ревью: спорные моменты
О код-ревью уже сказано очень много, но все равно остаются моменты, о которых можно поспорить. Именно их мы не так давно обсуждали на одной из внутренних ИТ-посиделок.
Код-ревью сам по себе — не панацея. Его можно построить так, чтобы помочь проекту. Но эта польза не гарантирована — полно примеров, которым код-ревью идет во вред. Поговорим о том, где здесь скрываются подводные грабли и как провести код-ревью лучше.
Код-ревью проводить бессмысленно, когда нет нормальной постановки задачи
Начнем с базовой вещи. Код-ревью — запоздалое решение, если изначально задача была поставлена не полностью, некорректно или без нормальных вводных.
Довольно распространенная ситуация — новый в команде разработчик писал-писал код, а на ревью ему говорят: «Так нельзя, так не делаем» (например, вообще не используем REST). В этот момент в неудобном положении оказываются все. Разработчик писал код, но результаты его работы никому не нужны. А ревьювер, хоть это и понимает, будет настаивать на своем, потому что в каком-то смысле он несет ответственность за проект.
Кто в этой ситуации должен донести до нового человека, что REST не используется, — вопрос философский. Возможно, это ответственность того, кто онбордит или задача тимлида. В некоторых компаниях считают, что это должна быть инициатива самого новичка. Но как правило, ответственность размыта между всеми участниками процесса, поэтому гораздо лучше, если все неочевидные моменты, касающиеся проекта и применяемых подходов, задокументированы.
В любой задаче нужно обсуждать, что требуется, заранее — до того, как написана хотя бы одна строчка кода. Хорошая практика — делать это при планировании.
В реальности не всегда выходит именно так. Бывает, что во время обсуждения предложено решение, с которым все согласились, и разработчик реализовал именно его. Но во время код-ревью ответственному приходит в голову мысль о том, что задачу в принципе нужно было решать иначе. Но это уже другая история — про улучшение кода с прицелом на будущий рефакторинг. Прислушиваться ли к новой идее на этапе ревью, зависит от деталей организации проекта.
Код-ревью должен быть быстрым
Плохая практика — откладывать код-ревью на неделю и более. Когда ревью растягивается, код очень медленно попадает в продакшн, а разработчики устают от постоянного возвращения к задачам, которые они сдали много дней назад. Приходится тратить время на новое погружение в контекст.
По-хорошему код-ревью надо делать сразу, как только появляется пулл-реквест. Задержка в час — нормально, в полдня — уже не очень, потому что все это время разработчик ждет и ему приходится хранить в голове контекст задачи.
Удачная практика — контролировать, посмотрел ли назначенный ревьювер код, и если не посмотрел, кидать ему дополнительные уведомления. Возможно, он просто отвлекся.
В целом неплохо, когда в ревью участвуют те же люди, которые пишут код проекта. Ожидая проверки своего пулл-реквеста, они понимают, насколько важно не задерживать коллег в симметричной ситуации.
Ревьювер должен понимать, что его задача не про субъективный перфекционизм
Читая чужой код, всегда появляются мысли о том, что можно было бы сделать иначе — покрасивее. Но ревьюверу нужно ограничивать свой поток замечаний. Во главе угла — работающий код, соответствующий требованиям проекта, т.е. стремление навести красоту должно иметь рамки разумного.
Нормальная практика — помечать некоторые комментарии, как незначительные (предположим, как рекомендует Google в своей методичке, префиксом NIT, https://google.github.io/eng-practices/review/reviewer/looking-for.html). Такой префикс означает, что мысль стоит того, чтобы донести ее до разработчика, но ревьювер не настаивает на выполнении пожелания, т.е. код можно улучшить, но это замечание не блокирует пулл-реквест. В зависимости от принятой в команде культуры, ревьювер может даже не ждать ответа на это замечание.
Бывают ситуации, когда сложно однозначно отделить вкусовщину от дельных замечаний. Но в подобных ситуациях полезно задать себе более высокоуровневые вопросы — зачем все это происходит.
Например, когда речь о допустимой сложности кода, необходимо задумываться о том, что пишем мы не только для того, чтобы код читала машина, но и для людей, которые впоследствии будут этот код дебажить и развивать. И по сути все зависит от трейдоффа между тем, как быстро нужно выкатывать фичи (т.е. тем, сколько времени мы можем потратить на вылизывание конкретного куска кода) и тем, насколько важно его упростить. Возможно, к конкретному куску кода вообще не надо будет возвращаться? Т.е. вылизывая его, мы потратим больше времени, но результат не изменится.
Нет внимательности — нет нормального код-ревью
Можно придумывать какие угодно способы распределения пулл-реквестов между ревьюверами, но если человек в принципе не хочет в этом участвовать, скорее всего он формально согласует код и даже не будет вникать в то, что там написано. И это несмотря на то, что сам по себе код-ревью включает в себя ответ на вопрос, а решает ли пулл-реквест поставленную задачу?
Заставить читать код внимательно невозможно.
Глобальные вопросы мотивации оставим за рамками этой статьи. Частичному рассеянию внимания иногда способствует распределение ответственности на нескольких ревьюверов. Например, на один и тот же пулл-реквест в качестве ревьюверов назначается тимлид и разработчик. Разработчик думает, что тимлид, который будет смотреть после него, всяко найдет проблемы, если они есть, поэтому сам он может проверить код одним глазком. Такую ситуацию обычно видно по качеству и количеству комментариев от обоих. Аналогично ответственность может отключаться, если код смотрит мейнтейнер проекта, а параллельно кто-то со стороны.
Увы, универсального способа борьбы с рассеянием внимания не существует. Этот вопрос остается на совести тех, кто проводит ревью.
Код-ревью — не только про код, но и про взаимодействия в команде
Код-ревью — путь обсуждений внутри команды, а иногда и обучения нового специалиста.
Плохая практика, когда замечания код-ревью перестают пропускать через себя, а просто исправляют механически, не глядя. Улучшается только обсуждаемый код, а не проект в целом. Да, в коде встречаются моменты, которые нужно однозначно исправлять. Но ведь есть же и вещи, которые следует обсудить, выработав некоторое решение на будущее, и в дальнейшем это должно помочь проекту. А как только ревью превращается в механическое исправление, эта его часть бесследно исчезает.
Когда это происходит? Когда комментарии недружелюбны. Есть товарищи, которые еще и гордятся тем, что проводят код-ревью очень жестко — каждый комментарий от них звучит как будто с наездом: «Какого черта здесь реализовано именно так?». Это плохая практика. Даже если в коде что-то не нравится, лучше выбирать более нейтральные формулировки.
Точно также существуют разработчики, которые плохо реагируют даже на очень мягкие комментарии. Вместо того, чтобы сделать код понятнее в ответ на замечание о сложности, они пишут ревьюверу, что тот сам дурак.
И тот, и другой вариант — не способствуют качеству код-ревью. Они либо приводят к конфликтам, либо к тому, что никакого обсуждения не происходит — одна из сторон просто уходит от дискуссии, чтобы сберечь нервы.
А иногда комментариев слишком много. Один из коллег рассказывал о своем предыдущем опыте, когда в крупной компании на пулл-реквест в рамках ревью набрасывалась толпа из 30 индусов, которые не имели ни малейшего представления о проекте. Количество комментариев никак не коррелировало с качеством кода. И это, конечно, очень показательный, но не единственный пример. А еще комментариев бывает много, когда не выполнен первый пункт, обсуждаемый в этой статье — нет нормальной постановки задачи или стайлгайда.
Совет ревьюверу. Не всегда происходящее в пулл-реквесте понятно с самого начала. И хорошая практика — прочитать весь пулл-реквест до конца, а уже потом забрасывать разработчика вопросами. Для этого в GitLab есть удобная функция — написать комментарий и поставить его в очередь, не отправляя сразу. Как правило, эта возможность заметно сокращает количество комментариев, и не приходится потом отменять свои вопросы, извиняясь за формулировки.
Совет разработчику. Не на все замечания ревьювера стоит реагировать в лоб и нервничать. К примеру, указано, что код слишком сложный, а упростить его не получается. Достаточно вспомнить, какова конечная цель всего процесса. Если ревьювер заботится о том, что код не поймет через полгода тот, кто в него полезет «с нуля», возможно, вопрос снимут комментарии в самом коде (а не в его обсуждениях в пулл-реквесте)?
Кросс-командное код-ревью стоит дороже; надо понимать, что и зачем исправляется
Сам по себе код-ревью стоит дорого. Хорошее ревью действительно помогает держать уровень качества кода, и этим очень ценно. Плохое ревью отнимает время множества людей, при этом проект не получает никаких бонусов — это просто выброшенные деньги компании.
Выше уже был пример с набегом на один фрагмент кода 30 человек, не связанных с проектом. Существуют ситуации, когда кросс-командное код-ревью помогает взглянуть на пулл-реквест свежим взглядом. Но это не тот случай. Если нет понимания, что каждый из разработчиков принесет в проект, нет смысла тратить время 30 человек, которые все равно не погружаются в задачу.
Проблема не только в том, что код смотрит человек, который не понимает, что в нем творится. Предположим, коллега как раз хочет проревьювить качественно — т.е. посмотреть не только чистоту кода, но и логику реализации. В этом случае ему придется тратить очень много времени даже на маленький пулл-реквест, чтобы погрузиться в контекст — прочитать доку, выяснить, что делает модуль в целом, какие туда планировалось внести изменения. И это время оплачивается по ставке разработчика.
Если нет задачи «широковещательно» показать код этим трем десяткам коллег, логичнее как-то делить ответственность. Назначать ревьюверов в соответствии с квалификацией и погружением в проект, добавлять апруверов для подстраховки. Можно даже учитывать при распределении задач на код-ревью размер пулл-реквеста в строках и загруженность каждого конкретного коллеги, а также придумывать любые другие ограничения. Лишь бы команду это устраивало и время тратилось с пользой!
Устоявшиеся практики код-ревью хороши, но не для джунов
Выше мы поговорили про разные аспекты код-ревью, вроде бы предложили работающие практики. Но нет ничего универсального.
Код-ревью новичка — тот момент, где иногда можно вставить вместо комментария свой вариант реализации кода. С более опытными коллегами всегда рекомендуется писать не свой ответ на задачу, а вопрос к коллеге-автору кода. Почему сделано именно так? Почему применен один алгоритм, а не другой? Возможно, действительно найдется достойное объяснение. А с новичками код-ревью в большей степени превращается в обучение с соответствующими временными затратами и расстановкой авторитетов. Важно помнить об этом, приглашая в команду джуна.
Хорошая практика, когда новичок пишет так, как считает нужным, а потом ему показывают, как сделать лучше. Далее он переделывает и только потом его код отправляется на ревью в привычном понимании. Получается, что код смотрят дважды. Но зато новичок быстрее вкатывается в проект. Также можно подключать новичка к ревью кода более опытных коллег. Так он будет учиться устоявшимся практикам и общим принципам на проекте.
Вместо итогов
Практика код-ревью должна зависеть от проекта, точнее от его критичности. Когда объект разработки — банковское приложение, которое считает деньги, на код надо смотреть с большим пристрастием и код-ревью проводить в несколько раундов с участием разных людей. А когда разрабатывается простой лендинг, нет смысла в этом усложнении. В итоге не стоит даже в рамках одной компании на разных проектах применять одни и те же шаблонные методы, не пытаясь адаптировать их к ситуации.
P.S. Мы публикуем наши статьи на нескольких площадках Рунета. Подписывайтесь на нашу страницу в VK или на Telegram-канал, чтобы узнавать обо всех публикациях и других новостях компании Maxilect.P.P.S. По ссылке вы есть неплохой перевод методички Google по код-ревью.