[Из песочницы] Как правильно делать код-ревью?
В жизни разработчика наступает момент, когда он начинает проводить код-ревью. Как правило, это один из признаков роста программиста: к его мнению начинают по-настоящему прислушиваться и доверять более серьезные задачи. Конкретный момент, когда разработчику предлагают заняться ревью зависит от структуры компании: есть команды, где это могут делать более-менее все по истечению некоторого времени работы, а есть такие, где процесс ревью целиком лежит на самых старших и опытных коллегах.
Вместе с тем, редко кто рассказывает о том, каких принципов необходимо придерживаться для проведения качественного ревью: каковы главные цели этой процедуры, за чем смотреть в первую очередь, насколько быстро его нужно проводить.
Всвязи с этим, «Руководство компании Google по проведению ревью» выглядит очень ценным документом, перевод первой части которого и представлен далее. Переводы остальных частей выйдут позже отдельными постами. Стоит отметить, что это адаптированный перевод, не все переведено слово-в-слово, во имя более русских формулировок и предложений.
Терминология:
CL: «changelist» — список изменений кода, отправленный в систему контроля версий на ревью. Аналог Pull Request в GitHub или Merge Request в GitLab.
Главная цель проведения ревью — улучшение состояния кодовой базы компании Google. Все инструменты и средства, используемые для проведения ревью, направлены именно на достижение этой цели.
На пути к достижению озвученной цели приходится идти на ряд компромиссов.
Во-первых, разработчики должны иметь возможность делать свои задачи. Если вы не принимаете никаких изменений в кодовую базу, то она никогда и не улучшится. В случае, когда ревьюер, делает любые изменения слишком сложными, разработчики просто не заходят ничего делать в будущем.
С другой стороны, именно ревьюер несет ответственность за качество изменений в CL и следит за тем, чтобы состояние кодовой базы со временем не деградировало. Это непростая задача, поскольку часто код проекта ухудшается посредством мелких изменений на протяжении некоторого периода времени. Это ощущается особенно остро, когда на команду давят сроки и качество в мелочах приносится в жертву.
Ревьюер несет ответственность за тот код, который он ревьюит. Он должен быть уверен, что кодовая база остается консистентной, поддерживаемой, и отвечает все другим принципам из «За чем необходимо следить в ревью».
Таким образом, мы приходим к следующему правилу, задающему стандарт проведения ревью:
Ревьюер должен принять CL тогда, когда он улучшает общую кодовую базу проекта, даже если CL не идеален.
Это самый главный принцип проведения ревью.
Конечно, есть некоторые оговорки: если CL добавляет в код фичи, которые ревьюер не не хочет видеть в проекте и не считает необходимыми, то он может не принимать изменения, даже если они улучшают кодовую базу.
Ключевым аспектом здесь является осознание того, что идеального кода не бывает: бывает код, который может быть лучше. Ревьюер не должен требовать совершенства в каждом отдельном небольшом кусочке CL перед тем, как принять изменения, он должен уметь находить баланс между улучшениями в CL как в коде, и важностью тех изменений, которые этот код несет. Вместо того, чтобы требовать совершенства, ревьюер должен стремиться к последовательному улучшению. CL, который улучшает поддерживаемость, читаемость и понятность кода не должен задерживаться в ревью на несколько дней или недель, по той причине, что он не идеален.
При этом ревьюер может свободно оставлять комментарии, поясняющие те моменты, которые могут быть сделаны лучше, однако, не имеют решающего значения. Помечайте такие комментарии определенным префиксом, например, «Nit:», чтобы автор знал что эта деталь не обязательна к выполнению и ее реализация остается на его усмотрение.
Замечание: данный документ не оправдывает изменений в CL, которые ухудшают состояние кодовой базы проекта. Единственным исключением являются экстренные случаи.
1.1. Менторство
Код-ревью может также нести образовательный смысл, обучая разработчикам новым знаниям о языке, фреймворке или общих принципах написания кода. Всегда хорошо, если вы оставляете комментарий, который учит разработчиков чему-то новому. Кроме того, общие знания позволяют улучшать кодовую базу. Помните о том, что если ваш комментарий несет только образовательный смысл и не содержит критичных требований (по описанным в данном документе стандартам), помечайте его «Nit:» или прямо пишите в комментарии что он не обязателен к выполнению.
1.2. Принципы
- Факты и данные всегда важнее мнений и персональных предпочтений.
- В вопросах стиля style guide является наивысшим авторитетом.
Все, что не отражено в style guide должно быть консистентным с другим кодом проекта. Если предыдущего кода нет, то стоит принять стиль автора. - Аспекты построения программ никогда не являются в чистом виде вопросами стиля написания или персональных предпочтений.
Они основаны на принятых в организации/проекте правилах и должны быть построены с учетом этих принципов, а не только личного мнения. Иногда существует несколько подходов к реализации одного и того же функционала.
В таком случае, если автор на основе данных или общепринятых инженерных практик может показать, что подходы одинаковы, то ревьюер должен принять выбор автора. В противном случае следует руководствоваться стандартными принципами выстраивания программ. - В ситуации, не регламентированной правилами, ревьюер может запросить от автора привести код к виду, консистентному с другим кодом проекта, если это не ухудшает общее состояние кодовой базы.
1.3. Разрешение конфиликтов
В случае возникновения конфликтных ситуаций, прежде всего, стоит постараться прийти к консенсусу, основываясь на правилах, описанных здесь: Руководство для авторов CL и здесь Руководство для ревьюеров.
Если прийти к согласию не удается, то ревьюеру и автору стоит назначить очную встречу или видеозвонок: это будет эффективнее войны в комментариях. В таком случае, постарайтесь зафиксировать результаты обсуждения в комментариях к CL, для будущих читателей.
Если ситуация все равно не разрешилась, то нужно попробовать эскалировать конфликт — вы можете обсудить вопрос командой, узнать мнение первоначального автора кода, в который вносятся изменения, или спросить совета у более опытных коллег. Не позволяйте CL «зависнуть» просто потому, что ревьюер и автор не могут прийти к согласию.
Замечание: Всегда учитывайте Стандарты код-ревью при рассмотрении каждого из принципов.
2.1. Дизайн (структура)
Самое важное, на что стоит обратить внимание в ревью — это дизайн CL в целом. Как взаимодействует код в рамках CL? Где находятся изменения: в общей кодовой базе или в вынесены в отдельную библиотеку? Насколько хорошо они интегрируются с остальными компонентами системы? Подходящее ли сейчас время для данных изменений?
2.2. Функциональность
Делает ли CL то, для чего он задуман? Насколько хорошо продуманы изменения для пользователей? При этом под «пользователем» понимается как конечный пользователь (если его затрагивают изменения), так и разработчики, которые будут использовать код в дальнейшем.
Предполагается, что разработчики достаточно хорошо тестируют свой код и на момент передачи в ревью он работает корректно. Вместе с тем, с точки зрения ревьюера, вы должны думать о граничных случаях, проблемах конкурентности, представлять себя на месте конечного пользователя, а также не забывать о багах, которые можно обнаружить просто читая код.
Существуют ситуации, когда может потребоваться дополнительная проверка CL: например, когда изменения касаются пользовательского интерфейса и через чтение кода сложно представить как это затронет конечного пользователя. В подобных ситуациях вы можете попросить разработчика сделать небольшое демо с новым функционалом, если запустить этот код самому представляется затруднительным.
Еще один случай, когда стоит отнестись к ревью более пристально — это наличие в коде CL параллельности в том или ином виде. Главные проблемы, которые может привнести параллельность — это дедлоки и гонки. Эти проблемы бывают очень трудноуловимыми, поэтому, необходимо чтобы и ревьюер и разработчик отнеслись к данному коду внимательнее. (Сложность ревью также является веской причиной избегать моделей конкурентности с возможными гонками и дедлоками).
2.3. Сложность
Проверяйте сложность кода на всех уровнях CL — в отдельных строках, функциях, классах. Слишком высокая сложность означает, что во-первых, нельзя быстро понять как работает код, во-вторых, при внесении изменений наверняка появятся баги.
Обычный случай, в результате чего код получается слишком сложным, это когда разработчики пишут слишком общий код или добавляют функционал, который сейчас не нужен в системе. Ревьюеры должны быть очень бдительны на предмет оверинжиниринга и должны убеждать разработчиков решать только те задачи, которые должны быть решены сейчас, и не касаться того, что, может быть и не придется делать в дальнейшем. Будущие проблемы должны решаться по мере поступления, в тот момент когда они принимают конкретные черты и получают ясные требования.
2.4. Тесты
Код, проходящий ревью, должен быть покрыт тестами: требуйте юнит, интеграционных или end-to-end тестов при проведении ревью. В общем случае, тесты должны быть добавлены в тот же CL, что и код. Исключениями являются
экстренные случаи.
Проверяйте правильность тестов — они не тестируют сами себя, а тесты на тесты пишутся крайне редко, обычно их проверяет человек.
Упадут ли тесты, если сломается код? Будут ли они работать правильно, если код изменится? Все ли тесты простые и проверяют то, что нужно проверять? Корректно ли разбиты проверки по методам?
Помните, что тесты — это тоже код, который нужно поддерживать. Не позволяйте тестам быть слишком сложными, просто потому, что они не входят в конечный релизный файл.
2.5. Наименования
Необходимо проверять, что разработчик выбрал подходящие наименования. Хорошее имя должно быть достаточно полным, чтобы понять, за что отвечает компонент, но вместе с тем, оставаться легко читаемым и не быть слишком длинным.
2.6. Комментарии
Насколько понятны и необходимы комментарии, оставленные разработчиком? Написаны ли они на понятном английском? Комментарии полезны, когда объясняют почему данный код существует, но излишни, если рассказывают о том, что делает код, хотя бывают и исключения: например, регулярные выражения и сложные алгоритмы. Если код сам по себе не является наглядным, то необходимо сделать его проще. Однако, чаще всего, комментарии должны пояснять то, чего не может сказать код: те решения, которые стоят за написанным.
Бывает полезным посмотреть комментарии, оставленные до CL: возможно, там есть «TODO», которое теперь можно убрать или совет про то, как должен быть сделан некоторый функционал.
Важно заметить, что комментарии — это не тоже самое что документация классов, модулей и функций. Документация нужна как раз для того, чтобы описать что делает код и как его использовать.
2.7. Стиль
В Google есть стайл гайды для всех основных и большинства вспомогательных языков программирования, которые мы используем. Убедитесь, что CL соответствует требованиям в соответствующих гайдах.
Если вы делаете замечание, которое касается стиля и этот момент никак не освещается в гайде, то помечайте такое требование как необязательное («Nit:»). CL не должен блокироваться на основании личных стилистических предпочтений.
Разработчик не должен мешать в один CL стилистические правки и исправление функционала. Это делает сложным для понимания, какой именно функционал изменился. Например, если разработчик хочет полностью переписать целый файл, поменяв его функционал и зарефакторив, то требуйте от него двух CL — первый с рефакторингом, а второй уже с функциональными изменениями.
2.8. Документация
Если CL меняет процесс взаимодействия пользователей с кодом, например, то, как теперь работают тесты или проходят релизы, следует также проверять соответствующие изменения в документации, включая README, g3doc (прим. — внутренний инструмент Google для хранения документации) и любые ссылки на сгенерированные документы. Если CL удаляет код, проверьте, что соответствующий раздел в документации также удален. Если документации нет, то запросите ее.
2.9. Каждая строчка
Проверяйте каждую строчку кода. Некоторые данные, такие как сгенерированный код или гигантская структура данных, можно читать по диагонали, но никогда не пролистывайте просто так код, написанный человеком. Конечно, что-то должно быть изучено пристальнее — вы должны сами провести для себя грань что именно и насколько глубоко. Помните, что вы всегда должны понимать что делает код.
Если вам сложно понять что происходит и ревью затягивается, то дайте знать об этом разработчику и, прежде чем продолжать ревью, дождитесь пояснений. В Google работают очень сильные разработчики и вы один из них. Если вы не понимаете, что написано, то, с большой долей вероятности, не поймут и другие. Таким образом, запрашивая пояснения, вы помогаете не только себе, но и вашим коллегам, которые столкнуться с этим кодом в дальнейшем.
Если вы понимаете что делает код, но не чувствуете себя достаточно компетентным для проведения ревью, убедитесь, что среди ревьюеров есть человек с соответствующей квалификацией по данному вопросу. Стоит особенно принципиально относится к этой проблеме, когда дело касается безопасности, доступности, многопоточности, локализации и тд.
2.10. Контекст
Обычно, инструменты для проведения ревью показывают только то, что поменял разработчик и небольшие фрагменты кода около изменений. Но, часто, необходимо полностью просмотреть целый файл чтобы понять контекст: предположим, вы видите, что были добавлены 4 строчки, однако, развернув файл вы понимаете что это строчки в методе из 50 строк и его теперь необходимо рефакторить.
Всегда полезно думать о CL в контексте системы в целом: улучшает ли данный CL код проекта или делает его менее понятным, хуже протестированным и тд?
Не принимайте CL, которые ухудшают состояние кодовой базы. Часто, проекты становятся слишком сложными именно по мере внесения в них мелких изменений, поэтому важно следить за сложностью в каждом отдельно взятом CL.
2.11. Позитивные моменты
Если вы видите в CL хорошо сделанную работу, то скажите об этом разработчику: ревьюеры должны не только обращать внимание на ошибки, но и поощрять за качественно написанный код. С точки зрения менторства намного более важно говорить о том, что сделано хорошо, чем заострять внимание на плохих моментах.
Итоги
Самое важное при проведении ревью:
- Код хорошо спроектирован,
- Функционал работает правильно,
- Изменения в пользовательском интерфейсе требуют особого внимания: помимо чистоты кода необходимо убедиться в том, что для пользователя изменения выглядят как задумано,
- Параллельно выполняемый код исполняется безопасно,
- Код не переусложнен,
- Нет надуманного и ненужного на данный момент функционала,
- На код написаны тесты
- Тесты хорошо спроектированы,
- Использованы корректные имена для файлов, классов, методов, переменных и тд
- Оставлены понятные и нужные комментарии, по большей части поясняющие причины решений, а не суть (суть должна быть понятна из самого кода),
- Код задокументирован (g3doc),
- Код соответствует нашим стайл-гайдам.
Вы должны просмотреть каждую строчку кода, брать во внимание контекст, быть уверенным в том, что улучшаете состояние кодовой базы и поощрять удачные решения разработчика.
В следующей части будет рассмотрено, как лучше ориентироваться в CL и с чего начинать ревью.
