Skip to main content

Code Review Guide

Введение и глоссарий​

Code review (CR) - инженерная практика в терминах гибкой методологии разработки. Она заключается в анализе (инспекции) кода с целью выявить ошибки, недочеты, расхождения в стиле написания кода, несоответствие написанного кода и поставленной задачи.

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

К преимуществам Code Review относят:

  1. Улучшается качество кода
  2. Находятся «глупые» ошибки (опечатки) в реализации
  3. Повышается степень совместного владения кодом
  4. Код приводится к единому стилю написания
  5. Происходит выравнивание опыта, обмен знаниями.

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

Project Management Applicaton (PMA) - приложение по управлению проектами включающая в себя Issue Tracker. Например, Redmine или Jira.

Repository Management System (RMS) - системы управления репозиториями. Например, Gitlab или Github.

*Merge Request (MR) или Pull Request (PR) - это запрос на вливание одной ветки git в другую, который имеет определенное количество привязанной к нему мета-информации. В RMS запрос доступен для просмотра и взаимодействия на выделенной ему странице. Пример.

Роли в процессе Code Review (не путайте с ролями RMS определяющими доступ!):​

  • Author- человек, который создал MR
  • Reviewer - человек, который проводит Code Review
  • Assignee (Ответственный) - человек, который ответственный за то чтобы MR был смержен, закрыт или передан другому Assignee. Обычно совпадает с Author

Основные положения​

  1. В качестве платформы для Code Review ДОЛЖНЫ использоваться Merge Requests используемой на проекте RMS.
  2. Любой код попадающий в общую ветку ДОЛЖЕН проходить Code Review если он не попадает под список исключений содержащийся в данном гайде.
  3. При создании MR НЕОБХОДИМО назначить ему Assignee. Наличие MR без Assignee не допускается.
  4. Как только MR готов к Review НЕОБХОДИМО назначить ревьювера в поле Reviewer. Выбор ревьювера описан ниже
  5. В MR РЕКОМЕНДУЕТСЯ добавлять только изменения касающиеся непосредственно вашей задачи. Побочные изменения следуют заводить как отдельный MR с отдельным Code Review
  6. Если вы понимаете что MR получается очень большим - подумайте как упростить ревьюверу жизнь - отдавайте его на ревью по мере готовности, а лучше вообще разбейте на несколько MR.
  7. При проведении Code Review важно оставаться доброжелательным, позитивным и конструктивным, избегать конфликтов.
  8. Если всё же в ходе ревью Author и Reviewer не могут сойтись во мнениях, последнее слово остается за Author за исключением тех случаев когда им является разработчик с ролью Developer.
  9. Если в вашей задаче необходимо произвести схожие действия в большом количестве репозиториев, то РЕКОМЕНДУЕТСЯ выполнить их в каком-то одном, получить оперативное ревью, поправить замечания и только потом уже трогать остальные репозитории.

Список исключений позволяющих мержить код без Code Review​

  1. Нужна срочная отгрузка важного хотфикса на прод, а ревьюеверы недоступны. В этом случае допускается мерж MR до аппрува с ревью. Ревью проводится постфактум, возможные исправления идут через новую ветку
  2. Речь идет исключительно об автоматически сгенерированном коде, проводить ревью которого бессмысленно.
  3. В команде нет второго разработчика соответствующего профиля, а подходящие разработчики из других команд не готовы взять на себя ревью кода команды.

Выбор ревьювера​

Ревьювера выбирает автор MR. Задача выбора не имеет какого-то правильного или неправильного ответа, слишком много факторов, однако некоторые гайдлайны всё-равно можно дать:

  1. Специализация ревьювера ДОЛЖНА соответствовать коду который отправляется на ревью. Бэкэнд код отправляем бэкэндеру, изменения во фронтэнде - фронтэндеру, изменения в CI/CD файлах - DevOps инженеру и т. д.;
  2. Если ваша роль в конкретном репозитории Developer, то вы ДОЛЖНЫ выбрать Reviewer-a с ролью Maintainer или выше чтобы не допустить ситуации с попаданием кода в общую ветку без ведома Maintainer-ов репозитория;
  3. СЛЕДУЕТ учитывать насколько человек знаком с изменяемым участком кода. Например если вы вдвоем работаете над одной историей, то имеет смысл давать на ревью задачи друг-другу;
  4. СЛЕДУЕТ учитывать загруженность человека и то насколько срочно вам нужно получить фидбэк по ревью;

Когда проводить ревью​

РЕКОМЕНДУЕТСЯ проводить ревью не реже одного раза в день. Например, утром когда вы начинаете рабочий день.

Добавьте себе в закладки страницу на которой показываются все MR которые вам необходимо отревьюшить:

Gitlabhttps://gitlab.com/dashboard/merge_requests?scope=all&state=opened&reviewer_username=логин&draft=no
Githubhttps://github.com/pulls?q=is%3Aopen+is%3Apr+review-requested%3Aлогин+archived%3Afalse+draft%3Afalse

Аналогично мониторьте Assigned To Me -

Gitlabhttps://gitlab.com/dashboard/merge_requests?assignee_username=логин
Githubhttps://github.com/pulls/assigned

Cтарайтесь держать все эти страницы максимально пустыми.

Если MR Author написал вам в личку, что поправил все замечания, то постарайтесь провести Code Review сразу после этого. Растягивание ревью на несколько дней нежелательно.

Если вы разработчик платформы и являетесь maintainer-ом её опенсорс пакетов, то вам необходимо мониторить Assigned/Review не только в RMS вашего проекта, но и для опенсорс пакетов платформы, которые расположены в Github.

Как проводить ревью​

  1. Смотрим постановку задачи в PMA если она есть;
  2. Смотрим описание MR если оно есть;
  3. Смотрим измененные в MR файлы;
  4. Убеждаемся, что нет конфликтов с веткой куда направлен MR;
  5. Убеждаемся, что все CI проверки успешно пройдены или провалены по причинам выходящим за рамки MR;
  6. Оцениваем правильный ли путь был выбран для реализации задачи;
  7. Оцениваем соответствует ли задача стандартам качества разработки принятым в проекте;

Если вы считаете что что-то в MR необходимо изменить - пишем комментарии у нужных строк непосредственно в MR, возвращаем задачу в статус "Новая", переводим его в статус Draft и пишем комментарий в PMA "необходимо внести изменения после ревью".

Во всех популярных RMS можно использовать code suggestions через кнопку "Insert a suggestion" в форме добавления комментария к строке. Автор MR может закоммитить такой комментарий сразу со страницы MR, что экономит время.

Если вы удовлетворены кодом в MR - нажимаем Approve и снимаем с себя Reviewer.

Если вы считаете что задача требует Review еще от кого-то (например, затронуты CI/CD файлы и требуется ревью DevOps-инженера) - назначаем нового ревьювера

Code Review НЕ подразумевает того что вы будете тестировать задачу в рамках этого процесса.

При проведении Code Review важно оставаться прагматичным и оценивать есть ли смысл тратить время на исправление тех или иных недочетов.

Как дать понять ревьюверу, что ему нужно провести Code Review вашей задачи​

Если вопрос не очень срочный, то достаточно назначить его ревьювером в Gitlab. Обычно в течение суток человек отревьюшит вашу задачу.

Если вам нужно более оперативное ревью или человек почему-то игнорирует ваш MR, вы ДОЛЖНЫ написать ему личное сообщение об этом.

Как обрабатывать замечания, полученные в ходе Code Review​

  1. Открываем страницу MR в RMS;
  2. Исправляем все замечания, которые с вашей точки зрения обоснованы, пушим их в ту же ветку;
  3. Если есть замечания с которыми вы не согласны - отписываемся по ним либо там же, либо в личку ревьюверу;
  4. НЕ резолвим самостоятельно никакие замечания ревьювера;
  5. Когда всё готово убираем у MR статус Draft нажав на кнопку Ready for Review или её аналог, проверяем что стоит корректный Reviewer для повторного ревью;
  6. Отписываемся ревьюверу в личку если нужен ревью в рамках текущего дня;