Хабрахабр

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

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 интерпретирует эти изменения как удаление одного класса и создание другого. Это приводит к куче вопросов во время код-ревью. И автора кода спрашивают, почему он написал этот уродливый код, хотя на самом деле этот код был просто перемещен из одного места в другое с небольшими изменениями.

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

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

Пример

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

image

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

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

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

image

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

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

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

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

image

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

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

image

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

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

image

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

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

image

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

Заключение

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

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

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

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

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

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

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

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

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть