Как провести код-ревью: советы по применению

Дарья Маташина Продуктовый редактор

15 апреля 2022

Разбираемся, кому и когда нужна (и не нужна!) дополнительная проверка кода, и каких ошибок избежать на старте.

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

Что такое код-ревью и зачем его проводят?

Код-ревью — это процесс проверки кода, который позволяет:

выявить → ошибки, пропуски, уязвимости и стилистические недочеты (с точки зрения проекта или принятых в команде правил).

Пример диалога между автором и ревьюером.

улучшить →

→ читаемость и понятность кода для других разработчиков.

→ архитектурные решения. Например, разбиение на модули, code style решения, неверно подобранный паттерн проектирования.

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

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

Когда не нужно проводить код-ревью?

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

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

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

Причины, чтобы отказаться от постоянной практики код-ревью:

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

Как организовать процесс проверки кода?

После выполнения задачи автор кода делает в отдельной ветке push в удаленный репозиторий и создает Merge Request (MR).

Назначается assignee (ответственный за MR) и reviewer (ответственный за проверку).

Ответственный за MR и ответственный за проверку.

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

Раунды

Перед стартом ревьюер должен оценить объем MR и определить, сможет ли его проверить на «одном дыхании»‎ — не теряя концентрации. Если объем MR слишком большой, советуем разбить его на части поменьше. Чем объемнее решение, тем ниже эффективность проверки.

Заряд батарейки — количество энергии участников ревью от раунда к раунду.

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

Если таких проблем не нашлось, уровень проверки понижается до тех пор, пока не найдутся места для улучшений.

«В первом раунде не стоит акцентировать внимание на мелких недочетах. Скорее всего, автор сам их обнаружит и поправит, и ревьюеру не придется тратить время на поиск незначительных проблем»‎, — отмечает разработчик Selectel Антон Щербак.

После код возвращается автору: он вносит изменения и снова отправляет на проверку. Процесс продолжается до тех пор, пока решение не признают приемлемым — оно одобряется тем, кто проводил ревью, и после assignee выполняет merge.

«Решение не должно быть идеальным — оно должно соответствовать потребностям проекта и выполнять поставленную задачу»‎, — резюмирует Антон Щербак.

Как понять, что решение готово?

→ Оно не имеет явных ошибок.

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

опечатки;
ошибки, возникшие из-за недостаточного погружения в контекст проекта;
необновленный конфигурационный файл.

→ Соответствует стайл-гайдам, принятым в команде.

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

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

→ Не затрагивает код, выходящий за рамки задачи.

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

→ Пропущено через линтеры, используемые в проекте.

Большинство команд в Selectel использует pre-commit — так при каждом коммите код прогоняется через линтеры. Для Python используется black, isort, flake8, pyupgrade и autoflake.

→ Обладает хорошей читаемостью и пояснением неочевидных мест.

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

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

→ Если на проекте пишутся автотесты, решение должно ими покрываться.

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

→ Нет оверинжениринга (кода «на будущее»‎).

Часто код, который решает еще не возникшие проблемы, не пригождается и становится лишним.

Еще Дональд Кнут говорил: преждевременная оптимизация — корень всех проблем в программировании.

Советы по процессу

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

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

Второй: используйте префикс NIT (сокращение от nitpick) для комментариев, которые даны в качестве рекомендаций. Помните: они не обязательны к исполнению, автор волен сам решать, следовать им или нет.

«Разработчики часто спорят о названиях переменных. Бывает, что эти нововведения никак не влияют на итоговое решение. Поэтому делать пометку в начале — нормальная практика: она говорит разработчику, что решение о внесении в код изменений остается за ним. Эти изменения не обязательны — решение и так хорошее‎»‎, — рассказывает разработчик Selectel Антон Щербак.

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

Так не стоит комментировать → «Ты тут ошибся, поправь по моему примеру. Вот ссылка».

Так лучше → «Мы уже сталкивались с аналогичной ситуацией. Вот как эта проблема решилась тут → ссылка. Можем ли реализовать нечто подобное?»

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