Как обычно происходит код-ревью? Вы отправляете пул-реквест, получаете обратную связь, вносите исправления, отправляете фиксы на повторный ревью, затем получаете одобрение, и происходит мерж. Звучит просто, но на деле процесс ревью бывает очень трудоемким.
Представьте, что у вас есть пул-реквест с сотнями строк изменений. Ревьюер должен потратить немало времени, чтобы полностью прочитать код и понять предлагаемые изменения. В результате весь процесс от создания пул-реквеста до его утверждения может занять несколько дней — в этом мало приятного и для ревьюера, и для автора изменений. И велики шансы, что в итоге ревьюер все равно что-то упустит. Или проверка может быть слишком поверхностной, а в худшем случае пул-реквест вообще может быть сразу отклонен.
Получается, что чем объемнее пул-реквест, тем меньше пользы будет от его проверки.
Как избежать таких ситуаций? Как сделать пул-реквест проще и понятнее для ревьюера и оптимизировать весь процесс?
Переводим статью нашего бэкенд-разработчика Сергея Жука про то, как устроен процесс код-ревью у команды мобильного приложения Skyeng.
Категории изменений
Давайте представим, что у вас есть задача — реализовать новый функционал в проекте. Пул-реквест, над которым вы работаете, может содержать разные категории изменений. Конечно в нём будет какой-то новый код. Но в ходе работы вы можете заметить, что какой-то код нужно предварительно порефакторить, чтобы он способствовал добавлению нового функционала. Или с этим новым функционалом в коде появилось дублирование, которое вы хотите устранить. Или вы вдруг обнаружили ошибку и хотите ее исправить. Как должен выглядеть окончательный пул-реквест?
Сначала давайте разберемся, какие категории изменений могут происходить с кодом.
- Функциональные изменения.
- Структурный рефакторинг — изменения классов, интерфейсов, методов, перемещения между классами.
- Простой рефакторинг — может быть выполнен с помощью IDE (например, извлечение переменных/методов/констант, упрощение условий).
- Переименование и перемещение классов — реорганизация пространства имен.
- Удаление неиспользуемого (мертвого) кода.
- Исправления code style.
Стратегии ревью
Очень важно понимать, что каждая из этих категорий ревьюирутеся по-разному и при их рассмотрении ревьюер должен фокусироваться на разных вещах. Можно сказать, что каждая категория изменений предполагает свою собственную стратегию ревью.
- Изменение функционала: решение бизнес-задач и дизайн системы.
- Структурный рефакторинг: обратная совместимость и улучшение дизайна.
- Примитивный рефакторинг: улучшение читабельности. Эти изменения в основном можно сделать при помощи IDE (например, извлечение переменных/методов/констант и прочее).
- Переименование/перемещение классов: улучшилась ли структура пространства имен?
- Удаление неиспользуемого кода: обратная совместимость.
- Исправления code style: чаще всего мерж пул-реквеста происходит сразу же.
Для проверки изменений разных категорий используются разные подходы, поэтому количество времени и усилий, затрачиваемые на их ревью, также различаются.
Функциональные изменения. Это самый длительный процесс, потому что он предполагает изменения доменной логики. Ревьюер смотрит, решена ли проблема и проверяет, является ли предложенное решение наиболее подходящим или его можно улучшить.
Структурный рефакторинг. Этот процесс требует гораздо меньше времени, чем функциональные изменения. Но здесь могут возникнуть предложения и разногласия по поводу того, как именно код должен быть организован.
При проверке остальных категорий в 99 % случаев мерж происходит сразу же.
- Простой рефакторинг. Код стал более читабельным? — мержим.
- Переименование/перемещение классов. Класс был перемещен в лучшее пространство имен?— мержим.
- Удаление неиспользованного (мертвого) кода — мержим.
- Исправления code style или форматирования — мержим. Ваши коллеги не должны проверять это во время код-ревью, это задача линтеров.
Почему нужно разделять изменения по категориям?
Мы уже обсуждали, что разные категории изменений ревьюируются по-разному. Например, функциональные изменения мы проверяем, отталкиваясь от требований бизнеса, а при структурном рефакторинге — проверяем обратную совместимость. И если мы смешаем несколько категорий, то ревьюеру будет трудно держать в уме одновременно нескольких стратегий ревью. И, скорее всего, ревьюер потратит на пул-реквест больше времени, чем необходимо, и из-за этого может что-то упустить. Более того, если пул-реквест содержит изменения разного рода, при любом исправлении ревьюеру придется пересмотреть все эти категории еще раз. Например, вы смешали структурный рефакторинг и функциональные изменения. Даже если рефакторинг выполнен хорошо, но есть проблема с реализацией функционала, то после исправлений ревьюер должен будет просмотреть весь пул-реквест с самого начала. То есть проверить заново и рефакторинг, и функциональные изменения. Так проверяющий тратит на пул-реквест больше времени. Вместо того, чтобы чтобы сразу смержить отдельный пул-реквест с рефакторингом, ревьюер должен еще раз просмотреть весь код.
Что точно не стоит смешивать
Переименование/удаление класса и его рефакторинг. Здесь мы сталкиваемся с Git, который не всегда правильно понимает такие изменения. Я имею в виду масштабные изменения, когда меняется много строк. Когда вы рефакторите класс, а затем перемещаете его куда-то, Git не воспринимает это как перемещение. Вместо этого Git интерпретирует эти изменения как удаление одного класса и создание другого. Это приводит к куче вопросов во время код-ревью. И автора кода спрашивают, почему он написал этот уродливый код, хотя на самом деле этот код был просто перемещен из одного места в другое с небольшими изменениями.
Любые функциональные изменения + любой рефакторинг. Мы уже обсуждали этот случай выше. Это заставляет ревьюера держать в голове сразу две стратегии рецензирования. Даже если рефакторинг выполнен хорошо, мы не сможем смержить эти изменения, пока функциональные изменения не будут утверждены.
Любые механические изменения + любые изменения, произведенные человеком. Под «механическими изменениями» я подразумеваю любое форматирование, выполненное с помощью IDE или генерации кода. Например, мы применяем новый code style и получаем изменений на 3000 строк. И если мы смешаем эти изменения с какими-либо функциональными или любыми другими изменениями, произведенными человеком, мы заставим ревьюера мысленно классифицировать эти изменения и рассуждать: это изменение, произведенное компьютером, — его можно пропустить, а это изменение, сделанное человеком, — его нужно проверить. Так ревьюер тратит очень много дополнительного времени на проверку.
Пример
Вот пул-реквест с функцией метода, который аккуратно закрывает клиентское соединение с Memcached:
Даже если пул-реквест небольшой и содержит всего сто строк кода, его все равно сложно проверять. Как видно по коммитам, он содержит различные категории изменений:
- функциональные (новый код),
- рефакторинг (создание/перемещение классов),
- исправления code style (удаление лишних док-блоков).
В то же время сам новый функционал составляет всего 10 строк:
В результате ревьюер должен просмотреть весь код и
- проверить, что рефакторинг в порядке;
- проверить, правильно ли реализован новый функционал;
- определить, было ли это изменением произведено автоматически IDE или человеком.
Поэтому такой пул-реквест сложно ревьюить. Чтобы исправить ситуацию, можно разбить эти коммиты на отдельные ветки и создать пул реквесты для каждой из них.
1. Рефакторинг: извлечение класса
Здесь всего два файла. Ревьюер должен проверить только новый дизайн. Если все в порядке — мержим.
2. Следующим шаг — тоже рефакторинг, мы просто перемещаем два класса в новое пространство имен
Такой пул-реквест довольно просто проверять, он может быть смержен сразу.
3. Удаление лишних блоков док-блоков
Здесь ничего интересного. Мержим.
4. Сам функционал
И теперь пул-реквест с функциональными изменениями содержит только нужный код. Так ваш ревьюер может сосредоточиться только на этой задаче. Пул-реквест небольшой и его легко проверить.
Заключение
Практическое правило:
Не создавайте огромных пул-реквестов со смешанными категориями изменений.Чем больше пул-реквест, тем труднее ревьюеру понять предложенные вами изменения. Скорее всего, огромный пул-реквест с сотнями строк кода будет отклонен. Вместо этого разбейте его на маленькие логические части. Если ваш рефакторинг в порядке, но функциональные изменения содержат ошибки, то рефакторинг можно спокойно смержить, и таким образом вы и ваш ревьюер сможете сосредоточиться на функционале, не просматривая весь код с самого начала.
И всегда выполняйте следующие шаги до отправки пул-реквеста:
- оптимизируйте свой код для чтения. Код гораздо чаще читается, чем пишется;
- опишите предлагаемые изменения, чтобы обеспечить необходимый контекст для их понимания;
- всегда проверяйте свой код перед созданием пул-реквеста. И делайте это так, как будто это чужой код. Иногда это помогает найти что-то, что вы упустили. Это снизит вероятность отклонения вашего пул-реквеста и количество исправлений.
Об идее разбивать изменения на категории мне рассказал мой коллега Олег Антоневич.
От редакции: Сергей много пишет интересного про программирование и PHP, а мы иногда что-то переводим: сервер потокового видео, рендеринг HTML файлов. Смело задавайте ему вопросы в комментариях к этой статье — он ответит!
Ну а также напоминаем, что у нас всегда много интересных вакансий для разработчиков!
