Как сделать код-ревью быстрее и эффективнее

в 11:26, , рубрики: code review, open source, php, Блог компании Skyeng, Программирование, Проектирование и рефакторинг, управление разработкой

image

Как обычно происходит код-ревью? Вы отправляете пул-реквест, получаете обратную связь, вносите исправления, отправляете фиксы на повторный ревью, затем получаете одобрение, и происходит мерж. Звучит просто, но на деле процесс ревью бывает очень трудоемким.

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

Получается, что чем объемнее пул-реквест, тем меньше пользы будет от его проверки.

Как избежать таких ситуаций? Как сделать пул-реквест проще и понятнее для ревьюера и оптимизировать весь процесс?

Переводим статью нашего бэкенд-разработчика Сергея Жука про то, как устроен процесс код-ревью у команды мобильного приложения Skyeng.

Категории изменений

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

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

  1. Функциональные изменения.
  2. Структурный рефакторинг — изменения классов, интерфейсов, методов, перемещения между классами.
  3. Простой рефакторинг — может быть выполнен с помощью IDE (например, извлечение переменных/методов/констант, упрощение условий).
  4. Переименование и перемещение классов — реорганизация пространства имен.
  5. Удаление неиспользуемого (мертвого) кода.
  6. Исправления code style.

Стратегии ревью

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

  1. Изменение функционала: решение бизнес-задач и дизайн системы.
  2. Структурный рефакторинг: обратная совместимость и улучшение дизайна.
  3. Примитивный рефакторинг: улучшение читабельности. Эти изменения в основном можно сделать при помощи IDE (например, извлечение переменных/методов/констант и прочее).
  4. Переименование/удаление классов: улучшилась ли структура пространства имен?
  5. Удаление неиспользуемого кода: обратная совместимость.
  6. Исправления code style: чаще всего мерж пул-реквеста происходит сразу же.

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

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

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

При проверке остальных категорий в 99 % случаев мерж происходит сразу же.

  1. Простой рефакторинг. Код стал более читабельным? — мержим.
  2. Переименование/перемещение классов. Класс был перемещен в лучшее пространство имен?— мержим.
  3. Удаление неиспользованного (мертвого) кода — мержим.
  4. Исправления code style или форматирования — мержим. Ваши коллеги не должны проверять это во время код-ревью, это задача линтеров.

Почему нужно разделять изменения по категориям?

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

Что точно не стоит смешивать

Переименование/удаление класса и его рефакторинг. Здесь мы сталкиваемся с Git, который не всегда правильно понимает такие изменения. Я имею в виду масштабные изменения, когда меняется много строк. Когда вы рефакторите класс, а затем перемещаете его куда-то, Git не воспринимает это как перемещение. Вместо этого Git интерпретирует эти изменения как удаление одного класса и создание другого. Это приводит к куче вопросов во время код-ревью. И автора кода спрашивают, почему он написал этот уродливый код, хотя на самом деле этот код был просто перемещен из одного места в другое с небольшими изменениями.

Любые функциональные изменения + любой рефакторинг. Мы уже обсуждали этот случай выше. Это заставляет ревьюера держать в голове сразу две стратегии рецензирования. Даже если рефакторинг выполнен хорошо, мы не сможем смержить эти изменения, пока функциональные изменения не будут утверждены.

Любые механические изменения + любые изменения, произведенные человеком. Под «механическими изменениями» я подразумеваю любое форматирование, выполненное с помощью IDE или генерации кода. Например, мы применяем новый code style и получаем изменений на 3000 строк. И если мы смешаем эти изменения с какими-либо функциональными или любыми другими изменениями, произведенными человеком, мы заставим ревьюера мысленно классифицировать эти изменения и рассуждать: это изменение, произведенное компьютером, — его можно пропустить, а это изменение, сделанное человеком, — его нужно проверить. Так ревьюер тратит очень много дополнительного времени на проверку.

Пример

Вот пул-реквест с функцией метода, который аккуратно закрывает клиентское соединение с Memcached:

image

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

  • функциональные (новый код),
  • рефакторинг (создание/перемещение классов),
  • исправления code style (удаление лишних док-блоков).

В то же время сам новый функционал составляет всего 10 строк:

image

В результате ревьюер должен просмотреть весь код и

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

Поэтому такой пул-реквест сложно ревьюить. Чтобы исправить ситуацию, можно разбить эти коммиты на отдельные ветки и создать пул реквесты для каждой из них.

1. Рефакторинг: извлечение класса

image

Здесь всего два файла. Ревьюер должен проверить только новый дизайн. Если все в порядке — мержим.

2. Следующим шаг — тоже рефакторинг, мы просто перемещаем два класса в новое пространство имен

image

Такой пул-реквест довольно просто проверять, он может быть смержен сразу.

3. Удаление лишних блоков документов

image

Здесь ничего интересного. Мержим.

4. Сам функционал

image

И теперь пул-реквест с функциональными изменения содержит только нужный код. Так ваш ревьюер может сосредоточиться только на этой задаче. Пул-реквест небольшой и его легко проверить.

Заключение

Практическое правило:

Не создавайте огромных пул-реквестов со смешанными категориями изменений.

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

И всегда выполняйте следующие шаги до отправки пул-реквеста:

  • оптимизируйте свой код для чтения. Код гораздо чаще читается, чем пишется;
  • опишите предлагаемые изменения, чтобы обеспечить необходимый контекст для их понимания;
  • всегда проверяйте свой код перед созданием пул-реквеста. И делайте это так, как будто это чужой код. Иногда это помогает найти что-то, что вы упустили. Это снизит вероятность отклонения вашего пул-реквеста и количество исправлений.

Об идее разбивать изменения на категории мне рассказал мой коллега Олег Антоневич.

От редакции: Сергей много пишет интересного про программирование и PHP, а мы иногда что-то переводим: сервер потокового видео, рендеринг HTML файлов. Смело задавайте ему вопросы в комментариях к этой статье — он ответит!

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

Автор: Skyeng-Habr

Источник

* - обязательные к заполнению поля


https://ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js