что означает ревью макета
Код ревью: как быть хорошим автором
Привет! Меня зовут Сергей Загурский, я работаю в Joom в команде инфраструктуры. В своей практике ревьюера кода я регулярно сталкиваюсь с тем, что автор не понимает, что ревьюер не является волшебным чёрным ящиком, в который можно закинуть любые изменения и получить по ним обратную связь. Ревьюер, как и автор, будучи человеком, обладает рядом слабостей. И автор должен (если, конечно, он заинтересован в качественном ревью), помочь ревьюеру настолько, насколько это возможно.
Хочу рассказать, как автор кода может упростить работу ревьюеру и увеличить тем самым как качество ревью, так и производительность ревьюера. Эта статья вполне может использоваться в качестве в вашей внутрикорпоративной документации как руководство для подготовки изменений к ревью. Она, собственно, и была скомпилирована из такого руководства.
Зачем мы делаем ревью кода
Вы наверняка и без меня это знаете. Поэтому расскажу о самых основах, не углубляясь в подробности.
Ревью кода замедляет разработку. Это главный аргумент, который вы услышите от противников ревью. И это правда, но не вся. Ревью действительно замедлит вашу разработку. Изменения действительно будут дольше проходить весь путь от клавиатуры до продакшена. Однако, замедляя разработку «здесь и сейчас», ревью позволяет сделать кривую сложности изменения системы более пологой. Я не претендую на научность, но проиллюстрировал это на графике выше.
Он показывает концепцию: качественный процесс ревью уменьшает темп роста сложности. И это — главная ценность ревью кода. Это та ценность, которая понятна бизнесу, т. к. речь идёт о скорости разработки, которая напрямую уменьшает стоимость разработки и увеличивает конкурентоспособность. Но если вдруг планируется, что ваш проект не должен надолго пережить точку пересечения графиков, то ревью можно и не вводить.
На примитивном уровне ревью кода также является моделью того, как с написанным кодом будет идти взаимодействие в дальнейшем. Если у ревьюера не возникает никаких проблем с пониманием, значит и при чтении или отладке кода в будущем также сложностей не будет. И наоборот, если ревьюер не может ничего понять, то это write only код. Его развитие и отладка будут стоить дороже.
На более высоком уровне, ревью хранит в целостности архитектуру вашей системы. Ревью сопротивляется неоправданному внесению каких-то противоречащих системе элементов (также известных как костыли). Ревью способствует единообразию подходов к решению задач, что также уменьшает сложность проекта.
Дальше мы сосредоточимся на самой фундаментальной составляющей процесса ревью — на понимании кода.
Как ревьюер делает ревью
Чтобы процесс ревью являлся таковым, ревьюеру требуется, не больше и не меньше, понять суть изменений. Это основа процесса, без которой ревью не имеет никакого смысла. Для понимания требуется время. И это не то количество времени, которое можно потратить в перерыве, «за кофейком». Для понимания требуется время, сопоставимое с тем временем, которое было затрачено на написание кода. Иногда даже больше. Ревью является полноценной работой, наряду с написанием кода. В чём-то даже сложнее.
Ревьюер часто бывает не настолько погружен в контекст, как автор. Это тоже усложняет понимание сути изменений. Ревьюер может первый раз в жизни видеть код, в котором производятся изменения. Ревьюер может ничего не знать о задаче, которую решает автор. Ревьюер может не понимать, зачем всё это делается. Своевременное предоставление ревьюеру информации о контексте экономит время ревьюера и улучшает качество ревью.
Если речь идёт о каких-то нетривиальных изменениях, то в голове ревьюера происходит тот же процесс анализа, что и в голове автора. И если в ходе этого анализа могут возникать разные решения, не имеющие друг перед другом очевидных преимуществ, то ревьюеру придётся потратить время, чтобы понять, почему автор остановился на определённом решении. Автор, в итоге, мог принять единственно верное решение, но ревьюер, в условиях отсутствия обоснования от автора, потратит своё время на это обоснование.
Бывает такое, что автор вместе с основными изменениями делает что-то, не связанное с решением своей задачи. Когда ревьюер сталкивается с таким, то он тратит дополнительное время, чтобы понять, как эти изменения связаны с решением основной задачи.
Ещё одним большим пожирателем времени ревьюера является написание замечаний. Да, есть замечания, которые относятся к каким-то, условно, косметическим аспектам. Однако даже их бывает полезно снабдить сопровождающей информацией вроде ссылки на нужную часть документа по стилю кода. И это всё требует времени.
Также нужно помнить, что ревьюер, будучи человеком, склонен откладывать начало ревью на потом, если видит ревью повышенной сложности. Вежливый ревьюер может постесняться потребовать разбить большой Pull Request на несколько и будет платить за это своим временем и качеством ревью.
Как помочь ревьюеру провести качественное ревью
Главная задача автора, который болеет за качество процесса ревью, состоит в том, чтобы максимально упростить и ускорить работу ревьюера. Автор, который без дополнительной обработки вываливает на ревьюера результат своей работы, проявляет крайнюю степень неуважения к работе ревьюера.
Перед тем, как превратить свои изменения в Pull Request, следует разбить их на логические куски, если в этом есть необходимость. Комфортный объём ревью заканчивается примерно на 500 строках кода изменений. Допустимый — примерно на 1000 строках. Всё, что за пределами 1000 строк, должно быть разбито на более мелкие Pull Request’ы.
Если вы, как и я, не разбиваете свои изменения на куски комфортного размера заранее, то это придётся проделать перед отправкой ваших изменений на ревью. Отмазка, что на это нужно потратить время, не катит. Принимая решение об отправке на ревью 1000+ строк кода, вы, фактически, оцениваете своё время дороже времени ревьюера. По нашим правилам у ревьюера всегда есть право потребовать разбить изменения на более мелкие куски. Мы всегда просим коллег относиться с пониманием, если он этим воспользуется. С опытом становится проще строить свою работу так, чтобы у вас не появлялись гигантские Pull Request’ы, в которых «ничего нельзя отделить».
Отдельно стоит упомянуть изменения, внесённые в код с помощью авторефакторинга или sed’а. Объём таких изменений может быть очень большим. Наличие автоматических изменений вместе с осмысленными изменениями усложняет ревью. Всегда отделяйте авторефакторинги в отдельные Pull Request’ы, если их объём сопоставим с объёмом нового кода, написанного вами.
Следующая часть подготовки состоит в информировании ревьюера о сути изменений. Как я уже писал выше, ревьюеру нужно знать: что сделано, зачем сделано, как сделано, и почему именно так сделано. Часть информации у ревьюера уже может быть. Часть — есть в коде. Всё, что не очевидно из кода, должно быть прописано в описании изменений.
Если задача подразумевала какой-то нетривиальный анализ, то автор должен быть готов, что этот же анализ будет проводить и ревьюер. И ревьюер может прийти к другому выводу. Часто бывает так, что автор в процессе решения задачи проходил через этап «более простого» решения и отверг его в процессе по какой-то причине. Для экономии времени ревьюера стоит включить обоснование тех или иных решений в описание изменений. Это обоснование останется в проекте, позволив более уверенно менять решение, если соображения, которыми руководствовался автор при принятии решения, утратили актуальность.
Случаются ситуации, когда у ревьюера может быть своё сильное мнение о том, как должна быть решена задача. В этом случае автору следует заранее обсудить это с ревьюером, чтобы исключить крайне некомфортную для всех сторон ситуацию, когда ревьюер потребует переделать значительную часть изменений. Возможно даже придётся встретиться и поговорить с ревьюером, чтобы обосновать своё решение и убедить ревьюера или принять его решение.
Итак, автор подготовил изменения к ревью. Пора отправлять ревьюеру? Нет! Тут наступает ещё один важный этап, саморевью. Под саморевью я понимаю именно ревью, осуществлённое самим автором. Не просто беглый визуальный контроль. Нужно постараться провести полноценное ревью. Ответственно проведённое саморевью сэкономит уйму времени ревьюеру. Наверняка в коде есть какие-то остатки отладочных конструкций, закомментированного кода, TODO-комментариев, бессмысленных изменений форматирования, не пригодившегося кода и прочего подобного. Взгляд глазами ревьюера также поможет оценить сложность ревью и даст возможность добавить дополнительной информации к ревью, которая была упущена при подготовке. Пренебрежение автором саморевью я считаю проявлением крайнего неуважения к ревьюеру.
В целом, я считаю допустимым потратить порядка 10% от времени, затраченного на написание кода, на подготовку к ревью. Это время автора, которое мы обменяем на экономию времени ревьюера, и на улучшение качества ревью. Следует помнить, что ревьюеру на качественное ревью легко может потребоваться и 20%, и 50% от времени, затраченного автором на написание кода.
Вот только теперь автор может с чистой совестью отправить изменения ревьюеру.
Дальше начинается жизненный цикл Pull Request’а. Ревьюер должен либо одобрить его, либо попросить внести изменения. Чтобы упростить работу ревьюера, автору стоит добавить комментарий к каждому запрошенному изменению. Это вполне может быть краткое «OK» или «Исправил», если ничего другого не требуется. Убедитесь, что вы понимаете, что просит ревьюер и что вам понятна его аргументация. Не стоит безоговорочно принимать любые запросы на внесение изменений, возводя тем самым ревьюера в околобожественный ранг. Ревью — это обоюдный процесс. Если ревьюеру что-то не понятно, то он спрашивает об этом автора, и наоборот. В случае, если автор не очень опытен, ревьюеру следует приложить особенные усилия к описанию запросов на изменения, чтобы воспользоваться возможностью поделиться с автором своим опытом. Бывают и спорные моменты, когда аргументация недостаточно сильная, чтобы склонить обе стороны к единой точке зрения. С учётом того, что автор находится в более уязвимой позиции, считаю, что при прочих равных преимущество должно оставаться за автором.
Не вносите изменений в свой Pull Request, не относящихся к тому, что попросил вас ревьюер. Это крайне сбивает с толку. Также следует воздержаться от rebase и подобных действий.
Получили одобрение от ревьюера? Отлично, ещё одно качественно написанное и оформленное изменение было добавлено в проект!
Как правильно делать код-ревью?
В жизни разработчика наступает момент, когда он начинает проводить код-ревью. Как правило, это один из признаков роста программиста: к его мнению начинают по-настоящему прислушиваться и доверять более серьезные задачи. Конкретный момент, когда разработчику предлагают заняться ревью зависит от структуры компании: есть команды, где это могут делать более-менее все по истечению некоторого времени работы, а есть такие, где процесс ревью целиком лежит на самых старших и опытных коллегах.
Вместе с тем, редко кто рассказывает о том, каких принципов необходимо придерживаться для проведения качественного ревью: каковы главные цели этой процедуры, за чем смотреть в первую очередь, насколько быстро его нужно проводить.
Всвязи с этим, «Руководство компании Google по проведению ревью» выглядит очень ценным документом, перевод первой части которого и представлен далее. Переводы остальных частей выйдут позже отдельными постами. Стоит отметить, что это адаптированный перевод, не все переведено слово-в-слово, во имя более русских формулировок и предложений.
CL: «changelist» — список изменений кода, отправленный в систему контроля версий на ревью. Аналог Pull Request в GitHub или Merge Request в GitLab.
1. Стандарты код-ревью
Главная цель проведения ревью — улучшение состояния кодовой базы компании Google. Все инструменты и средства, используемые для проведения ревью, направлены именно на достижение этой цели.
На пути к достижению озвученной цели приходится идти на ряд компромиссов.
Во-первых, разработчики должны иметь возможность делать свои задачи. Если вы не принимаете никаких изменений в кодовую базу, то она никогда и не улучшится. В случае, когда ревьюер, делает любые изменения слишком сложными, разработчики просто не заходят ничего делать в будущем.
С другой стороны, именно ревьюер несет ответственность за качество изменений в CL и следит за тем, чтобы состояние кодовой базы со временем не деградировало. Это непростая задача, поскольку часто код проекта ухудшается посредством мелких изменений на протяжении некоторого периода времени. Это ощущается особенно остро, когда на команду давят сроки и качество в мелочах приносится в жертву.
Ревьюер несет ответственность за тот код, который он ревьюит. Он должен быть уверен, что кодовая база остается консистентной, поддерживаемой, и отвечает все другим принципам из «За чем необходимо следить в ревью».
Таким образом, мы приходим к следующему правилу, задающему стандарт проведения ревью:
Ревьюер должен принять CL тогда, когда он улучшает общую кодовую базу проекта, даже если CL не идеален.
Это самый главный принцип проведения ревью.
Конечно, есть некоторые оговорки: если CL добавляет в код фичи, которые ревьюер не хочет видеть в проекте и не считает необходимыми, то он может не принимать изменения, даже если они улучшают кодовую базу.
Ключевым аспектом здесь является осознание того, что идеального кода не бывает: бывает код, который может быть лучше. Ревьюер не должен требовать совершенства в каждом отдельном небольшом кусочке CL перед тем, как принять изменения, он должен уметь находить баланс между улучшениями в CL как в коде, и важностью тех изменений, которые этот код несет. Вместо того, чтобы требовать совершенства, ревьюер должен стремиться к последовательному улучшению. CL, который улучшает поддерживаемость, читаемость и понятность кода не должен задерживаться в ревью на несколько дней или недель, по той причине, что он не идеален.
При этом ревьюер может свободно оставлять комментарии, поясняющие те моменты, которые могут быть сделаны лучше, однако, не имеют решающего значения. Помечайте такие комментарии определенным префиксом, например, «Nit: «, чтобы автор знал что эта деталь не обязательна к выполнению и ее реализация остается на его усмотрение.
Замечание: данный документ не оправдывает изменений в CL, которые ухудшают состояние кодовой базы проекта. Единственным исключением являются экстренные случаи.
1.1. Менторство
Код-ревью может также нести образовательный смысл, обучая разработчиков новым знаниям о языке, фреймворке или общих принципах написания кода. Всегда хорошо, если вы оставляете комментарий, который учит разработчиков чему-то новому. Кроме того, общие знания позволяют улучшать кодовую базу. Помните о том, что если ваш комментарий несет только образовательный смысл и не содержит критичных требований (по описанным в данном документе стандартам), помечайте его «Nit: » или прямо пишите в комментарии что он не обязателен к выполнению.
1.2. Принципы
1.3. Разрешение конфиликтов
В случае возникновения конфликтных ситуаций, прежде всего, стоит постараться прийти к консенсусу, основываясь на правилах, описанных здесь: Руководство для авторов CL и здесь Руководство для ревьюеров.
Если прийти к согласию не удается, то ревьюеру и автору стоит назначить очную встречу или видеозвонок: это будет эффективнее войны в комментариях. В таком случае, постарайтесь зафиксировать результаты обсуждения в комментариях к CL, для будущих читателей.
Если ситуация все равно не разрешилась, то нужно попробовать эскалировать конфликт — вы можете обсудить вопрос командой, узнать мнение первоначального автора кода, в который вносятся изменения, или спросить совета у более опытных коллег. Не позволяйте CL «зависнуть» просто потому, что ревьюер и автор не могут прийти к согласию.
2. На что обращать внимание в ревью
Замечание: Всегда учитывайте Стандарты код-ревью при рассмотрении каждого из принципов.
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 хорошо сделанную работу, то скажите об этом разработчику: ревьюеры должны не только обращать внимание на ошибки, но и поощрять за качественно написанный код. С точки зрения менторства намного более важно говорить о том, что сделано хорошо, чем заострять внимание на плохих моментах.
Итоги
Самое важное при проведении ревью:
Вы должны просмотреть каждую строчку кода, брать во внимание контекст, быть уверенным в том, что улучшаете состояние кодовой базы и поощрять удачные решения разработчика.
В следующей части будет рассмотрено, как лучше ориентироваться в CL и с чего начинать ревью.
Как проводить код-ревью
Из документации Google’s Engineering Practices
В этом руководстве приведены рекомендации по оптимальному проведению код-ревью, основанные на многолетнем опыте. Все вместе они составляют один документ, разбитый на множество разделов. Необязательно читать их все, но часто для себя и команды лучше изучить руководство полностью.
Стандарт код-ревью
Основная цель код-ревью заключается в том, чтобы гарантировать постоянное улучшение кодовой базы Google. Все инструменты и процессы посвящены этой цели.
Здесь необходим ряд компромиссов.
Во-первых, разработчики должны быть в состоянии успешно решать свои задачи. Если вы никогда не отправляете код, то и кодовая база никогда не улучшится. Кроме того, если рецензент сильно затрудняет любую работу, то в будущем разработчики не заинтересованы предлагать улучшения.
С другой стороны, обязанность рецензента убедиться, что качество CL не снизит общее качество кодовой базы со временем. Это может быть сложно, потому что часто деградация происходит из-за небольшого снижения качества кода со временем, особенно если команда находится под сильным давлением сроков и чувствует, что имеет право на увеличение технического долга.
Кроме того, рецензент несёт ответственность за рецензируемый код. Он хочет убедиться, что кодовая база остаётся последовательной, поддерживаемой и соответствует всему остальному, что упомянуто в разделе «Что проверять в коде».
Таким образом, мы получаем следующее правило в качестве стандарта для код-ревью:
Обычно рецензенты должны одобрить CL, как только он достигает состояния, когда определённо улучшает общее качество кода системы, даже если CL не идеален.
Это главный среди всех принципов код-ревью.
Конечно, у него есть ограничения. Например, если CL добавляет функцию, которую рецензент не хочет видеть в системе, то рецензент, безусловно, может отказать в коммите, даже если код хорошего качества.
Ключевым моментом здесь является то, что не бывает «идеального» кода — бывает только код получше. Рецензент не должен требовать от автора полировать каждый крошечный фрагментик. Скорее, рецензент должен сбалансировать необходимость дальнейшего прогресса по сравнению с важностью предлагаемых изменений. Вместо того, чтобы стремиться к идеалу, рецензент должен стремиться к непрерывному улучшению. Коммит, который в целом улучшает ремонтопригодность, читаемость и понятность системы, нельзя задерживать на дни или недели, потому что он не «идеален».
Рецензенты всегда могут оставлять любые комментарии по улучшению кода, но не очень важные изменения следует помечать, например, префиксом Nit:, чтобы автор знал, что это просто точка зрения, которую он может игнорировать.
Примечание. Ничто в этом документе не оправдывает CL, которые определённо ухудшают общее качество кода системы. Такое возможно только в чрезвычайной ситуации.
Менторинг
Код-ревью может быть важно ещё и для обучения разработчиков чему-то новому о языке, структуре или общих принципах проектирования ПО. Всегда приятно оставлять комментарии, которые помогают разработчику узнать что-то новое. Обмен знаниями вносит свой вклад в улучшение кода системы с течением времени. Просто имейте в виду, что если оставляете чисто образовательный комментарий, не критичный для соответствия описанным здесь стандартам, добавьте к нему префикс Nit: или иным образом укажите, что автор не обязан его разрешать.
Принципы
Разрешение конфликтов
В любом конфликте первым шагом всегда должно быть стремление разработчика и рецензента прийти к консенсусу, основанному на содержании этого документа и других документов в Руководстве автора CL и этом Руководстве рецензента.
Когда прийти к консенсусу особенно трудно, может помочь личная встреча или видеоконференция между рецензентом и автором (если вы это сделаете, обязательно запишите результаты обсуждения в комментарии к коммиту для будущих читателей).
Если это не решит ситуацию, то наиболее распространённый способ — эскалация. Часто она заключается в более широком обсуждении с командой, привлечении тимлида, обращении к мейнтейнеру или к менеджеру по разработке. Не позволяйте коммиту задерживаться из-за того, что автор и рецензент не могут прийти к соглашению.
Что проверять в коде
Примечание. При рассмотрении каждого из этих пунктов обязательно учитывайте Стандарт код-ревью.
Дизайн
Самое главное — учесть в код-ревью общий проект (дизайн). Имеют ли смысл взаимодействия разных частей кода? Это изменение относится к вашей кодовой базе или к библиотеке? Хорошо ли CL интегрируется с остальной частью системы? Время ли сейчас добавлять эту функциональность?
Функциональность
Делает ли этот CL то, что задумал разработчик? Хорошо ли оно для пользователей этого кода? Под «пользователями» подразумеваются и конечные пользователи (если их затрагивает изменение), и разработчики (которым придётся «использовать» этот код в будущем).
В основном, мы ожидаем, что ещё до коммита разработчики протестируют свой код достаточно хорошо, чтобы он правильно работал. Но как рецензент вы всё равно должны думать о крайних случаях, искать проблемы параллелизма, пытаться думать как пользователь и даже при чтении кода смотреть, что нет очевидных ошибок.
Если хотите, то можете проверить работоспособность. Наиболее важно сделать это, если код оказывает влияние на пользователей, например, изменение UI. Трудно понять, как некоторые изменения повлияют на пользователей, когда вы просто читаете код. Для таких изменений можете попросить разработчика предоставить демо, если вам слишком сложно углубляться в код и испытать его самостоятельно.
Ещё один момент, когда во время код-ревью особенно важно подумать о функциональности, — это если в CL происходит какое-то параллельное программирование, которое теоретически может вызвать взаимоблокировки или условия гонки. Такие проблемы очень трудно обнаружить, просто запустив код; обычно нужно, чтобы кто-то (и разработчик, и рецензент) тщательно продумали их и убедились, что проблемы не вводятся (обратите внимание, что это также хорошая причина не использовать модели параллелизма, где возможны условия гонки или взаимоблокировки, — это может сделать код очень сложным для понимания или код-ревью).
Сложность
Является ли CL более сложным, чем должен быть? Проверьте это на каждом уровне: отдельные строки, функции, классы. «Излишняя сложность» обычно означает невозможность быстрого понимания при чтении. Это также может означать, что разработчики, скорее всего, будут вводить ошибки при попытке вызвать или изменить этот код.
Особый тип сложности — это оверинжиниринг, когда разработчики сделали код более универсальным, чем он должен быть, или добавили функциональность, которая в настоящее время не нужна системе. Рецензентам следует быть особенно бдительными в отношении оверинжиниринга. Поощряйте разработчиков решать проблему, которая точно должна быть решена сейчас, а не проблему, которую, возможно, потребуется решить в будущем. Будущую проблему следует решать тогда, когда она появится, и вы можете увидеть её фактическую форму и требования в физической Вселенной.
Тесты
Запросите модульные, интеграционные или сквозные тесты, соответствующие изменению. В общем случае тесты следует добавить в тот же CL, что и производственный код, если CL не обрабатывает чрезвычайную ситуацию.
Убедитесь, что тесты правильны, разумны и полезны. Тесты не проверяют сами себя, и мы редко пишем тесты для наших тестов — человек должен сам убедиться, что тесты валидны.
Действительно ли тесты не проходят на сломанном коде? Если код изменится, не появятся ли ложные срабатывания? Делает ли каждый тест простые и полезные утверждения? Правильно ли тесты разделены между различными методами тестирования?
Помните, что тесты — код, который тоже придётся поддерживать. Не допускайте в них сложности только потому, что это не часть основного двоичного файла.
Именование
Разработчик везде выбрал хорошие имена? Хорошее имя достаточно длинное, чтобы полностью передать то, чем является или что делает элемент, не будучи настолько длинным, что становится трудно читать.
Комментарии
Написал ли разработчик чёткие комментарии на понятном языке? Действительно ли все комментарии необходимы? Обычно комментарии полезны, когда они объясняют, почему какой-то код существует, и не должны объяснять, что делает этот код. Если код недостаточно ясен, чтобы объяснить себя, то подлежит упрощению. Есть некоторые исключения (например, комментарии с объяснением действия кода часто бывают очень полезны для регулярных выражений и сложных алгоритмов), но в основном комментарии предназначены для информации, которую не может содержать сам код, например, обоснования решения.
Также может быть полезно посмотреть на комментарии в предыдущем коде. Возможно, есть TODO, который сейчас можно удалить, или комментарий, который не советует вводить это изменение, и т. д.
Обратите внимание, что комментарии отличаются от документации по классам, модулям или функциям, которая описывает задачу кода, как он должен использоваться и как он себя ведёт в работе.
Стиль
У нас в Google есть руководства по стилю для всех основных языков, и даже для большинства второстепенных. Убедитесь, что CL не противоречит соответствующим руководствам по стилю.
Если хотите улучшить некоторые элементы, которых нет в руководстве по стилю, добавьте к комментарию соответствующее примечание (Nit:). Разработчик будет знать, что это ваша личная ремарка, которая не является обязательной к исполнению. Не блокируйте отправку коммита только на основе личных предпочтений.
Автор не должен совмещать значительные изменения стиля с другими изменениями. Это затрудняет просмотр изменений в CL, усложняет слияния, откаты кода и вызывает другие проблемы. Например, если автор хочет переформатировать весь файл, попросите оформить смену стиля в одном CL, а после этого отправить CL с функциональными изменениями.
Документация
Если выход CL изменяет что-то в сборке, тестировании, процедурах взаимодействия или выпуска кода, проверьте факт обновления соответствующей документации, в том числе файлов README, страниц g3doc и всех генерируемых справочных документов. Если CL удаляет код или делает его устаревшим, подумайте, следует ли также удалить документацию. Если документация отсутствует, запросите её создание.
Каждая строка
Просмотрите каждую строку в коде. Хотя какие-нибудь файлы данных, сгенерированный код или большие структуры данных можно просмотреть мельком, но внимательно прочитайте каждый написанный человеком класс, функцию или блок кода, никогда не предполагайте по умолчанию, что там всё в порядке. Очевидно, что какой-то код заслуживает более тщательного изучения, чем другой — вы решаете сами, но вы должны по крайней мере быть уверены, что понимаете работу всего кода.
Если вам слишком сложно читать код и это замедляет ревью, то следует сообщить разработчику и подождать, пока он внесёт ясность, прежде чем продолжить работу. В Google мы нанимаем замечательных инженеров-программистов, и вы один из них. Если вы не можете понять код, очень вероятно, что другие разработчики тоже не смогут. Таким образом, вы также помогаете будущим разработчикам понять этот код, когда просите разработчика внести ясность.
Если код понятен, но вы не чувствуете в себе достаточной квалификации для оценки какого-то фрагмента, убедитесь, что в CL есть рецензент, который квалифицирован, особенно для сложных проблем, таких как безопасность, параллелизм, доступность, интернационализация и т. д.
Контекст
Часто бывает полезно взглянуть на CL в широком контексте. Обычно инструмент код-ревью показывает только несколько строк возле места изменения. Иногда нужно посмотреть на весь файл, чтобы убедиться, что изменение действительно имеет смысл. Например, вы видите добавление всего четырёх строк, но если посмотреть на весь файл, то эти четыре строки находятся в 50-строчном методе, который теперь действительно придётся разбить на более мелкие.
Также полезно думать о CL в контексте системы в целом. Улучшает ли он качество кода системы или делает её более сложной, менее протестированной и т. д.? Не принимайте коммит, который снижает качество кода системы. Большинство систем усложняются в результате суммы многих небольших изменений, поэтому важно предотвратить там даже небольшие сложности.
Хорошее
Если видите что-то хорошее в коммите, сообщите разработчику, особенно когда он в лучшем виде решил проблему, изложенную в одном из ваших комментариев. Код-ревью часто просто фокусируются на ошибках, но они также должны поощрять и ценить хорошие практики. С точки зрения менторинга иногда даже более важно сказать разработчику, что именно он сделал правильно, а не где ошибся.
Резюме
При выполнении код-ревью следует убедиться, что:
Навигация по CL в код-ревью
Резюме
Теперь, когда вы знаете, что проверять в коде, каков наиболее эффективный способ проведения код-ревью на нескольких файлах?
Шаг первый: окиньте взглядом весь коммит целиком
Посмотрите на описание CL и на то, что он делает в целом. Имеет ли это изменение вообще смысл? Если его изначально не следовало писать, пожалуйста, немедленно ответьте с объяснением, почему оно не нужно. Когда отклоняете такое изменение, также неплохо предложить разработчику, что сделать вместо него.
Например, вы можете сказать «Похоже, вы проделали хорошую работу, спасибо! Тем не менее, мы на самом деле собираемся удалять систему FooWidget, поэтому не хотим вносить какие-то новые изменения прямо сейчас. Как насчёт того, чтобы вместо этого вы провели рефакторинг нашего нового класса BarWidget?»
Обратите внимание, что рецензент не только отклонил текущий CL и предоставил альтернативное предложение, но и сделал это вежливо. Такая вежливость важна, потому что мы хотим показать, что уважаем друг друга как разработчики, даже когда не согласны друг с другом.
Если вы получаете довольно много нежелательных CL, то следует пересмотреть процесс разработки в вашей команде или опубликованные правила для внешних контрибуторов, чтобы улучшить коммуникацию при написании CL. Лучше сказать людям «нет» раньше, чем они проделают тонну работы, которую придётся выбросить или сильно переписать.
Шаг второй: изучите основные части CL
Найдите файл или файлы, которые представляют «основную» часть этого CL. Часто существует один файл с наибольшим количество логических изменений, и это основная часть CL. Сначала посмотрите на эти основные части. Это помогает понять контекст меньших частей CL и, как правило, ускоряет выполнение код-ревью. Если CL слишком велик, спросите разработчика, что посмотреть сначала, или попросите его разделить CL на части.
Если видите серьёзные проблемы в главной части CL, вы должны немедленно отправить эти комментарии, даже если у вас нет времени, чтобы просмотреть остальную часть CL прямо сейчас. Фактически, просмотр остальной части может оказаться пустой тратой времени, потому что если проблемы достаточно значительны, многие другие рассматриваемые фрагменты кода исчезнут и не будут иметь значения.
Есть две основные причины, по которым так важно немедленно отправить эти основные комментарии:
Шаг третий: просмотрите остальную часть CL в соответствующей последовательности
Убедившись в отсутствии серьёзных проблем с дизайном CL в целом, попробуйте выяснить логическую последовательность для просмотра файлов и убедитесь, что ничего не пропустите. Обычно когда вы просмотрели основные файлы, проще всего просто пройти все остальные в том порядке, в котором их представляет инструмент код-ревью. Также иногда полезно сначала прочитать тесты, а потом основной код, потому что тогда у вас будет представление о том, в чём смысл изменения.
Скорость код-ревью
Почему код-ревью следует делать быстро?
В Google мы оптимизируем скорость совместной работы, а не скорость отдельных разработчиков. Скорость индивидуальной работы важна, просто она не так приоритетна, по сравнению со скоростью командной работы.
При медленном просмотре кода происходит несколько вещей:
Как быстро выполнять код-ревью?
Если вы не в середине сосредоточенной задачи, то должны сделать код-ревью вскоре после его поступления.
Один рабочий день — максимальное время для ответа (т. е. это первое дело на следующее утро).
Следование этим рекомендациям означает, что типичный CL должен получить несколько раундов ревью (если это необходимо) в течение одного дня.
Скорость и отвлечения
Есть один момент, когда приоритет личной скорости превосходит командный. Если вы в середине сосредоточенной задачи, такой как написание кода, не отвлекайтесь на код-ревью. Исследования показали, что у разработчика может занять много времени, чтобы вернуться в плавный поток разработки после отвлечения. Таким образом, отвлечение от кодирования на самом деле обходится команде дороже, чем просьба к другому разработчику немного подождать код-ревью.
Вместо этого дождитесь точки останова в своей работе. Например, после завершения текущей задачи, после обеда, возвращаясь со встречи, возвращаясь с офисной мини-кухни и т. д.
Быстрые ответы
Когда мы говорим о скорости код-ревью, нас интересует время ответа, а не то, сколько времени требуется на весь процесс до конца. В идеале, весь процесс тоже должен быть быстрым, но даже важнее, чтобы быстро приходили отдельные ответы, чем весь процесс.
Даже если весь процесс код-ревью требует много времени, наличие быстрых ответов от рецензента на протяжении всего процесса значительно облегчает жизнь разработчикам, которые могут испытывать раздражение от «медленных» ответов.
Если вы слишком заняты для полного ревью сразу после получения запроса, то всё равно можете быстро ответить с сообщением о сроках или предложить ревью другим рецензентам, которые могут ответить быстрее, или предоставить некоторые первоначальные общие комментарии (примечание: ничто из этого не означает, что вы должны прервать кодирование даже для отправки такого ответа — отправьте ответ во время разумной точки останова в своей работе).
Важно, чтобы рецензенты тратили достаточно времени на обзор и были уверены, что их «LGTM» точно означает «этот код соответствует нашим стандартам». Тем не менее, отдельные ответы всё равно в идеале должны быть быстрыми.
Код-ревью между часовыми поясами
При работе между разными часовыми поясами попробуйте ответить автору, пока он ещё в офисе. Если он уже ушёл домой, то обязательно постарайтесь отправить ответ прежде, чем он вернётся в офис на следующий день.
LGTM с оговорками
Ради повышения скорости существуют определённые ситуации, в которых рецензент должен дать LGTM/одобрение, даже в случае неотвеченных комментариев к CL. Это делается в случае, если:
LGTM с оговорками особенно полезны, когда разработчик и рецензент находятся в разных часовых поясах, а в противном случае разработчик будет ждать целый день, чтобы получить «LGTM, одобрено».
Большие CL
Если кто-то присылает вам код-ревью настолько большой, что вы не уверены, когда сможете его просмотреть, то типичный ответ должен состоять в том, чтобы попросить разработчика разделить CL на несколько меньших CL. Обычно это возможно и очень полезно для рецензентов, даже если требует дополнительной работы от разработчика.
Если CL нельзя разбить на более мелкие CL и у вас нет времени быстро просмотреть всё это, по крайней мере, напишите несколько комментариев к общему дизайну CL и отправьте их обратно разработчику для улучшения. Одна из ваших целей как рецензента должна заключаться в том, чтобы всегда «разблокировать» разработчика или позволить ему быстро предпринять какие-либо дальнейшие действия, не жертвуя качеством кода.
Улучшения код-ревью со временем
Если будете следовать этим рекомендациям и строго подходить к код-ревью, то обнаружите, что весь процесс со временем ускоряется и ускоряется. Разработчики узнают, что требуется для качественного кода, и отправляют вам CL, которые с самого начала отлично подходят, требуя все меньше и меньше времени на просмотр. Рецензенты учатся быстро реагировать и не добавлять ненужные задержки в процесс рецензирования. Но не компрометируйте стандарты обзора кода или качество для воображаемого улучшения скорости — так на самом деле вы не добьётесь общего ускорения в долгосрочной перспективе.
Чрезвычайные ситуации
Есть ещё чрезвычайные ситуации, когда CL должны очень быстро пройти через весь процесс код-ревью, и где придётся смягчить принципы качества. Но пожалуйста, ознакомьтесь с описанием, какие ситуации квалифицируются как чрезвычайные, а какие нет.
Как писать комментарии в код-ревью
Резюме
Вежливость
В целом, важно быть вежливым и уважительным, а также очень ясным и полезным для разработчика, чей код вы просматриваете. Один из способов сделать это — убедиться, что вы всегда делаете комментарии о коде и никогда о разработчике. Не всегда нужно следовать этой практике, но вы обязательно должны использовать её, когда говорите что-то неприятное или спорное. Например:
Плохо: «Почему вы использовали здесь потоки, если очевидно, что параллелизм не даёт никакой выгоды?»
Хорошо: «Модель параллелизма здесь добавляет сложности в систему, и я не вижу какого-то фактического преимущества производительности. Поскольку нет никакого преимущества в производительности, лучше всего, чтобы этот код был однопоточным, а не использовал несколько потоков».
Объясняйте причины
В «хорошем» примере выше можно заметить, что он помогает разработчику понять, почему вы делаете свой комментарий. Вам не всегда нужно включать эту информацию в свои комментарии, но иногда уместно дать немного больше объяснений по поводу своей логики, лучших практик, которым вы следуете, или как ваше предложение улучшает качество кода.
Указания
В общем случае внесение исправлений — задача разработчика, а не рецензента. Вы не обязаны разрабатывать подробный проект решения или писать код для разработчика.
Впрочем, это не означает, что рецензент не может тут помочь. В целом, следует соблюдать надлежащий баланс между указанием на проблемы и предоставлением прямой помощи. Указание на проблемы и предоставление разработчику возможности принять решение часто помогает разработчику учиться и облегчает выполнение код-ревью. Это также может привести к лучшему решению, потому что разработчик ближе к коду, чем рецензент.
Однако иногда более полезны прямые инструкции, предложения или даже код. Основная цель код-ревью — получить наилучший CL. Вторичной целью является повышение квалификации разработчиков, чтобы для них ревью занимало всё меньше и меньше времени.
Принимайте объяснения
Если попросить разработчика объяснить непонятный фрагмент кода, обычно это приведёт к тому, что он перепишет его более ясно. Иногда добавление комментария — тоже подходящий ответ, если это не просто объяснение слишком сложного кода.
Объяснения, написанные только в инструменте ревью, не помогут будущим читателям. Они приемлемы только в некоторых случаях, например, когда вы просматриваете область, с которой не очень знакомы, и разработчик объясняет то, что обычные читатели кода и так должны знать.
Как преодолевать сопротивление в процессе код-ревью
Иногда разработчик спорит в процессе код-ревью. Либо он не согласен с вашим предложением, либо жалуется, что вы слишком строги в целом.
Кто прав?
Когда разработчик не согласен с вашим предложением, сначала найдите время, чтобы рассмотреть его позицию. Часто он ближе вас к коду и поэтому у него действительно может быть лучшее представление о некоторых аспектах. Есть ли смысл в его аргументе? Есть ли смысл с точки зрения качества кода? Если так, то признайте его правоту, и вопрос отпадёт.
Однако разработчики не всегда правы. В этом случае рецензент должен дополнительно объяснить, почему он считает, что его предложение является правильным. Хорошее объяснение демонстрирует как понимание ответа разработчика, так и дополнительную информацию о том, почему требуется изменение.
В частности, когда рецензент считает, что его предложение улучшит качество кода, он должен настаивать на нём, если считает, что полученное улучшение качества оправдывает дополнительную работу. Улучшение качества кода, как правило, происходит небольшими шагами.
Иногда требуется несколько раундов объяснений, прежде чем оно действительно будет принято. Просто убедитесь, что всегда остаётесь вежливым, и пусть разработчик знает, что вы его слышите, просто не согласны.
Насчёт обиды разработчиков
Рецензенты иногда считают, что разработчик расстроится, если рецензент настаивает на улучшении. Иногда разработчики действительно расстраиваются, но обычно ненадолго, а позже они будут очень благодарны, что вы помогли им улучшить качество их кода. Обычно, если вы вежливы в своих комментариях, разработчики на самом деле не расстраиваются вообще, и беспокойство находится только в голове рецензента. Расстраивает обычно больше стиль написания комментариев, чем настойчивость рецензента в отношении качества кода.
Отложенные правки
Типичный источник споров заключается в том, что разработчики (по понятным причинам) хотят добиться завершения работы. Они не хотят проходить ещё один раунд ревью для этого CL. Поэтому они говорят, что уберут что-то в более позднем CL, а сейчас вы должны сделать LGTM для этого CL. Некоторые разработчики очень хорошо с этим справляются, и сразу напишут другой CL, который исправит проблему. Однако опыт показывает, что чем больше времени проходит после исходного CL, тем меньше вероятность, что эта правка произойдёт. На самом деле, обычно, если разработчик не делает правку немедленно, то обычно не делает никогда. Не потому, что разработчики безответственны, а потому, что у них много работы, и правка теряется или забывается под валом другой работы. Таким образом, обычно лучше настаивать на немедленном исправлении, прежде чем коммит уйдёт в кодовую базу и будет «завершён». Позволить отложенные правки — типичный способ вырождения кодовых баз.
Если CL вводит новую сложность, то должен быть обязательно исправлен перед отправкой, если это не чрезвычайная ситуация. Если CL проявляет другие проблемы, которые нельзя решить прямо сейчас, разработчик должен зарегистрировать баг в трекере и назначить его на себя, чтобы он не потерялся. Он может дополнительно написать комментарий TODO в коде относительно данной ошибки.
Общие жалобы на строгость
Если вы раньше проводили довольно поверхностные код-ревью, и переключаетесь в строгий режим, некоторые разработчики начнут очень громко жаловаться. Повышение скорости код-ревью обычно устраняет эти жалобы.
Иногда, чтобы жалобы исчезли, могут потребоваться месяцы, но в конечном итоге разработчики склонны увидеть ценность строгих код-ревью, поскольку они видят, какой отличный код получается в итоге. Иногда самые громкие протестующие даже становятся вашими самыми сильными сторонниками, когда происходит что-то такое, что заставляет их действительно увидеть ценность строгих ревью.