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. Если в вашей задаче необходимо произвести схожие действия в большом количестве репозиториев, то РЕКОМЕНДУЕТСЯ выполнить их в каком-то одном, получить оперативное ревью, поправить замечания и только потом уже трогать остальные репозитории.
  10. При создании MR НЕОБХОДИМО отметить опцию "Удалить после слияния"(Delete source branch when merge request accepted). Если планируется делать слияние не через интерфейс RMS, то после слияния необходимо удалить исходную ветку.

Список исключений позволяющих мержить код без 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. Отписываемся ревьюверу в личку если нужен ревью в рамках текущего дня;