Google: Navigating a CL in review

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

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

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

Навигация в коде при code review

Теперь вы знаете, на что нужно обращать внимание в code review.

А как стоит рассматривать CL, который распределен по нескольким файлам? Есть ли смысл этих изменений? Хорошее ли у них описание?

Шаг первый: общий взгляд на изменения в коде

Посмотрите описание CL и то, что CL делает в целом. Имеет ли это изменение смысл? Если это изменение нежелательно, то немедленно ответьте разработчику, пояснив, почему изменение нежелательно. В случае, когда вы отклоняете подобное изменение, предложите разработчику альтернативное изменение.

Например, вы можете сказать: «Похоже, вы проделали хорошую работу, спасибо! В то же время мы планируем удалить систему FooWidget, которую вы модифицируете здесь. Поэтому не стоит вносить в нее какие-либо новые изменения. Предлагаю вам реорганизовать наш новый класс BarWidget.»

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

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

Шаг второй: Изучите основные части CL

Найдите файл или файлы, которые являются «основной» частью этого CL. Часто существует один файл, имеющий наибольшее количество изменений, и как правило, это и есть основной фрагмент CL. Необходимо в первую очередь взглянуть на эти основные части. Это помогает понять контекст более мелких изменений в CL и ускоряет выполнение code review. Если CL слишком велик, то следует спросить разработчика, на что следует обратить внимание; или попросить его разделить CL на несколько частей.

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

Есть две основные причины, по которым так важно немедленно отправить эти основные комментарии к дизайну:

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

Шаг третий: просмотрите остальную часть CL в соответствующей последовательности

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