Содержание

1. Стандарт код-ревью

2. На что обращать внимание

3. Навигация по CL

4. Скорость ревью

5. Как писать комментарии

6. Обработка обратной связи

Стандарт код-ревью

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

Для того, чтобы добиться этого, необходимо сбалансировать ряд компромиссов.

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

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

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

Таким образом, мы получаем следующее правило:

Рецензенты должны одобрять CL, если он улучшает общую код, даже если CL не идеален.

Это главный принцип среди всех подобных руководств.

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

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

Не ограничивайте себя в комментировании. Если комментарий не обязателен к исполнению, ставьте перед ним что-то вроде «Nit:» ("придирка"), чтобы автор знал, что его можно игнорировать.

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

Менторство

Код-ревью может существенно прокачать разработчика в плане языка, архитектуры или принципов ПО. Всегда приятно оставлять комментарии, которые помогают разработчику узнать что-то новое. Коллаборации прокачивают также и код. Просто имейте в виду, что если ваш комментарий носит чисто образовательный характер, но не является критическим, добавьте к нему префикс «Nit:» или иным образом укажите, что автор не обязан разрешать его в этом CL.

Принципы

• Технические факты и данные преобладают над мнениями и личными предпочтениями.

• В вопросах стиля руководство по стилю является абсолютным авторитетом. Если в руководстве нет какого-то правила, используемого разработчиком, допускается стиль, используемый разработчиком. Должен быть хоть какой-то стиль. Если предыдущего стиля нет, примите авторский.

• Архитектурные аспекты почти никогда не являются вопросом личных предпочтений. Они должны быть основаны на основополагающих принципах, а не просто на личных предпочтениях. Иногда есть несколько допустимых вариантов реализации. Если автор может продемонстрировать (либо с помощью данных, либо на основе твёрдых инженерных принципов), что несколько подходов одинаково верны, рецензент принимает предпочтение автора. В противном случае, выбор диктуется стандартными принципами проектирования.

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

Разрешение конфликтов

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

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

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

На что обращать внимание

Архитектура

Самое важное, на что нужно обратить внимание в код-ревью, — это общая архитектура. Осмысленно ли взаимодействие компонентов? Затрагивает ли изменение ваш проект или библиотеку? Хорошо ли код интегрируется с остальной частью системы? Не пора ли добавить эту функцию?

Функциональность

Выполняет ли CL задуманное разработчиком? Хорошо ли задуманное разработчиком для пользователей этого кода? «Пользователи» обычно являются как конечными пользователями, так и разработчиками (которые будут «использовать» этот код в будущем).

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

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

Очень важно вспомнить о функциональности, если в CL реализована многопоточность, которая теоретически может вызвать deadlock или race condition. Такого рода проблемы очень трудно обнаружить при простом запуске, и они требуют тщательного обдумывания разработчиком и рецензентом (лучше вообще не использовать многопоточность, где возможны race condition или deadlock, поскольку это может сильно усложнить проверку кода или его понимание).

Сложность

Является ли CL слишком сложным? Проверяйте на сложность каждую строчку. Функции слишком сложны? Классы слишком сложны? «Слишком сложный» обычно означает «читатель не может его быстро понять». Это также может означать, что «разработчики могут допустить баги при вызове или изменении этого кода».

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

Тесты

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

Убедитесь, что тесты правильные, осмысленные и полезные. Тесты не проверяют сами себя, и мы редко пишем тесты для наших тестов — вы должны убедиться, что тесты валидны.

Будут ли тесты на самом деле падать на багах? А если под ними изменится код? Делает ли тест простые и полезные проверки? Правильно ли разделены тесты между различными методами тестирования?

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

Правила именования

Хорошие ли имена выбрал разработчик? Хорошее имя должно быть достаточно длинным, чтобы дать полное описание о том, что представляет собой предмет или что он делает, но не настолько длинное, чтобы его было трудно читать.

Комментарии

Написал ли разработчик чёткие комментарии на понятном языке? Все ли комментарии действительно необходимы? Обычно комментарии полезны, когда они объясняют, почему существует тот или иной код, и не должны объяснять, что делает этот код. Если код недостаточно понятен, чтобы его можно было объяснить, его нужно упростить. Есть некоторые исключения (например, регулярные выражения и сложные алгоритмы часто выигрывают от комментариев, которые объясняют, что они делают), но в основном комментарии предназначены для информации, которую не может содержать код, например, для обоснования решения.

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

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

Стиль

У нас в Google есть руководства по стилю для всех наших основных языков и даже для большинства второстепенных языков. Убедитесь, что CL следует соответствующим руководствам по стилю.

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

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

Последовательность

Что делать, если существующий код не соответствует руководству по стилю? В соответствии с нашими принципами проверки кода руководство по стилю является абсолютным авторитетом: если руководство по стилю чего-то требует, CL должен следовать рекомендациям.

В некоторых случаях, руководство по стилю даёт рекомендации, а не декларирует требования. В этих случаях нужно решить, должен ли новый код соответствовать рекомендациям, или же он должен соответствовать уже написанному коду. Лучше следовать руководству по стилю, если только локальное несоответствие не будет слишком запутанным.

Если никакое другое правило не применяется, автор должен поддерживать согласованность с существующим кодом.

В любом случае, попросите автора сообщить об ошибке и добавить TODO для очистки существующего кода.

Документации

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

Внимание к каждой строке

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

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

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

Исключения

Что, если вам не имеет смысла просматривать каждую строку? Например, вы являетесь одним из нескольких рецензентов CL, и вас могут спросить:

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

• Чтобы просмотреть только определенные аспекты CL, такие как общий дизайн, последствия для конфиденциальности или безопасности и т. д.

В этих случаях, отметьте в комментарии, какие части вы просмотрели. Предпочтительно ставить отметку "мне нравится" (LGTM - Looks Good To Me) в комментарии.

Если вместо этого вы хотите поставить LGTM после просмотра CL другими рецензентами, отметьте это явно в комментарии. Стремитесь быстро реагировать, как только CL достигнет желаемого состояния.

Контекст

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

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

Правильные вещи

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

Итого

Выполняя код-ревью, вы должны убедиться, что:

• Код хорошо написан.

• Функциональность хороша для всех пользователей кода.

• Любые изменения пользовательского интерфейса разумны и хорошо выглядят.

• Любая многопоточность выполняется безопасно.

• Код не сложнее, чем должен быть.

• Разработчик не реализует вещи, которые могут понадобиться в будущем.

• Код покрыт модульными тестами.

• Тесты хорошо продуманы.

• Разработчик использовал понятные имена.

• Комментарии ясны и полезны, и в основном объясняют, почему, а не что.

• Код надлежащим образом задокументирован.

• Код соответствует принятым руководствам по стилю.

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

Навигация по CL

Кратко

Теперь, когда вы знаете, на что обращать внимание, как же эффективнее ревьюить код, разбросанный по нескольким файлам?

1. Есть ли смысл в изменении? Достаточно ли описание?

2. Сначала обратите внимание на самую важную часть изменений. Хорошо ли она спроектирована в целом?

3. Посмотрите на остальную часть CL в соответствующей последовательности.

Шаг первый: взгляните на изменения широко

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

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

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

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

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

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

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

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

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

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

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

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

Скорость ревью

Почему проверка кода должна быть быстрой?

В Google мы оптимизируем скорость командной разработки продукта, а не скорость написания кода отдельным разработчиком. Скорость индивидуального развития важна, но не так важна, как скорость всей команды.

Когда проверка кода идет медленно, происходит несколько вещей:

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

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

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

Насколько быстрыми должны быть проверки кода?

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

Один рабочий день — это максимальное время, необходимое для ответа на запрос на код-ревью (т. е. первым делом на следующее утро).

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

Скорость или отрыв от задачи?

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

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

Быстрые ответы

Когда мы говорим о скорости код-ревью, нас интересует время отклика, а не то, сколько времени требуется CL, чтобы пройти все проверки и вмёрджиться. В идеале, весь процесс также должен быть быстрым, но еще более важно, чтобы отдельные ответы приходили быстро, чем чтобы весь процесс происходил быстро.

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

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

Важно, чтобы рецензенты тратили на рецензирование достаточно времени, чтобы быть уверенными, что их «LGTM» означает «этот код соответствует нашим стандартам». Тем не менее, индивидуальные ответы в идеале должны быть быстрыми.

Ревью в условиях разных часовых поясов

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

LGTM с комментариями

Чтобы ускорить проверку кода, существуют определенные ситуации, в которых рецензент должен дать LGTM/одобрение, даже если он также оставляет неразрешенные комментарии к CL. Это делается, когда либо:

• Рецензент уверен, что разработчик надлежащим образом рассмотрит все оставшиеся комментарии рецензента.

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

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

Большие CL

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

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

Улучшения проверки кода с течением времени

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

Авралы

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

Как писать комментарии

Кратко

• Будьте благожелательны.

• Объясните свои рассуждения.

• Уравновешивайте чёткие указания простым указанием на проблемы и предоставлением решения разработчику.

• Поощряйте разработчиков упрощать код или добавлять комментарии к коду, а не просто объяснять, почему он такой сложный.

Благожелательность

В общем, важно быть вежливым и уважительным, а также очень понятным и полезным для разработчика, чей код вы просматриваете. Один из способов сделать это — убедиться, что вы всегда комментируете код и никогда не комментируете разработчика. Вам не всегда нужно следовать этой практике, но вы обязательно должны использовать ее, когда говорите что-то, что в противном случае могло бы расстроить или вызвать споры. Например:

Плохо: «Зачем вы втащили сюда многопоточность, когда от неё тут явно нет никакой пользы?»

Хорошо: «В данном случае, многопоточность усложняет систему без какого-либо реального выигрыша в производительности. Поскольку выигрыша в производительности нет, лучше, чтобы код был однопоточным, а не многопоточным».

Объясните, почему

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

Руководство

Как правило, за исправление CL отвечает разработчик, а не рецензент. От вас не требуется детального проектирования решения или написания кода для разработчика.

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

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

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

Маркировка серьезности комментария

Рассмотрите возможность маркировки серьезности ваших комментариев, отличая необходимые изменения от рекомендаций или предложений.

Вот некоторые примеры:

Nit: Незначительно. Технически это можно сделать, но на ситуацию это не повлияет.

Optional (или Consider): идея интересная, но не обязательная.

FYI: необязательно делать это в этом CL, но на будущее это интересная идея.

Маркировка делает цель рецензирования явной и помогает авторам расставлять приоритеты по важности различных комментариев. Она также помогает избежать недоразумений; например, без признака комментария авторы могут интерпретировать все комментарии как обязательные, даже если некоторые комментарии носят информационный или необязательный характер.

Принятие объяснений

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

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

Обработка обратной связи

Иногда разработчик отказывается от ревью. Либо он не согласен с вашим предложением, либо считает, что вы придираетесь.

Кто прав?

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

Однако разработчик не всегда прав. В этом случае, рецензент должен дополнительно объяснить, почему он считает свое предложение правильным. Хорошее объяснение демонстрирует как понимание ответа разработчика, так и дополнительную информацию о том, почему вы требуете изменения.

В частности, когда рецензент считает, что его предложение улучшит работоспособность кода, он должен продолжать выступать за исправление, если он считает, что полученное в результате улучшение качества кода оправдывает дополнительные правки. Улучшение работоспособности кода, как правило, происходит небольшими шагами.

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

Разочарование разработчика

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

"Подчищу позже"

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

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

Общие жалобы на строгость

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

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

Разрешение конфликтов

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