Что такое код-ревью

В индустрии разработки программ очень распространена практика код-ревью. Программист отправляет написанный код своим коллегам — они просматривают его и высказывают свои замечания.

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

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

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

Пулреквест (PR) — это предложение слить изменения в ветке разработчика с другой веткой. Иногда их называют мёрж-реквестами (MR).

Типичный код-ревью к пулреквесту на GitHub выглядит так:

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

Подобные замечания не ставят приоритет комментариям, поэтому они мало полезны:

Это неправильно сформулировано.

Если поставить перед комментарием лэйбл, намерение станет ясным, а тон резко изменится.

suggestion: Это неправильно сформулировано.

nitpick (non-blocking): Это неправильно сформулировано.

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

suggestion: Это неправильно сформулировано
Можем ли мы изменить этот текст, чтобы он соответствовал формулировке со страницы маркетинга?

Conventional comments предлагает формат, где сообщение описывается как:

<label> [decorations]: <subject>
[discussion]

• label - тип комментария;
• subject - основная мысль комментария;
• decorations (опционально) - дополнительные лэйблы для комментария. Они окружены скобками;
• discussion (опционально) - здесь содержатся подтверждающие заявления, контекст, рассуждения и все остальное, чтобы помочь сообщить "почему" и "последующие шаги" для разрешения замечания;

Например:

question (non-blocking): На этом этапе имеет значение, какой поток выиграл?
Может быть, чтобы предотвратить состояние гонки, мы должны продолжать зацикливаться, пока все они не выиграют?

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

Не стесняйтесь отклоняться от этого конкретного списка лэйблов, если это кажется уместным.

Декорации дают дополнительный контекст для комментария.

suggestion (security): Я немного обеспокоен тем, что мы внедряем собственную функцию очистки DOM здесь...
Можем ли мы вместо этого рассмотреть возможность использования фреймворка?

suggestion (test, if-minor): Похоже, что мы пропустили какие-то тесты

Декорации могут быть индивидуальными для каждой команды. Рекомендуется установить минимальный набор декораций на ваше усмотрение.

Возможные декорации включают:

• (non-blocking) - комментарий с такой декорацией не должен препятствовать принятию рассматриваемого пулреквеста. Это полезно для команд, которые рассматривают блокирование комментариев по умолчанию.
• (blocking) - комментарий с такой декорацией должен препятствовать принятию рассматриваемого вопроса до тех пор, пока он не будет решён. Это полезно для команд, которые считают, что комментарии не блокируются по умолчанию.
• (if-minor) - эта декорация даёт автору некоторую свободу действий, которая заключается в том, что он может разрешить комментарий только в том случае, если изменения окажутся незначительными или тривиальными.

@elain-mask
nitpick (non-critical): Некритично, но для консистентности папку с изображениями батутов назвал бы assets.

@vasya_pupkin
praise: Супер! Отлично поработал над этой фичей!

@anonymous
issue(critical): Здесь уж слишком разбухшая логика
suggestion(critical): Сейчас не особо больно, но по-хорошему бы декомпозировать это все дело

Преимущества и недостатки общепринятых комментариев (они же conventional comments):

Преимущества

• Цель и смысл наших комментариев ясна.
• Разделение между темой комментария и аргументацией очень чёткое и прямое.
• Автор знает, с каким приоритетом стоит расставлять замечания, потому, что комментарии хорошо размечены.

Издержки

• Категоризация комментариев может работать не всегда, что-то обязательно не вместится в список.
• Формата нужно как-то обязывать придерживаться, а это дополнительные расходы на инфраструктуру или документацию.
• Дополнительная когнитивная нагрузка при ревью.

• Conventional comments
• Github saved replies

На практике

Скопировать ссылку "На практике" Скопировано

Игорь Камышев советует

Скопировать ссылку "Игорь Камышев советует" Скопировано

Код-ревью — это область, где особенно ярко проявляются софт-скиллы инженеров. Провести хорошее код-ревью сложнее, чем написать хороший код.

Не расстраиваться

Скопировать ссылку "Не расстраиваться" Скопировано

Не стоит воспринимать комментарии к коду как личное оскорбление.

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

Ругать код, а не автора

Скопировать ссылку "Ругать код, а не автора" Скопировано

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

❌ «Ты болван, тут нужно использовать пул коннектов!»

✅ «Слушай, а почему тут не используется пул коннектов? Мне кажется, это может привести к проблемам с производительностью».

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

Смотреть вглубь

Скопировать ссылку "Смотреть вглубь" Скопировано

При обзоре чужого решения велик соблазн давать мелкие советы. Это называется эффектом велосипедного сарая (bikeshedding). Он создаётся, потому что для обсуждения сложных, глубоких вопросов нужно сильно погружаться в контекст внесённых изменений, а это трудно. Намного проще написать десяток комментариев о забытых точках с запятой и спокойно продолжить заниматься своей задачей.

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

+ Развернуть