Чеклист для Code Review

У нас есть принципы проведения code review. Вот прямо в виде отдельного документа. Зачем так заморачиваться?

На первый взгляд с code review всё понятно. Вот код, вот его автор, вот ревьюер — он комментирует. Автор смотрит комментарии, делает правки. Код стал лучше; можно мёрджить. Вроде всё просто?

На деле всё интереснее. Культура обратной связи, процесс формирования и обновления принципов code style в компании, обучение, knowledge sharing. Всё это влияет на процесс. Code review может быть инструментом для обеспечения нужного качества кода, а может превратиться в кошмар — повод для бесконечных склок, бюрократию, недоразумение.

Мы хотим пользоваться потенциалом code review по части пользы и избегать побочных эффектов. Для этого мы решили начать с простого шага — договориться. Сделать так, чтобы все участники процесса понимали суть происходящего плюс-минус одинаково.

Список принципов получился вот такой.

Базовые договорённости

+ Нашей целью не является создание идеального кода. Настоящая цель — обеспечить приемлемое качество.

+ Через процедуру ревью должны проходить все изменения в коде. Апрув от квалифицированного ревьюера — это обязательное условие перед тем, как делать merge в мастер.

+ Этот процесс нужен, чтобы улучшить качество кода, который мы создаём; управлять распространением знаний о проекте; продвигать общие подходы к дизайну и паттернам.
+ Ещё, ревью — это инструмент обучения: и ревьюер, и ревьюируемый могут узнать что-то новое.

Правила для всех

+ Мы отдаём себе отчёт в том, что многие технические решения являются частным мнением. Обсуждаем варианты и стремимся достигать компромисса быстро.

НЕТ

Твой вариант плохой, сделай по-другому.

ДА

Интересное решение, у меня есть ещё одно. Как тебе такой вариант: … ?

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

НЕТ

Я совсем не понял, зачем ты сделал это преобразование.

ДА

Мы договорились, что этот код нужен для того, чтобы …

+ Задаём вопросы, а не требуем.

НЕТ

Тебе надо назвать эту переменную x.

ДА

Как смотришь на то, чтобы назвать эту переменную x?

+ В любой непонятной ситуации сначала просим прояснить, прежде чем двигаться дальше.

НЕТ

Тут непонятно. Переделай.

ДА

Тут мне не совсем понятно. Можешь пояснить?

+ Помним, что обсуждаем код. Не используем «человеческие» характеристики в комментариях, особенно оскорбительные.

НЕТ

Что за тупой код?!

ДА

Этот код не решает исходную задачу. Вот почему: …

+ Считаем, что каждый участник процесса умный и не стремится намеренно сделать плохо.

НЕТ

Ты нарочно что ли делаешь ту же ошибку уже в пятый раз?

ДА

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

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

НЕТ

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

ДА

Выполнены не все требования к задаче.

+ Если попадаем в ситуацию, когда сильно не согласны с собеседником, не стесняемся взять таймаут. Сначала остыть, подумать и только потом отвечать.

НЕТ

Так никто не делает! Это же очевидно! Начинаю сомневаться в твоей квалификации!

ДА

Я не согласен. Вот почему: …

+ Стремимся быть скромными.

НЕТ

На этот вопрос тебе никто в компании не ответит! По крайней мере, лучше меня.

ДА

Тут я не уверен, давай разбираться.

+ Не используем обобщения.

НЕТ

Ты всегда делаешь эту ошибку.

ДА

Смотри, вот это место противоречит нашему код-гайду.

+ Не используем сарказм.

НЕТ

Если ты хотел внепланово отпраздновать Хэллоуин и напугать меня кодом, у тебя получилось.

ДА

К этому месту в коде у меня есть вопросы: …

Правила для тех, кто проверяет код

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

Затем:

+ Рассказываем, в каких местах мы уверены, а в каких нет.

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

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

+ Предлагаем альтернативные варианты, но считаем, что автор коммита уже рассматривал их.

+ Стараемся понять, как рассуждал автор коммита.

+ Не забываем хвалить удачные места в коде.

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

+ Ещё раз, стараемся быть позитивными!

Правила для тех, чей код проверяют

+ Благодарим ревьюера за его идеи.

+ Не принимаем комментарии на свой счёт. Ревьюят наш код, а не нас лично.

+ Объясняем, почему выбрали то или иное решение.

+ Стараемся понять точку зрения ревьюера.

+ Отвечаем на каждый комментарий.

+ Если есть возможность, обсуждаем комментарии ревьюера лично. Это может сильно ускорить процесс.

+ После того, как ревью завершилось, а мы уверены в коде и его влиянии на проект — можем делать merge.

Этот список принципов — чеклист по форме, но не по сути. С ужасом представляем себе человека, который методично идёт по всем пунктам: «Проследить за тональностью комментариев — сделано; постараться понять ход мысли автора — сделано; быть позитивным — сделано».

Главная польза, которую могут дать эти принципы — передать дух, задать правильный настрой на процесс. А ещё зафиксировать именно наш опыт: что работает лучше, а что хуже — знаем, потому что пробовали.

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

Авторы:

команда CreatikSoft

Активно помогали:

Евгений Степченко,

Project Manager

Сергей Никитин,

Frontend Engineer