Заголовок провокационный, да? )
И так – чтобы не было недопонимания – что я понимаю под код-ревью в этой статье: процесс проверки кода одного разработчика другим разработчиком (или несколькими рзработчиками) на стадии перед выполнением мержа фиче-ветки в основную ветку.
Обычно, задачи перед код-ревью ставятся следующие:
1) контроль качества кода,
2) ознакомление других разработчиков с изменениями в коде.
Теперь, давайте, разберёмся, почему код-ревью не выполняет эти задачи.
Почему не работает “код-ревью как контроль качества кода”?
Ваш коллега получил задачу, на выполнение которой у него уйдёт, условно, 8 чистых рабочих часов. Мы же все всё правильно делаем (на самом деле, нет): декомпозируем и т.д. и на выполнение задачи у разработчика уходит не больше 8 часов. Под “чистыми рабочими часами” я понимаю время, потраченное исключительо на эту задачу:
- ознакомиться с задачей и относящейся к ней документацией по функциональному и системному анализу,
- созвониться с аналитиком и обсудить непонятные вопросы,
- созвониться с фронтендером и обсудть непонятное с ним,
- изучить техническую документацию на язык, библиотеку, фреймворк – если нужно реализовать какой-то нетривиальный функционал,
- определиться, какой алгоритм позволит выполнить бизнес-функцию наиболее оптимально,
- определиться с тем, как новый код нужно встроить в архитектуру приложения, чтобы в результате не получилась “пятая нога, которая будет бежать в противоположную сторону”,
- запрограммировать решение,
- определиться как и что тестировать и написать на решение тесты,
- оформить пулл-реквест.
Ещё раз обращу внимание, что на вот такое погружение в задачу и её выполнение у разработчика уходит чистыми 8 рабочих часов. Это условная цифра. Она может быть как значительно меньше, так и значительно больше. Это сейчас не особо интересно. Интересно, что происходит дальше.
А дальше происходит вот что. После оформления пулл-реквеста первым разрабтчиком, второй разработчик в течение 5 -15 минут:
- откладывает всторону свою 8-часовую задачу, которую он в это самое время делал,
- выгружает из своей оперативной памяти всё необходимое ему для выполнения своей задачи,
- открывает в браузере страничку с пулл-реквестом,
- так как страничка с пулл-реквестом содержит только ту часть кода, которая была изменена, второму разработчику нужно будет либо подгрузить себе в память остальной код приложения, либо всегда держать его в памяти дял проведения код-ревью,
- мысленно выполняет всё то же самое, что первый разработчик делал 8 часов, кроме написаия кода и тестов: погружается в проблему и находит решение,
- сверяет свою мысленную модель решения с решением первого разработчика,
- если модель и решение совпадают – согласовывает решение,
- если модель и решение не совпадают – проводит сравнительный анализ (опять же, мысленно), чтобы установить: сильно ли существующее решение хуже его мысленной модели,
- если решение субъективно кажется ему хуже – пишет по каждому пункту подробное объяснение на тему: как и почему нужно переделать существующее решение. Почему нужно подробное объяснение? Чтобы первый разработчик не подумал, что к его коду придираются необоснованно.
Если первый разработчик не согласен с результатами ревью, процесс повторяется рекурсивно до тех пор, пока стороны не придут к согласию.
Если первый разработчик согласен с решением и исправляет его процесс код-ревью может повториться от 1 и более раз. Вдруг, первый разработчик снова неправильно что-то сделает?
Вы уже, наверное, начали догадываться, что здесь может пойти не так?
1. 8 часов всегда планируется на задачу и не включают в себя время на код-ревью и исправление кода после него.
В результате, второй разработчик (ну, или сколько их там будет проводить код-ревью) тратит время своей задачи на выполнение задачи первого разработчика. В особо запущенных случаях на код ревью тратится время вообще другого проекта (читайте – бюджет другого проекта, чужие деньги).
Не верите? Попробуйте списать своё время на ту задачу, которую вы проверяете – увидите как вся “бухгалтерия” спринта пойдёт в разнос. )
Если первый разработчик потратит на задачу не 8 часов, а 4, то он всё равно будет рассчитывать на то, что у него остались в запасе ещё 4 часа и сильно удивится, когда увидит, что за 1 сеанс гипноза код-ревью времени на исправление осталось сильно меньше 4 часов. Удивление будет возрастать с каждым новым сеансом ревью.
2. Первый разработчик тратит на задачу условные 8 часов.
Почему-то предполагается, что второй (и последующие разработчики) сделают то же самое в уме за 5-15 минут.
За 15 минут оно, конечно, получится, но будет как в той сказке про 12 шапок из 1 шкурки: сделать-то сделали, но только зачем.
Погрузиться в контекст задачи на том же уровне, на котором это сделал первый разработчик, врядли получится. Как следствие этого, врядли получится полностью проверить правильность реализации задачи и тестировния решения.
Что вообще можно полноценно проверить в чужом коде за 15 минут?
Можем проверить, что в пайплайне:
- не упали тесты,
- не рапортует об ошибках статический анализатор кода,
- не рапортует о недостаточности тестов анализатор тестового покрытия,
- соблюдение общих требований к архитектуре прложения,
- солюдение соглашений об именах классов и переменных ))
Безусловно, можем проверить и сам код, если задача была уровня “на пол часа” (добавить поле в модельку, добавить колонку в базу данных или слегка поправить код) или разработчик в явном виде написал “код с душком” (ошибки, не связанные с бизнес-логикой, относящиеся к семантике языка, принятым солашениям о разработке или оптимальности алгоритма).
Но для проведения таких проверок нам точно нужно привлекать одного или больше других разработчиков, занятых своими задачами или вообще другим проектом? Может быть “наука ракетостроения” шагнула далеко вперёд и такие проверки мы можем проводить автоматически, без использования “логарифмической линейки”?
Почему не работает “код-ревью как процесс ознакомления других разработчиков с изменениями в коде”?
Как предполагается это должно работать:
Разработчик по вышеуказанному алгоритму оформил пулл-реквест и пригласил других разработчиков с ним ознакомиться. Ознакомиться почему-то должны не все члены команды проекта, а только один или несколько разрабтчиков. Мне представляется, что это должны быть:
- либо совсем неопытные разработчики (или не разработчики), которые не могут самостоятельно находить изменения в коде и информацию о том, кто когда и зачем эти изменения сделал,
- либо наоборот, самые опытные и сильные разработчики, которые являются хранителями “золотого стандарта” кода приложения у себя в памяти и способны его оттуда выписать на бумажке в случае “отключения электричества”.
Приглашённые разработчики:
- вытесняют из своей оперативной памяти все рабочие задачи,
- подгружают себе в оперативную память код всего приложения. Ну, или той его части, которая меняется пулл-реквестом,
- читают в веб-браузере изменения кода, предложенные пулл-реквестом,
- мысленно выполняют мёрж предложенных изменений с расположенным в их оперативной памяти кодом,
- переносят обновлённый код приложения из оперативной памяти в долговременную,
- жмут кнопку “согласовать” (странно, почему она не называется “ознакомился”, да?),
- возвращаются к выполнению своих собственных задач.
Для самых маленьких объясню без иронии, почему код-ревью – это не про “ознакомление команды с изменниями кода”:
1) с кодом знакомится не вся команда, а только один или несколько избранных человек,
2) в таком ознакомлении нет смысла, т.к. всё то же самое в любой отдельно взятый момент времени любой человек, имеющий доступ к репозиторию кода, может прочитать в самом коде, а не в пулл-реквесте. А если не прочитает – так, может, оно ему на другом проекте не сильно-то и надо?
Что такое код-ревью на самом деле и когда он работает:
На самом деле, код-ревью – это менторство – процесс обучения новых или неопытных разработчиков тому, как правильно у вас работу работать.
К новичку прикрепляеся ментор, который погружает его в процессы, в код, в требования к разработке, и через код-ревью даёт видимую всем обратную связь (показывает как нельзя, как надо и почему так надо, а по-другому нельзя). Для ментора код-ревью и связанный с ним процесс обучения новичка являются отдельным самостоятельным родом занятий, хотя бы на время обучения новичка. На менторство должно выделяться отдельное время, а не вороваться у других задач и проектов. Менторство дожно быть запланированным, а не спонтанным: проводить код-ревью как часть обучения надо в строго определённое время, а не в момент получения извещения о новом пулл-реквесте.
Почему так? Чтобы это работало, а не получалось 12 шапок из 1 шкурки.
Сюда же, к менторству я отношу и психологический аспект проведения код-ревью. Это когда разработчик вроде бы уже поднабрался опыта, но ещё не уверен в своих силах и ему для психологическго комфорта, нужна “поддержка” – видимость защищённости от ответственности.
Похоже на то как развиваются дети: материнсское молоко им уже не нужно (выросли зубки и могут сами есть мясо с кровью), но соска ещё нужна, чтобы успокоиться. Вот, код-ревью может какое-то время выполнять роль такой соски – делать видимость перекладывания ответственности за качество кода с подросшего разработчика на команду или ментора.
Что тут важно понимать:
Если к концу испытательного срока или после перехода джуна в мидлы, разработчику на одном и том же проекте всё ещё требуется ментор, то вы явно делаете что-то не то и очень скоро почувствуете это не только на уровне “шестого чувства”, но и в самой что ни на есть объективной реальности:
1) затянувшееся "менторство" может означать одно из двух:
а) есть разработчик, который хочет, чтобы его менторили. Сам он не желает или не умеет думать. Ему легко и комфортно работать по алгоритму:
- если есть где в коде посмотреть аналогичное готовое решение, я его скопирую и доработаю под свою задачу,
- если такого решения нет, то “это вот я, а это мама моя. Я буду служить вместе с нею. Я один ничего не умею” – делаю как-нибудь и иду на код-ревью. Правильное решение мне напишут вместе с замечаниями. Есля я не пойму замечаний, то позвоню синьору и он с демонстрацией экрана всё покажет и расскажет. Если и после этого не пойму, то синьор скинет мне образец кода или сделает сам, если сроки будут подходить к концу.
Как работать с такими “вечными джунами”, уже давно перешедшими на более высокие позиции, – это отдельная история, для самостоятельной статьи.
б) есть разработчик (или несколько), который не доверяет команде, хочет контролировать всё и всех.
Очень скоро этот разработчик становится “бутылочным горлышком” в потоке разработки. И от этого страдают все, включая может быть и самого “вечного ментора”.
2) затянувшаяся “психологическая помощь” означает прокрастинацию команды – трату времени на распределение ответственности. Код-ревью в этом случае заканчивается в момент, когда все участники процесса прийдут к общему мнению о том, что ответственность за качество предлагаемых изменений достаточно равномерно распределена между всеми участниками процесса.
Как правило, затянувшиеся “менторство” и “психологическая помощь” идут рядышком.
Приведу примеры (из жизни) того, как не работает код-ревью:
1. Команда на одном из проектов проводила очень дотошные код-ревью. Разбирали, прям, всё-всё до мелочей. Каждый кусок кода друг друга подолгу обсуждали и помногу исправляли.
Чтобы оправдать эту активность, команда отказалась от любого автоматического статического анализа кода.
Что в итоге?
Через год работы команда имеет код с кучей "костылей", "граблей" и "пасхалок".
Почему так получилось?
Потому что код-ревью не должен работать как затянувшаяся “психологическая помощь”.
Эта ативность помогла команде не сломать проект через месяц после начала работы над ним. Но это только оттянуло агонию и позволилио перейти "точку невозврата" – когда проект в его нынешнем виде и тащить уже нет сил и бросить нельзя.
У меня челюсть отвалилась, когда я прочитал отчёты detekt, qodana и sonarqube по коду этого проекта.
2. На другом проекте старший разработчик взял на себя роль “вечного ментора”, который один знает как правильно, всем всё покажет и расскажет. Через год после начала разработки проекта с нуля, он уволился. На проекте остались два разработчика, которые могут сделать “как уже до этого делали, чтоб работало”, не видят причин проблем с производительностью приложения и не хотят ничего менять.
Что интересно, обе команды, не сговариваясь, применяют к своему собственному коду принцип “работает, не трогай”, как-будто этот код они видят впервые и сами не понимают, что и как он делает.
Целью этой статьи было показать, что код-ревью – это не самостоятельный вид активности, обепечивающий распространение знаний в команде или гарантирующий качество кода, а один из инструментов обучения новых или начинающих разработчиков.
Если вы не согласны, пишите, обсудим.
П.С.:
Наткнулся на статью про код-ревью. В одном из комментариев к ней читатель высказал похожие мысли, что и я:
"Для начала следует разобраться, что такое код-ревью в команде, и для чего он проводится. Худший вариант - это проводить код ревью "потому что иначе кнопка merge не активна".
Первое что надо запомнить (и донести до разработчиков) - код ревью НЕ ЯВЛЯЕТСЯ средством обеспечения качества программного продукта. За качество продукта (отсутствие ошибок) отвечает сам разработчик кода и принятая на продукте СМК (система менеджмента качества). При определенном везении, вам могут указать на ошибку в коде на ревью, однако сам факт того что на ревью вышел код с ошибкой - уже является "отклонением" в терминах СМК. Попытка сделать из код-ревью инструмент QA приводит к размытию ответственности, и - у семи нянек дитя без глаза...
Код-ревью может использоваться для обучения (например, задача дается джуну или в общем случае человеку который раньше не имел дела с этой частю кода). В этом случае назначается наставник или бадди, который проводит код-ревью ДО предъявления команде (драфты MR тоже можно ревьювить!).
Код-ревью может использоваться в большой распределенной команде как способ всем хотя бы примерно знать что коллеги творят в других частях системы. Чтобы эти код-ревью были эффективны - надо аккуратно писать название и аннотацию к MR. Потому что весь остальной код человек будет читать по-диагонали.
Код-ревью может использоваться для запроса экспертного мнения. Для этого не стесняемся сами ставить комментарий-вопрос в нужном куске и звать туда через тэг нужного эксперта.
Код-ревью может использоваться для того, чтобы был code-ownership и не было скрытых конфликтов в команде. Тогда натурально "Approve" означает: "Да, я подтверждаю что если на ночном дежурстве меня поднимут по проблеме в этом коде - я смогу с ним разобраться и не буду орать 'какой дурак это накодил!". Собственно, момент код-ревью - это последний момент когда вы можете или выразить свое несогласие - или уже молчать вечно...
Наконец, код-ревью может использоваться ретроспективно, вне связи с изменениями, например для анализа кодовой базы на наличие уязвимостей, тех.долга и так далее. Результатом этого обычно являются таски на рефактор в бэклог.
В любом случае, надо избегать ситуаций когда принципиальные замечания появляются на стадии код-ревью. Обычно, это стадия - где слишком поздно все выбросить и переписать (если это только не учебная задача и на нее не заложены ресурсы с многократным избытком - см выше про задачу обучения джунов). Ключевые элементы реализации функционала должны быть проговорены и согласованы на этапе груминга бэклога или крайний срок - на дейлике. Код-ревью - это этап верификации выполнения договоренностей в команде, а не этап нормирования этих договоренностей!
В сухом итоге - код ревью это не сколько технический, сколько социальный инструмент взаимодействия команды. Если это осознать, и не относиться к код-ревью как к магическому инструменту который разом исправит пороки команды - то он сразу начнет лучше работать..."