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

• порядке проведения ревью,
• скорости (и на что она влияет),
• как правильно писать комментарии,
• дискуссии в ходе ревью.

1. Навигация по списку изменений в ревью

TL;DR

Теперь, когда понятно на что нужно обращать внимание, необходимо определиться, каков порядок проведения ревью. Можно выделить 3 основных этапа:

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

1.1. Обзор изменений

Посмотрите на описание CL и его основной функционал: нужны ли эти изменения? Если предлагаемый функционал не нужен, постарайтесь сообщить об этом как можно скорее и объясните причины. При этом, желательно, предложить разработчику задачу взамен.

Ваше сообщение может выглядеть следующим образом: "Спасибо за хороший код, однако, сейчас мы идем к тому, чтобы полностью избавиться от системы FooWidget, которая здесь затрагивается. Поэтому в данный момент мы никаким образом не модифицируем ее. Было бы здорово, если бы ты помог с рефакторингом BarWidget класса. Что думаешь?"

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

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

1.2. Проверка самого важного

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

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

Почему важно сообщать о больших проблемах рано:

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

1.3. Проверка всего остального

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

2. Скорость проведения ревью

2.1. Почему код-ревью должно быть сделано быстро?

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

Что происходит, когда код-ревью делается медленно:

• Продуктивность команды снижается. Конечно, разработчики, не получившие ревью вовремя, просто начинают делать другую работу. Вместе с тем, релизы багфиксов и нового функционала задерживаются на дни, недели и месяцы, по мере того, как изменения ждут ревью и повторного ревью.
• Разработчики недовольны процессом ревью. Если ревьюер отвечает раз в несколько дней и каждый раз запрашивает мажорные изменения, разработчикам становится сложно работать. Часто это выражается в жалобах на строгость ревьюера. Однако, если ревьюер стабильно запрашивает одинаковые значительные изменения, которые улучшают кодовую базу, и при этом реагирует на обновления CL быстро, жалобы уходят. Большинство жалоб на процесс проведения ревью исчезает, когда оно становится быстрым.
• Негативное влияние на состояние кодовой базы. Когда ревью медленное, давление на команду растет и часто пропускается код ненадлежащего качества. Медленные ревью также затрудняют рефакторинг, удаление лишнего кода и другие улучшения в CL.

2.2. Насколько быстрыми должны быть ревью?

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

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

Если следовать приведенным правилам, то обычный CL пройдет несколько раундов ревью (если потребуется) в течение одного рабочего дня.

2.3. Скорость vs прерывание

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

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

2.4. Отвечайте быстро

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

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

Если вы слишком заняты, чтобы провести ревью полностью, вы можете дать знать разработчику, когда сможете заняться его CL или порекомендовать других ревьюеров, которые смогут ответить быстрее. Также вы можете оставить первые обзорные комментарии на код. Но помните, даже ответы такого рода не должны прерывать вашу работу по написанию кода — лучше ответить во время перерыва.

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

2.5. Разные часовые пояса

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

2.6. LGTM (looks good to me = с моей точки зрения все хорошо) с комментариями

В некоторых ситуациях, для того чтобы ускорить ревью, вы можете ставить аппрув даже если к CL остались комментарии. Это может быть сделано в следующих случаях:

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

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

2.6. Большие CL

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

Если CL не может быть разбит на более мелкие части и у вас недостаточно времени, чтобы провести полное ревью, напишите несколько комментариев по общей архитектуре CL, возможно там есть моменты, которые разработчик сможет доработать уже сейчас. Одна из ваших задач, как ревьюера, давать разработчикам быстрый ответ и не блокировать их дальнейшую работу.

2.7. Прогресс в код-ревью

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

2.8. Экстренные ситуации

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

3. Как писать комментарии в ревью

TL;DR

• Будьте добрыми.
• Объясняйте свои доводы.
• Находите баланс между тем, чтобы давать прямые указания и просто обозначать проблемы, позволяя разработчику решить их самостоятельно.
• Мотивируйте разработчиков писать простой код и добавлять комментарии, вместо объяснения сложных участков конкретно вам.

3.1. Вежливость

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

Плохо: "Почему ты решил использовать потоки, при том что многопоточность не дает здесь никакого выигрыша?"

Хорошо: "В данном случае, многопоточность не приносит никакой выгоды, а только усложняет систему. Поскольку, нет никакого выигрыша в производительности, лучше чтобы данный код работал в одном потоке."

3.2. Объясняйте причины

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

3.3. Давайте советы

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

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

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

3.4. Следите за объяснениями

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

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

4. Дискуссии в процессе ревью

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

4.1. Кто прав?

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

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

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

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

4.2. Расстраиваются ли разработчики?

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

4.3. "Сделаю позже"

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

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

4.4. Жалобы на строгость ревью

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

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

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

4.5. Разрешение конфликтов

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