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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Затем:

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

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

  • + Благодарим ревьюера за его идеи.
  • + Не принимаем комментарии на свой счёт. Ревьюят наш код, а не нас лично.
  • + Объясняем, почему выбрали то или иное решение.
  • + Стараемся понять точку зрения ревьюера.
  • + Отвечаем на каждый комментарий.
  • + Если есть возможность, обсуждаем комментарии ревьюера лично. Это может сильно ускорить процесс.
  • + После того, как ревью завершилось, а мы уверены в коде и его влиянии на проект — можем делать merge.

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

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

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

Ixtens


Авторы:
команда Ixtens

 

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

Серёга Никитин

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

 
Серёга Никитин
Сергей Никитин,
Frontend Engineer