Как обычно происходит код-ревью? Вы отправляете пул-реквест, получаете обратную связь, вносите исправления, отправляете фиксы на повторный ревью, затем получаете одобрение, и происходит мерж. Звучит просто, но на деле процесс ревью бывает очень трудоемким.
Представьте, что у вас есть пул-реквест с сотнями строк изменений. Ревьюер должен потратить немало времени, чтобы полностью прочитать код и понять предлагаемые изменения. В результате весь процесс от создания пул-реквеста до его утверждения может занять несколько дней — в этом мало приятного и для ревьюера, и для автора изменений. И велики шансы, что в итоге ревьюер все равно что-то упустит. Или проверка может быть слишком поверхностной, а в худшем случае пул-реквест вообще может быть сразу отклонен.
Получается, что чем объемнее пул-реквест, тем меньше пользы будет от его проверки.
Как избежать таких ситуаций? Как сделать пул-реквест проще и понятнее для ревьюера и оптимизировать весь процесс?
Переводим статью нашего бэкенд-разработчика Сергея Жука про то, как устроен процесс код-ревью у команды мобильного приложения 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 файлов. Смело задавайте ему вопросы в комментариях к этой статье — он ответит!
Ну а также напоминаем, что у нас всегда много интересных вакансий для разработчиков!
Комментарии (23)
gekt0r
12.03.2019 15:53… Чтобы исправить ситуацию, можно разбить эти коммиты на отдельные ветки и создать пул реквесты для каждой из них...
Мне такой подход видится избыточным. Проще отделять разные изменения в разных коммитах в рамках одной ветки. Если ревьюеру необходимо — пусть посмотрит изменения каждого коммита в отдельности, а городить только ради ревью кучу веток считаю бесполезным
VIkrom
12.03.2019 16:50Практическое правило из заключения можно сократить: не создавайте огромных пулл-реквестов. Абсолютного «огромного» значения не существует, тут команда сама должна принять решение.
Daniyar94
12.03.2019 18:38Можно ссылку на оригинальную статью?
sshmakov
13.03.2019 09:12Я так понимаю, что автор seregazhuk
Хорошо написано, можно целиком вставлять в Code Style
MLman
13.03.2019 17:33Спасибо за статью.
Очень дельные советы.
Если бы все их придерживались — было бы супер )
UnclShura
13.03.2019 19:39Советы настолько очевидны, насколько и бесполезны. Поблема лежит в самом код ревью а не в подходе к нему. Почему-то сейчас считается, что ревьювить надо абсолютно все. Это имеет смысл, если вы не доверяете программисту или на вас свалились идиотсткие требования некомпетентных аудиторов. По большому счему нет вообще смысла ревьювить сотый однотипный рефакторинг сделаный высококвалифицированым разработчиком. Вообще нет смысла. Что вы там хотите увидеть? Что он наконец-то ошибся и не заметил, а вы заметили и теперь он лох? Зачем?
Тут сверху приведен нормальный сценарий работы программиста — добавил фичу, заметил, что можно порефакторить, порефакторил, исправил багу, откатил половину, т.д. Влезать в этот (тверческий!) процесс с регламентом нельзя. Значит надо как-то подстраивать процесс извне. Очень непопулярно сейчас, но я предпочитаю выборочные ревью по инициативе самого разработчика — когда надо стороннее мнение. Тупой рефакторинг — не ревьювить вообще. Требования аудиторов заткнуть формальным ревью, на которое требуется время не более, чем на кнопку нажать.
RomanVPro
Данный Вами совет и прост, и сложен для применения одновременно. Прост — потому что он очевиден: чем меньше изменений, тем легче сделать код ревью. Сложен — потому что как правило разработчик не ставит перед собой отдельные задачи: отрефакторить код, поправить кодстайл, добавить фичу. Все это происходит совместно, иногда чередуясь между собой: вот разработчик добавил новую фичу, вот он обнаружил дублирование кода, а вот увидел проблемы со стилем кода. То есть это может быть физически сложно разделить изменения на несколько мелких пул реквестов. Что делать с этой проблемой? Мы в команде пока не придумали каких-то конкретных правил.
JustDont
Ну, «что делать»-то тут вполне очевидно: сложная правка может быть разделена на несколько простых. Но это ручной труд разработчика — это не написание нового кода и даже не улучшение старого, это просто классификационное разделение всей правки так, чтоб другим разработчикам было читабельнее.
Если рассматривать эту ситуацию в контексте «сэкономленное время N ревьюверов важнее, чем потерянное время 1 коммитера» — то этим надо заниматься всегда и до упора.
Но так бывает мягко говоря не всегда — и вот тут-то и начитаются сложности.
RomanVPro
Так вопрос в том, как именно это сделать, когда файлы уже изменены, и разработчик даже видит, что правки разные: и код стайл, и рефакторинг, и новая фича здесь же. Похоже, что единственный вариант — откатить все изменения и сделать несколько пул реквестов уже с разбиением на категории? Но это же как минимум удваивает время, да и разработчик с ума сойдет :)
JustDont
А, вы в этом смысле.
Да, удобных инструментов переразбиения коммитов просто нет. И уж тем более таких, которые бы еще как-то бы препятствовали выстрелам в ногу — таких вообще даже в теории нет.
Johan
К сожалению, с инструментами в этом деле туго.
Однако, если ревьюеры достаточно непреклонны и нещадно разворачивают смешанные изменения, разбивать их очень быстро надоедает. Вырабатывается внутренняя дисциплина, если хотите.
Пример. Сидишь, правишь файлы, а тут, о ужас, стиль кодирования годичной давности залежался. Откладываешь все свои изменения, правишь стиль и отправляешь на ревью. Потом, накладываешь те изменения поверх исправленного стиля и продолжаешь.
И волки сыты, и овцы целы.
В качестве бонуса, не приходится заниматься одновременно несколькими вещами. В центре внимания всегда одна небольшая задача.
Мой личный опыт свидетельствует, что смена подхода происходит, буквально, после нескольких повторений.
JustDont
Которая нифига не помогает в случае серьезного рефакторинга.
Вот идёт какая-то переработка, в результате которой существенный кусок кода перекраивается — интерфейсы меняются, сорсы переносятся, ненужное выкидывается, и так далее.
И тут мы значит садимся, и начинаем думать, как это всё разбить. Сначала интерфейсы поменять? Это будет рефакторинг кода, который потом пойдет в /dev/null. Сначала новый код добавить? Это будет дублирование функций. Сначала старый убрать? Это вообще не сбилдится.
В итоге возникает необходимость пустопорожней работы: скажем, сначала пишем новый код на место старого и отправляем на ревью, а потом тут же садимся и новый код рефакторим до новых интерфейсов. И как угодно с этим возись, а красочная картинка не сложится — или большой и сложный пуллреквест, или «работа ради работы» чтоб реквест разбился на части и стал попроще.
Не все переделки кода можно разбить на мелкие и удобные для чтения части.
Johan
В моём понимании это не «работа ради работы», а «работа ради качества». Мы же применяем code review чтобы найти потенциальные проблемы, а при большом объёме изменений эта техника становится малоэффективной.
Собственно, любая техника контроля качества, которая подразумевает участие человека, теряет эффективность по мере роста этого самого объёма.
А вот с этим спорить не возьмусь, наверняка найдутся примеры. Миграция чего-нибудь со сломанной обратной совместимостью, может быть, ещё что-то.
cynovg
Заметил проблему — открыл задачу. Вполне возможно, что этой задачей могут заняться менее квалифицированные коллеги.
wsf
Задача упала в беклог, на нее потратили время при планировании, человек сидел час въезжал в контекст, потом создал ревью чем отвлек еще человека. А еще она просто может повиснуть в беклоге. А в реальности дел на минуту.
cynovg
В реальности вы подумали, что дел на минуту и допустили ошибку в коде рефакторинга и задачу завернули из-за кода, который к ней не имел никакого отношения.
wsf
для этого придумали тесты
cynovg
значит, вам с проектами везёт больше, чем мне.
wsf
Ну а как рефакторить без тестов? Вы наломаете больше чем пользы получите
cynovg
Тесты — это не панацея.