код review что такое
Практики хорошего code review, или что такое code review за 15 минут. Доклад Никиты Соболева на DUMP в Казани
В 2019 году на DUMP в Казани выступал Никита Соболев – технический директор компании wemake.services. И Никита на протяжении почти 40 минут пытался вскипятить мозги слушателей секции Backend, рассуждая о code review. Сегодня хотим привести расшифровку этого «взрывного» доклада, чтобы если уж мозги бурлили, то у всех сразу.
А вот, кстати, и сам Никита Соболев во время своего выступления.
Для удобства вот ссылка на презентацию.
Доклад начался с постановки того, о чем же он будет. И внезапно прозвучала фраза: «Я не буду рассказывать о code review. Доклад будет про хардкорные технические инструменты и методы автоматизации». Главный тезис – если вы делаете code review, то вы уже проиграли. Интригующе звучит? 🙂 Давайте вместе с Никитой разбираться, что же за этим стоит.
За основу были взяты понятия, которые предложил американский писатель Карлос Кастанеда, делание и неделание. Что же это значит? Делание – это привычка, внутри которой мы существуем. Эдакое «хомячье колесо». А неделание – взгляд на те же самые вещи, но под другим углом. Никита ловко наложил эти понятия на «просмотр кода». И получилось, что мы имеем «делание code review» и, соответственно, «неделание code review». Вот как раз последнему и посвящен доклад.
Делание становится рутиной, чем-то сложным, где нужно применить свои коммуникативные навыки, быть эмпатичным. И встает вопрос: а нафига я буду смотреть какой-то код? Неделание же – это гармония. Ты смотришь на код и восхищаешься им. Ты понимаешь, что вот этот вот код станет частью продукта и принесет новые деньги. И конечная цель, к которой подводит нас Никита всеми этими речами в том, чтобы делать code review за 15 минут. И даже есть живой пример.
Но для начала нужно разобраться, почему же все так плохо с code review?
Когда ты узнал, что есть code review, то начинаешь пытаться решить все проблемы с его помощью. А проблем на самом деле очень много, как и целей. Какие же цели ставят перед code review?
Казалось бы, вполне себе нормальные цели, но нет. Никита считает, что все эти цели неправильные. Причем как только вы это поймете, то измените свое отношение с делание на неделание. Встают резонные вопросы: а какая тогда правильная цель? Чем же плохи эти цели? Пойдем разбираться с конца.
Обучение людей с помощью code review.
Обычно новым людям дают какую-то неважную задачу, а это плохая идея. Она тянет за собой изоляцию (посиди вон там, сам поразбирайся) и отсутствие контроля (задача же неважная, поэтому всем пофиг).
Решение: маленькие задачи. Вы можете дать новичку сразу что-то важное, но при этом небольшое. Это ускоряет обратную связь, увеличивает вероятность найти косяк в коде. Ведь чем массивнее код, тем сложнее поиск ошибок – говорит статистика. Под «маленькими» подразумеваются задачи от 15 минут до 2х, максимум 4х часов. Соответственно, это все предполагает наличие онбординга и хорошей документации. За примером хорошего онбординга (без нудного code review по неделям) идем на Open-Source. Что у них есть? Чек-лист:
Если ваш онбординг построен как-то не так, то вам есть куда расти, потому что на Open-Source люди стремятся вот к этому.
Для вдохновения Никита предлагает ряд хороших мест:
Перейдем к обсуждению следующей проблемы — review архитектуры. Обычно оно корявое. Какие с ним могут быть проблемы?
Решение: проектировать и прототипировать design до review. Design до review – это когда вы делаете не саму архитектуру, а ее скелет. Текстом, или фиксируете в каком-нибудь файлике. Человек по скелету сможет сказать нравится\не нравится. Это намного быстрее и эффективнее, чем code review в данном смысле.
И, конечно, автоматизация! Никита рассказал о том, как можно автоматизировать архитектурные проверки.
В каждом приложении есть слои (тут, кстати, Никита проводил аналогию с офигенным бургером). И чтобы проверять их – есть все возможности. Далее примеры пойдут на языке python.
Таким образом можно ревьюить архитектуру автоматически.
Следующая проблема – «неповторение грабель».
Часто code review нужен для того, чтобы показать новым разработчикам, что мы так уже делали. Как правило, приходится потом повторять как попугай одно и то же. Решение: документация и регрессионное тестирование.
В понимании нашего чудесного докладчика, хорошая документация – это когда вот так.
Все начинается с какого-то бизнес-требования, затем это превращается в BDDSpec. Вкратце, BDDSpec — это то же самое требование, но написанное структурированным языком, которое потом превращается в тест. Этот тест является вашей документацией, потому что структурированный текст понятен человеку. Собственно, BDD – инструмент общения, при помощи которого вы пишите и тест, и документацию. Отсюда появляется еще одна сущность – реализация, связанная с тестом и документацией. Все это помогает избежать совершения одних и тех же ошибок, ведь все уже указано в тестах.
В ситуациях когда что-то сломалось, можно написать регрессионный тест. И выходит вот такая «документашка».
И теперь другая автоматизация! Часто люди делают ошибки не только в коде. Для решения этой проблемы есть замечательный проект – Danger. С ним схема будет выглядеть вот так:
На 2020 год danger-правило можно написать на следующих языках: JS, Swift, Ruby, Kotlin и Python. Никита в своем докладе привел примеры на JS.
Так это выглядит в итоге. Здесь исключено человеческое взаимодействие, все автоматизировано.
И еще одна фишечка – утилита bellybutton. Она есть для всех языков (проверено Никитой). Допустим, у вас есть некая deprecated_fn(), которую не надо вызывать. Она старая и странная, и существует просто потому, что так надо. Вы говорите человеку, чтоб не трогал ее, но он все равно трогает. Когда-нибудь надоест это повторять, и тогда вы берете декларативный файл формата yaml и пишете:
Декларативный файл можно поместить в репозиторий, описать там все правила для текущего проекта и больше человеку по code review не говорить об этом вообще никогда.
Контроль качества – разбираем еще одну проблему. Для человека, который пишет статические анализаторы, слышать фразу о том, что человек может контролировать качество – анекдот.
Почему все так? Во-первых, люди не умеют писать код. Загвоздка в том, что человек должен писать понятно и для другого человека, и для компьютера, а последние два говорят на разных языках. Отсюда и большая сложность. Во-вторых, эта сложность «быть понятным для всех и сразу» постепенно накапливается. В-третьих, скрытость метрик. Чтобы проверить качество кода, нужно сначала вывести метрики куда-то на экран, посмотреть все это дело, а затем еще долго исправлять. Именно поэтому проблема качества кода – это очень и очень серьезная проблема, и к ней нужно подходить с самого начала проекта. Решение: самый жесткий линтер из доступных. Чаще придется писать свой: под разные языки + существующий может быть недостаточно строг. Как говорит Никита: «чем больше вы ненавидите линтер, тем он лучше».
Проблема баланса кода и архитектуры. Тут могут возникать такие ситуации:
Решение: Architecture on Demand. Оно позволит писать ровно столько архитектуры, сколько нужно. Простая архитектура и простой код под текущую ситуацию. Подробнее Никита этот способ разобрал здесь.
И last but not least – проблема контроля выполнения. Обычно говорят, что контроль выполнения – это когда code review, прогон всех тестов, запуск на компьютере, но нет. Перед нами встают проблемы:
Решение: BDD (эффективный способ общения и с заказчиком, и с программистами, и с кем угодно) и Review Apps. Последнее нужно для «посмотреть глазками», потому что мало просто прогнать тест. При таком подходе вы сможете сделать ревью уже задеплоенного приложения. Например, использовать ZEIT для GitLab. Так появляется возможность получить на каждый pull request версию приложения.
Только тогда, когда вы прошли все тесты, клиент посмотрел и сказал, что это то, что нужно, вы сами посмотрели, что это работает, – можно сказать, что задача выполнена. В итоге получается, что это все не про code review. Код еще даже никто не смотрел.
А теперь подведем итоги. Были разобраны все обозначенные проблемы. Как можно увидеть, ни одна из них к code review отношения не имеет. Для каждой из проблем есть свое целевое решение. Зачем тогда нужно code review? Code review нужно для контроля корректности автоматики. Автоматики у нас полно. Она, естественно, сбоит. И нам нужно глазами проверять, что человек при ней сделал неправильно.
Инструменты, о которых мы говорили:
А нужно ли тогда код вообще смотреть, спросите вы? Ответ – конечно, ведь нужен контроль некоторых нюансов:
Вот это основные задачи code review. Все остальное вполне может решить автоматизация. При таком подходе «делание code review» превращается в «неделание», причем за 15 минут. И последние несколько слов от Никиты: «созерцайте ваш код, смотрите на то, как можно сделать лучше вашу кодовую базу и делайте это».
А вы что думаете по поводу философского взгляда на code review?
Как сделать код-ревью быстрее и эффективнее
Как обычно происходит код-ревью? Вы отправляете пул-реквест, получаете обратную связь, вносите исправления, отправляете фиксы на повторный ревью, затем получаете одобрение, и происходит мерж. Звучит просто, но на деле процесс ревью бывает очень трудоемким.
Представьте, что у вас есть пул-реквест с сотнями строк изменений. Ревьюер должен потратить немало времени, чтобы полностью прочитать код и понять предлагаемые изменения. В результате весь процесс от создания пул-реквеста до его утверждения может занять несколько дней — в этом мало приятного и для ревьюера, и для автора изменений. И велики шансы, что в итоге ревьюер все равно что-то упустит. Или проверка может быть слишком поверхностной, а в худшем случае пул-реквест вообще может быть сразу отклонен.
Получается, что чем объемнее пул-реквест, тем меньше пользы будет от его проверки.
Как избежать таких ситуаций? Как сделать пул-реквест проще и понятнее для ревьюера и оптимизировать весь процесс?
Переводим статью нашего бэкенд-разработчика Сергея Жука про то, как устроен процесс код-ревью у команды мобильного приложения Skyeng.
Категории изменений
Давайте представим, что у вас есть задача — реализовать новый функционал в проекте. Пул-реквест, над которым вы работаете, может содержать разные категории изменений. Конечно в нём будет какой-то новый код. Но в ходе работы вы можете заметить, что какой-то код нужно предварительно порефакторить, чтобы он способствовал добавлению нового функционала. Или с этим новым функционалом в коде появилось дублирование, которое вы хотите устранить. Или вы вдруг обнаружили ошибку и хотите ее исправить. Как должен выглядеть окончательный пул-реквест?
Сначала давайте разберемся, какие категории изменений могут происходить с кодом.
Стратегии ревью
Очень важно понимать, что каждая из этих категорий ревьюирутеся по-разному и при их рассмотрении ревьюер должен фокусироваться на разных вещах. Можно сказать, что каждая категория изменений предполагает свою собственную стратегию ревью.
Функциональные изменения. Это самый длительный процесс, потому что он предполагает изменения доменной логики. Ревьюер смотрит, решена ли проблема и проверяет, является ли предложенное решение наиболее подходящим или его можно улучшить.
Структурный рефакторинг. Этот процесс требует гораздо меньше времени, чем функциональные изменения. Но здесь могут возникнуть предложения и разногласия по поводу того, как именно код должен быть организован.
При проверке остальных категорий в 99 % случаев мерж происходит сразу же.
Почему нужно разделять изменения по категориям?
Мы уже обсуждали, что разные категории изменений ревьюируются по-разному. Например, функциональные изменения мы проверяем, отталкиваясь от требований бизнеса, а при структурном рефакторинге — проверяем обратную совместимость. И если мы смешаем несколько категорий, то ревьюеру будет трудно держать в уме одновременно нескольких стратегий ревью. И, скорее всего, ревьюер потратит на пул-реквест больше времени, чем необходимо, и из-за этого может что-то упустить. Более того, если пул-реквест содержит изменения разного рода, при любом исправлении ревьюеру придется пересмотреть все эти категории еще раз. Например, вы смешали структурный рефакторинг и функциональные изменения. Даже если рефакторинг выполнен хорошо, но есть проблема с реализацией функционала, то после исправлений ревьюер должен будет просмотреть весь пул-реквест с самого начала. То есть проверить заново и рефакторинг, и функциональные изменения. Так проверяющий тратит на пул-реквест больше времени. Вместо того, чтобы чтобы сразу смержить отдельный пул-реквест с рефакторингом, ревьюер должен еще раз просмотреть весь код.
Что точно не стоит смешивать
Переименование/удаление класса и его рефакторинг. Здесь мы сталкиваемся с Git, который не всегда правильно понимает такие изменения. Я имею в виду масштабные изменения, когда меняется много строк. Когда вы рефакторите класс, а затем перемещаете его куда-то, Git не воспринимает это как перемещение. Вместо этого Git интерпретирует эти изменения как удаление одного класса и создание другого. Это приводит к куче вопросов во время код-ревью. И автора кода спрашивают, почему он написал этот уродливый код, хотя на самом деле этот код был просто перемещен из одного места в другое с небольшими изменениями.
Любые функциональные изменения + любой рефакторинг. Мы уже обсуждали этот случай выше. Это заставляет ревьюера держать в голове сразу две стратегии рецензирования. Даже если рефакторинг выполнен хорошо, мы не сможем смержить эти изменения, пока функциональные изменения не будут утверждены.
Любые механические изменения + любые изменения, произведенные человеком. Под «механическими изменениями» я подразумеваю любое форматирование, выполненное с помощью IDE или генерации кода. Например, мы применяем новый code style и получаем изменений на 3000 строк. И если мы смешаем эти изменения с какими-либо функциональными или любыми другими изменениями, произведенными человеком, мы заставим ревьюера мысленно классифицировать эти изменения и рассуждать: это изменение, произведенное компьютером, — его можно пропустить, а это изменение, сделанное человеком, — его нужно проверить. Так ревьюер тратит очень много дополнительного времени на проверку.
Пример
Вот пул-реквест с функцией метода, который аккуратно закрывает клиентское соединение с Memcached:
Даже если пул-реквест небольшой и содержит всего сто строк кода, его все равно сложно проверять. Как видно по коммитам, он содержит различные категории изменений:
В результате ревьюер должен просмотреть весь код и
1. Рефакторинг: извлечение класса
Здесь всего два файла. Ревьюер должен проверить только новый дизайн. Если все в порядке — мержим.
2. Следующим шаг — тоже рефакторинг, мы просто перемещаем два класса в новое пространство имен
Такой пул-реквест довольно просто проверять, он может быть смержен сразу.
3. Удаление лишних блоков док-блоков
Здесь ничего интересного. Мержим.
4. Сам функционал
И теперь пул-реквест с функциональными изменениями содержит только нужный код. Так ваш ревьюер может сосредоточиться только на этой задаче. Пул-реквест небольшой и его легко проверить.
Заключение
Не создавайте огромных пул-реквестов со смешанными категориями изменений.
Чем больше пул-реквест, тем труднее ревьюеру понять предложенные вами изменения. Скорее всего, огромный пул-реквест с сотнями строк кода будет отклонен. Вместо этого разбейте его на маленькие логические части. Если ваш рефакторинг в порядке, но функциональные изменения содержат ошибки, то рефакторинг можно спокойно смержить, и таким образом вы и ваш ревьюер сможете сосредоточиться на функционале, не просматривая весь код с самого начала.
И всегда выполняйте следующие шаги до отправки пул-реквеста:
От редакции: Сергей много пишет интересного про программирование и PHP, а мы иногда что-то переводим: сервер потокового видео, рендеринг HTML файлов. Смело задавайте ему вопросы в комментариях к этой статье — он ответит!
Ну а также напоминаем, что у нас всегда много интересных вакансий для разработчиков!
Как правильно делать код-ревью?
В жизни разработчика наступает момент, когда он начинает проводить код-ревью. Как правило, это один из признаков роста программиста: к его мнению начинают по-настоящему прислушиваться и доверять более серьезные задачи. Конкретный момент, когда разработчику предлагают заняться ревью зависит от структуры компании: есть команды, где это могут делать более-менее все по истечению некоторого времени работы, а есть такие, где процесс ревью целиком лежит на самых старших и опытных коллегах.
Вместе с тем, редко кто рассказывает о том, каких принципов необходимо придерживаться для проведения качественного ревью: каковы главные цели этой процедуры, за чем смотреть в первую очередь, насколько быстро его нужно проводить.
Всвязи с этим, «Руководство компании 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 и с чего начинать ревью.