Улучшаем процесс ревью в команде

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

При этом разработчик (надеемся):

1. В курсе бизнес процесса

2. Понимает, как его код встроен в общую архитектуру решения

3. Уверен, что предлагаемое решение - работает

Ревьюер (в худшем случае):

1. Не знает бизнес процесса

2. Не в курсе архитектуры решения

3. Не в курсе, работает ли решение и работает ли так, как должно

Тимлид при этом хочет:

1. Чтобы ревью проходило быстро

2. Чтобы ревью было качественным

3. Чтобы разработчик и ревьюер остались в нормальных отношениях.

Что же можно сделать в каждой из трех ролей, чтобы все остались довольны (и целы)?

Разработчик

Расшарить контекст

Разработчик (в теории) знает про задачу все. Потому, если он хочет быстро завершить ревью по задаче, стоит поделиться своим знанием с миром. Именно с миром, а не с конкретным ревьюером:

• ревьюеров может быть много

• знание может пригодиться и другим - QA, PM, тимлиду и др

Для этого лучший вариант - оставить комментарий в трекере:

1. Что конкретно сделал

2. Как и что проверил

3. Возможные нюансы, которые могут быть полезны ревьюеру (Ссылки на документацию, нюансы решения)

Всю эту информацию ревьюеру все равно придется собирать (для качественного ревью), и он (они) либо пойдут к разработчику, либо будут искать сами. И то, и другое - дополнительные затраты времени 1+ участников команды, которых можно избежать.

Self review

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

Что стоит проверить:

• решение должно выполнять то, что нужно - приложение собирается, поднимается, тесты выполняются (да, и такое бывает). Если у вас это автоматизировано - прекрасно. Если нет - стоит автоматизировать.

• решение может быть смержено (нет конфликтов)

• в решении нет ничего лишнего (может решаться различными линтерами/чекерами/сонаром, но может и не решаться)

Отписываться по комментариям, но не закрывать обсуждение

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

• согласен и фикс будет в рамках мерж реквеста

• согласен, но фикс будет в рамках отдельной технической задачи/ветки

• не согласен и почему

Важно при этом не закрывать обсуждение по замечанию самостоятельно - закрыть должен тот, кто открыл, когда будет достигнут и зафиксирован консенсус. Потому что ревьюер в первую очередь пойдет проверять свои замечания. Если они закрыты - их придется открывать и (возможно) инициировать снова. Не говоря уже о том, что часть из них может быть незаслуженно пропущена/забыта, что создает риск пропустить проблему и получить дефект от QA (в лучшем случае).

Ревьюер

Погрузиться в контекст

Ревьюер может ничего толком и не знать по задаче. Для качественного ревью ему нужно погрузиться в контекст:

1. Что за бизнес процесс и как меняется

2. Какие есть технические ограничения на проекте

3. Какие есть правила в команде по организации архитектуры системы и кода

В идеале контекст уже должен был быть описан разработчиком. Если нет - контекст стоит запросить у самого разработчика либо постановщика задачи.

Проводить ревью поэтапно

Будучи в контексте, можно начинать само ревью. В его рамках нужно:

1. Проверить, что задача делает то, что нужно

2. Визуально проанализировать корректность корнер-кейсов (исключения, ошибки межсервисных взаимодействий, ttl операций и пр)

3. Проверить решение на соответствие правилам команды по вопросам архитектуры

4. Проверить решение на соответствие правилам команды по организации кодовой базы

5. Проверить технические детали реализации (сложность алгоритмов, жадность и пр.)

6. Оценить стилистику написания

Важно производить ревью поэтапно и именно в такой последовательности - от общего (важного) к частному (неважному).

Еще важный момент - обнаружив проблемы на одном уровне, стоит остановиться и вернуть на доработку. Причина проста: изменения более общего (важного) практически гарантированно приводят к изменениям более частного (неважного). Ревьюер потратит время на комменты участков кода, которые потенциально изменятся, а разработчик потратит время на анализ того, что ему нужно править, а что уже неактуально.

Отделять важное от неважного

В рамках ревью можно обнаружить проблемы разной степени важности:

• Критичные - проблемы, с которыми решение не может быть принято.

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

• Не важные - вопросы обычно стилистического характера

При указании проблемы крайне полезно указать ее важность. Например, отмечать технические и не важные проблемы пометками вроде "TECH" и "NIT". В зависимости от срочности задачи и объема проблемы технические проблемы можно либо делать в рамках текущей задачи, либо выносить в технические задачи на доработку (рефакторинг). Так образуется прозрачный технический бэклог, объем которого известен всем участникам команды.

Тимлид

В рамках конкретного мерж реквеста тимлид вполне может быть и не нужен. Его задача - выстроить процесс и отслеживать его работу.

Напомню ожидания тимлида от ревью:

1. Чтобы ревью проходило быстро

2. Чтобы ревью было качественным

3. Чтобы разработчик и ревьюер остались в нормальных отношениях.

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

Чтобы ревью проходило быстро

Чтобы определить, как сделать ревью быстрым, нужно выяснить, почему оно может быть медленным:

• Долго погружаться в задачу

• Коллеги не могут договориться по техническим нюансам

• Ревьюер тратит время на проверку вещей, которые можно было проверить автоматически

Долго погружаться в задачу можно по разным причинам:

• Разработчик мог не описать контекст - нужно приучить всех делать это

• Разработчик мог плохо описать контекст - нужно выработать гайд/чеклист, что должно быть в описании контекста и использовать его в рабочем процессе

• Ревьюер далек от предметной области - стоит пересмотреть процесс выбора/назначения ревьюера

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

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

• проверка сборки проекта

• прохождение юнит тестов

• проверка линтерами, статическими анализаторами и т.п.

Чтобы ревью было качественным

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

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

Если в команде нет людей, способных качественно проверить обе составляющие решения (бизнес и технику) либо их мало, стоит рассматривать ревью в несколько этапов и параллельно растить экспертизу у членов команды (чтобы не замыкать ревью на 1-2 людях).

Чтобы разработчик и ревьюер не разругались

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

1. Все замечания должны быть направлены на код, а не на его автора. Не "ты наговнокодил", а "код стилистически (ассимптотически) неоптимален"

2. Критикуешь - предлагай. Если есть идеи, как решить замечание - стоит их озвучить. ("Произойдет выполнение запроса, хотя возможны кейсы, когда это не нужно. Optional.orElseGet в помощь")

3. Есть, за что похвалить - хвали. Мы все привыкли отмечать только недостатки, хотя успехи не менее (а порой и более) важны

4. Следствие из п.1. Если есть вопросы к человеку - нужно обратиться напрямую к человеку в личку. Стандартная история в духе "хвалить - на людях, ругать - на 1-1"

Заключение

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

• Перед передачей на ревью - поставьте себя на место ревьюера.

• Автоматизируйте все, что можно автоматизировать (если профит покроет затраты на автоматизацию).

• Стройте процессы, оптимизируйте их и следуйте им.

Уважайте коллег, их труд и их время, как свое собственное :-)