У нас есть принципы проведения code review. Вот прямо в виде отдельного документа. Зачем так заморачиваться?
На первый взгляд с code review всё понятно. Вот код, вот его автор, вот ревьюер — он комментирует. Автор смотрит комментарии, делает правки. Код стал лучше; можно мёрджить. Вроде всё просто?
На деле всё интереснее. Культура обратной связи, процесс формирования и обновления принципов code style в компании, обучение, knowledge sharing. Всё это влияет на процесс. Code review может быть инструментом для обеспечения нужного качества кода, а может превратиться в кошмар — повод для бесконечных склок, бюрократию, недоразумение.
Мы хотим пользоваться потенциалом code review по части пользы и избегать побочных эффектов. Для этого мы решили начать с простого шага — договориться. Сделать так, чтобы все участники процесса понимали суть происходящего плюс-минус одинаково.
Список принципов получился вот такой.
Базовые договорённости
- + Нашей целью не является создание идеального кода. Настоящая цель — обеспечить приемлемое качество.
- + Через процедуру ревью должны проходить все изменения в коде. Апрув от квалифицированного ревьюера — это обязательное условие перед тем, как делать merge в мастер.
- + Этот процесс нужен, чтобы улучшить качество кода, который мы создаём; управлять распространением знаний о проекте; продвигать общие подходы к дизайну и паттернам.
- + Ещё, ревью — это инструмент обучения: и ревьюер, и ревьюируемый могут узнать что-то новое.
Правила для всех
- + Мы отдаём себе отчёт в том, что многие технические решения являются частным мнением. Обсуждаем варианты и стремимся достигать компромисса быстро.
НЕТ
Твой вариант плохой, сделай по-другому.
ДА
Интересное решение, у меня есть ещё одно. Как тебе такой вариант: … ?
- + Считаем, что в любой непонятной ситуации лучше обсудить детали в оффлайне; так быстрее. В таком случае в комментариях потом пишем решение, до которого договорились.
НЕТ
Я совсем не понял, зачем ты сделал это преобразование.
ДА
Мы договорились, что этот код нужен для того, чтобы …
- + Задаём вопросы, а не требуем.
НЕТ
Тебе надо назвать эту переменную x.
ДА
Как смотришь на то, чтобы назвать эту переменную x?
- + В любой непонятной ситуации сначала просим прояснить, прежде чем двигаться дальше.
НЕТ
Тут непонятно. Переделай.
ДА
Тут мне не совсем понятно. Можешь пояснить?
- + Помним, что обсуждаем код. Не используем «человеческие» характеристики в комментариях, особенно оскорбительные.
НЕТ
Что за тупой код?!
ДА
Этот код не решает исходную задачу. Вот почему: …
- + Считаем, что каждый участник процесса умный и не стремится намеренно сделать плохо.
НЕТ
Ты нарочно что ли делаешь ту же ошибку уже в пятый раз?
ДА
В прошлый раз мы обсуждали вот такой момент. Сейчас он снова появился. Давай поймём, как не допустить его в следующий раз?
- + Выражаемся простыми словами. Помним, что собеседник в онлайне не всегда может понять наши намерения по высказыванию.
НЕТ
Создаётся впечатление, что касательно изначальной спецификации были реализованы не все реквестируемые пункты.
ДА
Выполнены не все требования к задаче.
- + Если попадаем в ситуацию, когда сильно не согласны с собеседником, не стесняемся взять таймаут. Сначала остыть, подумать и только потом отвечать.
НЕТ
Так никто не делает! Это же очевидно! Начинаю сомневаться в твоей квалификации!
ДА
Я не согласен. Вот почему: …
- + Стремимся быть скромными.
НЕТ
На этот вопрос тебе никто в компании не ответит! По крайней мере, лучше меня.
ДА
Тут я не уверен, давай разбираться.
- + Не используем обобщения.
НЕТ
Ты всегда делаешь эту ошибку.
ДА
Смотри, вот это место противоречит нашему код-гайду.
- + Не используем сарказм.
НЕТ
Если ты хотел внепланово отпраздновать Хэллоуин и напугать меня кодом, у тебя получилось.
ДА
К этому месту в коде у меня есть вопросы: …
Правила для тех, кто проверяет код
Первым делом разбираемся, зачем именно нужно рассматриваемое изменение в коде: исправить баг, улучшить пользовательский опыт, рефакторинг.
Затем:
- + Рассказываем, в каких местах мы уверены, а в каких нет.
- + Следим за тональностью комментариев. Помним, что нейтральный тон может быть расценен как негативный. Везде, где уместно, используем позитивный тон.
- + Определяем способы упростить код, сохраняя при этом решение задачи.
- + Предлагаем альтернативные варианты, но считаем, что автор коммита уже рассматривал их.
- + Стараемся понять, как рассуждал автор коммита.
- + Не забываем хвалить удачные места в коде.
- + Чётко формулируем минимальные требования для успешного прохождения ревью, а также желательные, но не обязательные улучшения.
- + Ещё раз, стараемся быть позитивными!
Правила для тех, чей код проверяют
- + Благодарим ревьюера за его идеи.
- + Не принимаем комментарии на свой счёт. Ревьюят наш код, а не нас лично.
- + Объясняем, почему выбрали то или иное решение.
- + Стараемся понять точку зрения ревьюера.
- + Отвечаем на каждый комментарий.
- + Если есть возможность, обсуждаем комментарии ревьюера лично. Это может сильно ускорить процесс.
- + После того, как ревью завершилось, а мы уверены в коде и его влиянии на проект — можем делать merge.
Этот список принципов — чеклист по форме, но не по сути. С ужасом представляем себе человека, который методично идёт по всем пунктам: «Проследить за тональностью комментариев — сделано; постараться понять ход мысли автора — сделано; быть позитивным — сделано».
Главная польза, которую могут дать эти принципы — передать дух, задать правильный настрой на процесс. А ещё зафиксировать именно наш опыт: что работает лучше, а что хуже — знаем, потому что пробовали.
Чтобы пользоваться этими правилами, не обязательно помнить все пункты наизусть. Достаточно будет выучить одно мнемоническое правило: уважение друг к другу и польза для проекта. Все пункты списка — следствия из этих двух вещей.
Авторы:
команда Ixtens
Активно помогали:
Евгений Степченко,
Project Manager
Сергей Никитин,
Frontend Engineer