Google: Why you should write small CLs?

Google, основываясь на многолетнем опыте, представил свои стандарты того, как лучше всего проводить code review. Все вместе они представляют собой один законченный документ, разбитый на множество отдельных разделов. Это перевод стандартов по проведению Code Review от Google. Вот ссылка на оригинал!

Терминология:

CL — расшифровывается как «список изменений» (“ChangeList”), что означает одно автономное изменение, которое было отправлено в систему управления версиями или подвергается проверке кода. Другие организации часто называют это «изменением» или «патчем».

Почему лучше писать небольшие CL?

• Проверка проходит быстрее. Для ревьюера легче выделить по пять минут на небольшие CL, чем полчаса для просмотра одного большого.
• Проверка проходит внимательнее. При серьезных изменениях ревьюеры и авторы, как правило, разочаровываются большими объемами комментариев до такой степени, что упускают важные моменты.
• Маленькая вероятность ошибки. Поскольку вы вносите небольшие изменения, вам и вашему ревьюеру будет легче понять их влияние на CL и посмотреть, не была ли допущена ошибка.
• Будет не так обидно, если CL отклонят. Если вы пишете большой CL, а затем ваш ревьюер говорит, что общее направление неверно, то будет довольно обидно потратить так много времени на бесполезный CL.
• Проще сделать merge с основной веткой. Работа над большим CL занимает много времени, поэтому у вас будут конфликты при слиянии с основной веткой.
• Проще спроектировать хороший дизайн. Намного легче проработать дизайн и исправность кода небольшого изменения, чем уточнить все детали большого.
• Меньше блокировок по проверке кода. Отправка небольших частей вашего общего изменения позволяет продолжить писать код, пока вы ожидаете пересмотра текущего CL.
• Проще откатиться назад. Как правило, большой CL касается файлов, которые обновляются между первоначальной отправкой CL и откатом CL, что усложняет откат (промежуточные CL, вероятно, тоже придется откатывать).

Обратите внимание, что ревьюеры могут отклонить ваш CL по причине чересчур большого количества изменений. Обычно они просят разбить его на несколько частей. А для этого вам потребуется много времени и сил. Поэтому проще сразу писать маленькие CL.

Что значит небольшие изменения?

В целом, правильный размер для CL — это отдельное изменение. Это значит, что:

• CL вносит минимальное изменение, касающееся только одной фичи. Обычно это только одна часть функции, а не целая функция одновременно. Лучше ошибиться в написании слишком маленьких CL, чем слишком больших. Обсудите с вашим ревьюером приемлемый размер.
• Все, что ревьюер должен знать о CL, содержится в CL, описании CL, существующей кодовой базе или CL, которые уже проверены.
• Система будет продолжать работать хорошо для своих пользователей и разработчиков после добавления CL.
• CL не настолько мал, что его последствия трудно понять. Если вы добавляете новый API, вы должны включить использование API в тот же CL, чтобы ревьюеры могли лучше понять, как будет использоваться API. Это также предотвращает проверку неиспользуемых API.

Не существует четких правил относительно размера CL. Обычно 100 строк — это разумный размер для CL, а 1000 строк обычно слишком велик, но все зависит от вашего ревьюера. Количество файлов, которые затрагивает изменение, также влияет на размер CL. Изменение в 200 строк в одном файле может быть приемлемым, но при распределении по 50 файлам будет слишком большим.

Имейте в виду, что, хотя вы были тесно связаны с вашим кодом с момента его написания, ревьюер часто не имеет контекста. То, что вам кажется приемлемым размером CL, может быть довольно большим для вашего ревьюера. Если вы сомневаетесь, напишите CL меньшего размера. Ревьюеры редко жалуются на получение слишком маленьких CL.

Когда большие CL — хорошо?

Есть несколько ситуаций, в которых большие изменения не так плохи:

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

Разделение по файлам

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

Например: вы отправляете один CL для изменений в протоколе буфера, а другой CL для изменений в коде, который использует этот протокол. Вы должны отправить первый CL перед вторым, но оба они могут быть рассмотрены одновременно. Если вы сделаете это, вы можете сообщить обоим ревьюерам о других CL, чтобы у них был контекст по вашим изменениям.

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

Раздельный рефакторинг

Обычно лучше проводить рефакторинг отдельно от изменений функций или исправлений ошибок. Например, перемещение и переименование класса должны быть в одном CL, а исправление ошибки в этом классе в другом. Ревьюерам намного легче понять изменения, вносимые каждым CL, когда они разделены.

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

Выполняйте тесты в том же CL

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

Тем не менее, независимые тестовые модификации могут выполняться в отдельных CL, аналогично рекомендациям по рефакторингу. Например:

• проверка ранее существующего, отправленного кода с новыми тестами.
• рефакторинг тестового кода (например, введение вспомогательных функций).
• введение большой части кода тестового фреймворка (например, интеграционного теста).

Не ломайте сборку

Если у вас есть несколько зависимых CL, вам необходимо убедиться, что вся система продолжает работать корректно после отправки каждого из них. В противном случае вы можете сломать сборку для всех ваших коллег-разработчиков на несколько минут между вашими представлениями CL (или даже дольше, если что-то пойдет не так).

Ситуации, когда сложно сделать CL небольшим

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

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

Если все эти варианты не работают (что должно быть крайне редко), тогда заранее получите согласие ваших ревьюеров на просмотр большого CL и предупредите о предстоящем. Будьте готовы к тому, что процесс проверки потребует более длительного времени. Будьте бдительны, чтобы не допускать ошибок, и проявите внимательность при написании тестов.