Что такое code review
Code review — инженерная практика в терминах гибкой методологии разработки. Это анализ (инспекция) кода с целью выявить ошибки, недочеты, расхождения в стиле написания кода, в соответствии написанного кода и поставленной задачи. К очевидным плюсам этой практики можно отнести:
Улучшается качество кода
Находятся «глупые» ошибки (опечатки) в реализации
Повышается степень совместного владения кодом
Код приводится к единому стилю написания
Хорошо подходит для обучения «новичков», быстро набирается навык, происходит выравнивание опыта, обмен знаниями.
Что можно инспектировать
Для ревью подходит любой код. Однако, review обязательно должно проводиться для критических мест в приложении (например: механизмы аутентификации, авторизации, передачи и обработки важной информации — обработка денежных транзакций и пр.). Также для review подходят и юнит тесты, так как юнит тесты — это тот же самый код, который подвержен ошибкам, его нужно инспектировать также тщательно как и весь остальной код, потому что, неправильный тест может стоить очень дорого.
Как проводить review
Вообще, ревью кода должен проводиться в совокупности с другими гибкими инженерными практиками: парное программирование, TDD, CI. В этом случае достигается максимальная эффективность ревью. Если используется гибкая методология разработки, то этап code review можно внести в Definition of Done фичи.
Из чего состоит review
Сначала design review — анализ будущего дизайна (архитектуры).Данный этап очень важен, так как без него ревью кода будет менее полезным или вообще бесполезным (если программист написал код, но этот код полностью неверен — не решает поставленную задачу, не удовлетворяет требованиям по памяти, времени). Пример: программисту поставили задачу написать алгоритм сортировки массива. Программист реализовал алгоритм bogo-sort, причем с точки зрения качества кода — не придраться (стиль написания, проверка на ошибки), но этот алгоритм совершенно не подходит по времени работы. Поэтому ревью в данном случае бесполезно (конечно — это утрированный пример, но я думаю, суть ясна), здесь необходимо полностью переписывать алгоритм.
Собственно, сам code review — анализ написанного кода. На данном этапе автору кода отправляются замечания, пожелания по написанному коду.
Также очень важно определиться, за кем будет последнее слово в принятии финального решения в случае возникновения спора. Обычно, приоритет отдается тому кто будет реализовывать код (как в scrum при проведении planning poker), либо специальному человеку, который отвечает за этот код (как в google — code owner).
Как проводить design review
Design review можно проводить за столом, в кругу коллег, у маркерной доски, в корпоративной wiki. На design review тот, кто будет писать код, расскажет о выбранной стратегии (примерный алгоритм, требуемые инструменты, библиотеки) решения поставленной задачи. Вся прелесть этого этапа заключается в том, что ошибка проектирования будет стоить 1-2 часа времени (и будет устранена сразу на review).
Как проводить code review
Можно проводить code review разными способами — дистанционно, когда каждый разработчик сидит за своим рабочим местом, и совместно — сидя перед монитором одного из коллег, либо в специально выделенным для этого месте, например meeting room. В принципе существует много способов (можно даже распечатать исходный код и вносить изменения на бумаге).
Данный вид review проводится перед внесением изменений в VCS. Этот подход позволяет содержать в репозитории только проверенный код. В microsoft используется этот подход: всем участникам review рассылаются патчи с изменениями. После того как собран и обработан фидбэк, процесс повторяется до тех пор пока все ревьюверы не согласятся с изменениями.
Данный вид review проводится после внесения изменений в VCS. При этом можно коммитить как в основную ветвь, так и во временную ветку (а в основную ветку вливать уже проверенные изменения).
Заметки консультанта
Лучшие секреты рецензирования кода. Пять типов рецензирования
Posted by Shamrai Alexander на 13 июня, 2013
За и против формальных проверок, через-плечо, через-почту, парного программирования и инструментального рецензирования.
Есть много способов ободрать кошку. Я могу придумать сразу четыре. Есть также много способов выполнить коллегиальную оценку и каждый имеет свои за и против.
Формальные инспекции
Исторически «формальные» рецензии обычно вызывают «инспекциями». Это пережиток исследования Майкла Фагана 1976 года в IBM относительно эффективности коллегиальных оценок. Он попробовал много комбинаций переменных и придумал процедуру для рецензирования до 250 строк прозы или исходного кода. После 800 итераций он придумал формализованную инспекционную стратегию, и по сей день Вы можете заплатить ему, чтобы он Вам рассказал об этом (название компании: Fagan Associates). Его методы были позже другими изучены и подробно расширены, прежде всего Томом Джилбом и Карлом Виджерсом.
В целом «формальное» рецензирование относится к тяжеловесному процессу рецензирования с тремя -шестью участниками, встречающимися вместе в одной комнате с распечатками и/или проектором. Кто-то «модератор» или «контроллер» и действует как организатор, сохраняет всех в контексте задачи, управляет темпом анализа и действует как арбитр в спорах. Все прочитывают материалы заранее, чтобы должным образом подготовиться к встрече.
Каждому участнику будет присвоена определенная «роль». Для «рецензента» можно определить задачу критического анализа, в то время как «наблюдатель» может бы быть призван как консультант проблемной области или изучить, как правильно делать рецензирование. На инспекции Фагана «читатель» просматривает исходный код только для понимания – не для критического анализа – и представляет его группе. Это разделяет то, что автор предполагал, от того, что фактически представлено; часто сам автор в состоянии выбрать дефекты, получив такое описание со стороны.
Когда дефекты обнаруживаются при формальном рецензировании, они обычно регистрируются довольно подробно. Помимо общего расположения ошибки в коде, включается также детализация, такая как серьезность (например, серьезный, незначительный), тип (например, алгоритм, документация, использование данных, обработка ошибок) и фаза ввода (например, ошибка разработчика, проблема проектирования, ошибка требований). Обычно эта информация хранится в базе данных, поэтому возможно проанализировать под разными углами метрики дефектов и, возможно, сравниться с подобными метриками от QA.
Формальные инспекции также обычно используют и другие метрики, такие как отдельное время, потраченное на предварительную встречу ознакомления, и время самой встречи, уровни строк кода инспекции и проблемы, с которыми встречаются в самом процессе. Эти числа и комментарии периодически исследуются на встречах усовершенствования процесса; Инспекции Фагана имеют еще следующий шаг, который требует прохождения анкеты-опросника по процессу после каждой встречи для дальнейшего его улучшения.
Типичный поток операций для «формальной» инспекции. Не показаны артефакты, создаваемые анализом: журнал дефектов, заметки по встрече и журнал метрик. Некоторые в инспекции также используют закрывающую анкету-опросник на заключительной встрече.
Самый большой актив формальных инспекций также и самый ее большой недостаток: у вас много людей, тратящих много времени на чтение кода и обсуждение его последствий, вы идентифицируете много дефектов. Существует много исследований, которые показывают, что формальные инспекции могут идентифицировать большое количество проблем в исходном коде.
Но большинство организаций не может позволить себе связать так много людей надолго. Вы также должны запланировать встречи – это уже грандиозная задача сам по себе, которая заканчивает тем, что используется дополнительное время разработчика. Наконец, чтобы эффективно использовать формальные методы, необходимо проводить по ним обучение, а это дополнительное время и расходы, с которыми трудно согласиться, особенно когда Вы еще не привыкли к выполнению рецензирования кода.
Много исследований за прошлые 15 лет продемонстрировали, что другие формы рецензирования раскрывают столько же дефектов, как и формальные рецензии, но с намного меньшим количеством времени и обучением. Этот результат – ожидаемый теми, кто попробовал много типов рецензий – поместил формальные инспекции в сторону отрасли.
В конце концов, если вы можете извлечь всю доказанную пользу из формальных инспекций и готовы потратить 1/3 времени разработчика, это лучше, чем ничего.
Поэтому давайте посмотрим другие методы.
Рецензирование через-плечо
Это наиболее распространенное и неофициальное из рецензирований кода. Рецензирование «через-плечо» простое – разработчик стоит над рабочим местом автора, в то время как автор проводит рецензента по изменениям кода.
Обычно автор «ведет» рецензирование, находясь за клавиатурой и мышью, открывая различные файлы, указывая на изменения и объясняя, почему был выбран такой путь. Автор может представить изменения, используя различные инструменты, и даже переходя назад и вперед между изменениями и другими файлами в проекте. Если рецензент видит что-то неправильное, они могут поучаствовать в небольшом «парном программировании на месте», когда автор пишет исправление, а рецензент наблюдает. Большие изменения, где рецензент не обязателен, выполняются отдельно.
С современным программным обеспечением использующим общий рабочий стол так называемый анализ «через-плечо» может иметь место и для удаленных ситуаций. Это усложняет процесс, потому что вы должны запланировать эти встречи совместного использования и связаться по телефону. Положение через-плечо позволяет людям указывать, писать примеры или даже переходить к электронной доске для обсуждения, что сложнее сделать по Интернету.
Самое очевидное преимущество рецензирования через-плечо — простота выполнения. Любой может это делать, в любое время и без обучения. Это может применяться каждый раз, когда вам рецензирование необходимо – особенно при сложном изменении или изменении в «стабильной» ветви кода.
Когда все лично рецензируют, метод через-плечо позволяет обучаться и делиться навыками между разработчиками, и заставляет людей взаимодействовать лично вместо того, чтобы скрываться за безличной электронной почтой и мессенджером. Вы естественно говорите больше, когда вы можете поболтать, и может родиться идея вместо того, чтобы зарегистрировать некий формальный «дефект» в какой-нибудь базе данных.
К сожалению, непринужденность и простота процесса также приводят к горе недостатков. Во-первых, это не измеряемый процесс – нет ничего, что передается менеджеру, рассматриваются ли все изменения кода. Фактически, нет никаких метрик, отчетов или инструментов, которые измеряют что-либо вообще о процессе.
Во-вторых, для автора просто неумышленно пропустить изменение. Бесчисленное количество раз мы наблюдали анализ, который завершался, автор регистрировал свои изменения, и когда он видит, что список файлов уже загружен, он говорит, «О, я изменял этот? «. Слишком поздно!
Процесс рецензирования через-плечо
В-третьих, когда рецензент сообщает о дефектах и покидает комнату, редко когда он возвращается назад, чтобы проверить, что дефекты были исправлены должным образом и что никакие новые дефекты не были введены. Если Вы не проверяете, что дефекты исправлены, значение их обнаружения уменьшается.
Есть другой эффект рецензирования через-плечо, которые некоторые люди рассматривают как преимущество, а другие как недостаток. Поскольку автор управляет темпом анализа, часто рецензент идет слишком быстро по коду. Рецензент не может продумать сложную часть кода. Рецензент не имеет шансов пройтись по другим исходным файлам, чтобы удостовериться, что изменение не будут ломать что-то еще. Автор мог бы рассказать что-то, что разъяснит код рецензенту, но следующий разработчик, который прочитает этот код, не будет иметь этого объяснения, если этого не будет как комментарий в коде. Для рецензента трудно быть объективным и понимающим эти проблемы, будучи ведомым по коду с взаимодействующим c ним разработчиком.
Например, скажем, что для автора определили задачу с исправлением ошибки, где часть диалогового окна прорисовывалась неправильно. После борьбы с документацией GUI Windows он наконец обнаруживает недокументированную «функцию» в вызове API прорисовки текста, которая вызывала проблемы. Он обработает ошибку с некоторым новым кодом и решит проблему. Когда рецензент доберется до этого обходного решения, это будет выглядеть сначала забавно.
«Почему Вы делаете так,» спросит рецензент, «ведь API GUI Windows сделает это за Вас?»
«Да, я думал так также,» ответит автор, «но оказывается, что в этом случае обрабатывается не правильно. Поэтому я должен был вызвать его по-другому здесь.»
Для рецензента будет просто принять изменения. Но следующий разработчик, который прочтет этот код, будет иметь такой же вопрос и может даже удалит обходное решение в процессе чистки кода. «В конце концов», скажет следующий разработчик, «API Windows делает это для нас, поэтому никакой потребности в этом дополнительном коде нет!»
С другой стороны, не все так плохо. С изменениями, которые касаются многих файлов, часто полезно рассматривать файлы в определенном порядке. И иногда изменение будет прозрачно будущему читателю, но рецензенту, возможно, понадобилось объяснение того, почему вещи были изменены таким образом.
Наконец, рецензирование через-плечо по определению не работает, когда автор и рецензент не находятся в одном и том же здании; они, вероятно, могут быть в соседних офисах. Для любого вида удаленного рецензирования Вы должны обеспечить некоторую электронную коммуникацию. Даже с общим рабочим столом и спикерфонами многие преимущества взаимодействий лицом к лицу будут утеряны.
Рецензирование через-почту
Это вторая наиболее распространенная форма неофициального рецензирования кода и метод, предпочтительный для большинства проектов с открытым исходным кодом. В этом случае целые файлы или изменения упаковываются автором и отправляются рецензентам по электронной почте. Рецензенты рассматривают файлы, задают вопросы и обсуждают с автором и другими разработчиками, предлагают изменения.
Самая сложная часть почтовой рецензии состоит в поиске и сборе рецензентов для файлов. На конец автор должен выяснить, как собрать файлы. Например, если это рецензирование изменений, предлагаемых для регистрации в управлении версиями, пользователь должен идентифицировать все файлы, добавленные, удаленные и измененные и скопировать их куда-нибудь, затем загрузить предыдущие версии этих файлов (так чтобы рецензенты видели, что было изменено), и организовать файлы так, чтоб рецензенты знали, какие с какими файлами сравнивать. Далее рецензенты должны извлечь эти файлы из электронной почты и сгенерировать различия для них.
Система управления версиями может тут помочь. Обычно эта система может сообщить, какие файлы были изменены и может извлечь предыдущие версии. Несмотря на то, что некоторые люди пишут свои собственные сценарии для сбора всех этих файлов, некоторые используют коммерческие инструменты, которые делают это же и могут обработать большое количество различных случаев из файлов в различных состояниях и клиент-серверных конфигураций.
Система управления версиями может также помочь автоматической отсылкой электронных писем. Например, серверный триггер «регистрации» в системе управления версиями может послать электронное письмо в зависимости от того, кто загрузил код (например, ведущий разработчик каждой группы рецензирует код от участников своей группы) и какие файлы были изменены (например, некоторые файлы «принадлежат» пользователю, который лучше всего квалифицирован для рецензирования изменений). Автоматизация полезна, но для многих процессов рецензирования кода вы захотите провести рецензию перед регистрацией, а не после.
Рисунок 3: Типичный процесс для почтового рецензирования кода уже зарегистрированного в системе управления версиями. Эти фазы не отличимые в действительности, потому что нет никакого материального объекта «рецензирование».
Также как рецензирование через-плечо, рецензирование через-почту просто реализовать, несмотря на то, что имеет место более длительный сбор файлов. Но в отличие от рецензирования через-плечо, они работают одинаково хорошо с разработчиками, работающими через холл или за океаном. И вы устраняете проблему обучения авторов рецензентами через изменения.
Другое уникальное преимущество рецензирования через-почту — простота, с которой другие люди могут быть вовлечены в анализ. Возможно, есть специалист по проблемной области для секции кода, от которого рецензент хочет получить мнение. Возможно, рецензент хочет передать процесс другому рецензенту. Или возможно электронное письмо послано нескольким людям сразу, а они решают для себя, кто лучше всего квалифицирован и каждый рассматривает свою часть кода. Эти процедуры сложны рецензируя лично и с формальными инспекциями, когда все участники должны быть приглашены на встречу заранее.
Еще одно преимущество рецензирования через-почту – они не выбивают рецензентов из «зоны». Хорошо известно, что разработчику требуется 15 минут, чтобы войти «в зону», где они погружены в свою работу и очень производительны. Даже простая просьба рецензирования у разработчика выбивает его из зоны – даже если будет ответ «я слишком занят». С электронными письмами рецензенты могут работать во время самостоятельно выбранного перерыва, поэтому они могут оставаться в своей зоне в течение многих часов.
Есть несколько важных недостатков рецензировании через-почту. Самый большой тот, который присущ большинству тривиальных рецензирований, трудно отследить различные потоки изменений кода и обсуждений. С несколькими обсуждениями относительно нескольких различных областей кода, возможно приглашая других разработчиков в спор, трудно отследить то, что каждый говорил или дошла ли группа до согласия.
Рисунок 4: Типовой процесс для рецензирования кода через-почту уже зарегистрированного в системе управления версиями. Эти фазы не отличимые в действительности, потому что нет никакого материального объекта «рецензирование».
Есть еще сложный момент с удаленным рецензированием. Иронично, но это одно из явных преимуществ рецензирования через-почту то, что они могут быть проводиться с удаленными разработчиками. Удаленное рецензирование может занять много дней, т.к. каждый переход «назад и вперед» может занять день, поэтому для завершения рецензирования может потребоваться пять дней вместо тридцати минут. Это тянет за собой много одновременных рецензий, а это в свою очередь означает еще больше трудностей для сохранения переговоров и связанных с ними изменений кода.
Предположим, что разработчик в Хайдарабаде, открывая Outlook, обнаруживает 25 электронных писем от различных людей, обсуждающих аспекты трех различных изменений кода, которые он сделал за последние несколько дней. Потребуется много времени только для того, чтобы разобраться, перед тем как может начаться реальная работа.
Среди всех преимуществ перед рецензированием через-плечо, почтовое рецензирование имеет некоторые недостатки. Менеджеры по продукту все еще не уверены, рассматриваются ли все изменения кода. Даже с серверными триггерами управления версиями, все, что Вы знаете это то, что изменения были отосланы, но не то, что каждый действительно смотрел на них. И если было согласовано, что определенные дефекты должны быть исправлены, Вы не можете проверить, что эти исправления были сделаны правильно. Также все еще нет никаких метрик для измерения процесса, определения эффективности или измерения эффекта изменений в процессе.
С почтовой передачей мы видели использование нескольких инструментов (т.е. электронная почта, клиентские триггера управления версиями для набора файлов и серверные триггера для автоматизации потока операций), которые позволяют получить некоторые преимущества перед рецензированием через-плечо, без привнесения существенных недостатков. Возможно, введением более сложных, специализированных инструментов мы сможем продолжать добавлять преимущества при удалении оставшихся недостатков.
Инструментальное рецензирование
Это относится к любому процессу, в котором используются специализированные инструменты во всех аспектах рецензии: сбор файлов, передача и отображение файлов, комментариев и дефектов для всех участников, сбор метрики и передача менеджерам по продукту и передача администраторам некоторого контроля над потоком операций.
Существует несколько основных элементов, которые должны присутствовать в инструменте рецензирования, если он должен решить большие проблемы с другими типами рецензий:
Автоматизированный сбор файлов
Как мы обсуждали при почтовом рецензировании, вы не может заставлять разработчиков тратить время на ручной сбор файлов и сравнений для рецензии. Инструмент должен интегрироваться с вашей системой управления версиями для извлечения текущих и предыдущих версии, чтобы рецензенты могли легко видеть рецензируемые изменения.
В идеале инструмент может сделать это и с локальными изменениями, которые не зарегистрированы в управлении версиями, и с уже зарегистрированными изменениями (например, по дате, метке, ветви или уникальному номеру изменения). Даже если Вы не делаете оба этих типа рецензирования сегодня, то вы скорее всего захотите эту опцию в будущем.
Объединенное отображение: Различия, комментарии, дефекты
Один из самых больших поглотителей времени в любом типе рецензирования находится в рецензентах и разработчиках, имеющих необходимость связать каждую свою беседу с определенным файлом и номером строки. Инструмент должен быть в состоянии отображать файлы и различия в файле перед/после изменений так, чтоб были видны потоки переговоров и никто не тратил много времени на комментарии, дефекты и исходный код.
Автоматизированный набор метрик
С одной стороны точные метрики — единственный метод понять ваш процесс и единственный метод измерить изменения, которые происходят при изменении процесса. С другой стороны, ни один разработчик не хочет рецензировать код с секундомером и инструментами учета количества строк.
Инструмент, который автоматизирует набор ключевых метрик, является единственным методом сделать разработчиков счастливыми (т.е. без дополнительной работы для них) и получить значимые метрики для вашего процесса. Полное обсуждение метрик рецензирования и что они означают это уже другая задача, но ваш инструмент должен, по крайней мере, собирать эти три уровня: количество строк/час (скорость инспекции), дефектов/час (скорость дефектообразования) и дефектов/количество строк (плотность дефектов).
Почти все другие типы рецензирования создают проблемы для менеджеров по продукту, которые не знают рассматривают ли разработчики все изменения кода или проверяют ли рецензенты, что дефекты действительно исправлены и не вызвали новые дефекты. Инструмент должен быть в состоянии осуществить этот поток операций, по крайней мере, на уровне создания отчетов (для пассивного контроля потока операций) и в лучшем случае на уровне управления версиями (с серверными триггерами, которые выполняют поток операций на уровне управления версиями).
Клиенты и интеграция
Некоторым разработчикам нравятся инструменты с командной строкой. Другие предпочитают интеграцию с редакторами и клиентами управления версиями. Администраторам нравятся веб-клиенты, которые нет необходимости устанавливать. Важно, чтобы инструмент поддерживал много способов чтения и записи данных в системе.
Инструменты разработчика также имеют свойство интегрироваться с другими инструментами. Клиенты управления версиями находятся внутри IDE. Контроль заданий коррелирует с изменениями в управлении версиями. Точно так же Ваш инструмент рецензирования должен интегрировать с другими вашими инструментами – все от клиентов управления версиями и IDE до метрик и отчетов. Бонус — инструмент, который представляет общедоступный API так, чтобы Вы могли сделать настройки и дополнительную интеграцию сами.
Если Ваш инструмент удовлетворит этот список требований, то Вы будете обладать преимуществами перед рецензированием через-почту (работа с несколькими, возможно удаленными разработчиками, минимизация прерываний), но без каких-либо проблем выполнения потока операций, сбора метрик, и напрасной траты времени для упаковки файлов/различий, поставки и контроля.
Недостаток любого инструментального рецензирования – цена, т.е. деньги для коммерческого инструмента или время, если разработка внутренняя. Вы также должны удостовериться, что инструмент достаточно гибок, чтобы обработать Ваш специфический процесс рецензирования кода; иначе Вы можете найти инструмент, который управляет Вашим процессом, а не наоборот.
Несмотря на то, что инструментальное рецензирование может решить проблемы, которые имеют типовые рецензии кода, есть все еще один метод, у которого, в то время как он не часто используемый, есть потенциал для поиска еще большего количества дефектов, чем у стандартного рецензирования кода.
Парное программирование
Большинство людей связывает парное программирование с XP и гибкой разработкой в целом, но это также процесс разработки, который включает непрерывное рецензирование кода. Парное программирование – два разработчика, пишущие код на одном рабочем месте только одним разработчиком в один момент времени и непрерывным обсуждением в свободной форме и рецензированием.
Исследования парного программирования показали, что оно очень эффективно и в поиске ошибок, и в стимулировании передачи знаний. И некоторые разработчики действительно любят его.
Однако тут есть спорный вопрос, лучше ли рецензии парного программирования, хуже или оно как дополнительное к более стандартным методам. Рецензент глубоко вовлечен в код, уделяя много внимания проблемам и последствиям, являющимся результатом различных реализаций. С одной стороны, это дает рецензенту много времени на инспекцию и глубокое понимание проблемы, поэтому возможно, это означает, что анализ будет более эффективным. С другой стороны, такая близость — это точно не то, чего вы хотите от рецензента; так же, как любой автор не видит всех опечаток в своей собственной работе, рецензент, который слишком близок к коду, и не может отстраниться и не критиковать ее с новой и непредвзятой позиции. Некоторые люди полагают использовать оба метода – парное программирование для глубокого анализа и последующее стандартное рецензирование для введения новых глаз. Несмотря на то, что это занимает много времени разработчика для его реализации, казалось бы, что такой подход обнаружит больше всего дефектов. Хотя мы никогда на практике этого не замечали.
Единственная самая большая жалоба на парное программирование это то, что оно занимает слишком много времени. Вместо того, чтобы привести рецензента на 15-30 минут посмотреть изменение, мы берем одного разработчика на несколько дней в парное программирование и вас теперь два разработчика на одной задаче на все время.
Некоторым разработчикам просто не нравится парное программирование, это зависит от расположения разработчика к его партнеру. Парное программирование также не решает проблему удаленных разработчиков.
Полное обсуждение за и против парного программирования в целом вне нашей темы.
Заключение
Каждый из пяти типов рецензирования полезен по-своему. Формальные инспекции и парное программирование — доказанные методы, но требуют большого количества времени разработчика и не работают с удаленными разработчиками. Рецензирование через-плечо является самым простым для реализации, но не может быть реализован как управляемый процесс. Рецензирование через-почту и инструментальное рецензирование давят на баланс между временем, которое инвестируют, и простотой реализации.
Как провести код-ревью: советы по применению
Разбираемся, кому и когда нужна (и не нужна!) дополнительная проверка кода, и каких ошибок избежать на старте.
Текст будет полезен разработчикам и лидам, которые еще близко не знакомы с код-ревью или хотят упорядочить свои знания, узнать лайфхаки из практики.
Что такое код-ревью и зачем его проводят?
Код-ревью — это процесс проверки кода, который позволяет:
выявить → ошибки, пропуски, уязвимости и стилистические недочеты (с точки зрения проекта или принятых в команде правил).
- улучшить →
→ читаемость и понятность кода для других разработчиков.
→ архитектурные решения. Например, разбиение на модули, code style решения, неверно подобранный паттерн проектирования.
→ работу в команде. Способствует диалогу между автором кода и ревьюером, дает возможность прокачать навыки и узнать что-то новое.
В код-ревью нет жестких правил о том, кто именно должен проводить проверку. Идеальный сценарий, когда ревью осуществляет более опытный коллега — например, тимлид или ведущий разработчик проекта. В реальности так выходит не всегда: часто мидл проверяет мидла.
Когда не нужно проводить код-ревью?
Системная практика оценки кода внутри команды — не всегда полезное с точки зрения затраты ресурсов решение. Оно отнимает время и у автора, и у проверяющего, а еще требует глубокого погружения в процессы и сил на коммуникацию.
Причины, чтобы отказаться от код-ревью в конкретном случае:
- нет человека, который обладает экспертизой в области специфики задачи;
- изменения слишком незначительны и не требуют проверки.
Причины, чтобы отказаться от постоянной практики код-ревью:
- у всех разработчиков одинаковый уровень знаний, навыков и уровень погружения в контекст;
- приняты другие практики проверки кода — например, парное программирование;
- постоянные конфликты из-за код-ревью в команде;
- практика уже показала свою неэффективность.
Как организовать процесс проверки кода?
- После выполнения задачи автор кода делает в отдельной ветке push в удаленный репозиторий и создает Merge Request (MR).
- Назначается assignee (ответственный за MR) и reviewer (ответственный за проверку).
- Затем начинается серия проверок, которая называется раундами. Цель каждой итерации — найти проблемы и недочеты, которые могут отразиться на работе проекта в будущем.
Раунды
Перед стартом ревьюер должен оценить объем MR и определить, сможет ли его проверить на «одном дыхании» — не теряя концентрации. Если объем MR слишком большой, советуем разбить его на части поменьше. Чем объемнее решение, тем ниже эффективность проверки.
Заряд батарейки — количество энергии участников ревью от раунда к раунду.
В первом раунде проверяющему важно оценить код на предмет высокоуровневых, глобальных проблем. Это, например, неверно выбранный подход к проектированию решения или разбиение на функции, отсутствие модульности.
Пример комментария.
Если таких проблем не нашлось, уровень проверки понижается до тех пор, пока не найдутся места для улучшений.
«В первом раунде не стоит акцентировать внимание на мелких недочетах. Скорее всего, автор сам их обнаружит и поправит, и ревьюеру не придется тратить время на поиск незначительных проблем», — отмечает разработчик Selectel Антон Щербак.
После код возвращается автору: он вносит изменения и снова отправляет на проверку. Процесс продолжается до тех пор, пока решение не признают приемлемым — оно одобряется тем, кто проводил ревью, и после assignee выполняет merge.
«Решение не должно быть идеальным — оно должно соответствовать потребностям проекта и выполнять поставленную задачу», — резюмирует Антон Щербак.
Как понять, что решение готово?
→ Оно не имеет явных ошибок.
Ревью может выявить недочеты, которые видны без запуска проекта. Обычно это:
- опечатки;
- ошибки, возникшие из-за недостаточного погружения в контекст проекта;
- необновленный конфигурационный файл.
→ Соответствует стайл-гайдам, принятым в команде.
В команде должен быть принят свод правил, по которым ведется разработка ПО. Он нужен для того, чтобы соблюдался единый стиль и было проще разобраться в контексте.
Если автор решения выходит за рамки принятых стайл гайдов или отклоняется от них, стоит указать ему на это.
→ Не затрагивает код, выходящий за рамки задачи.
Решение должно быть выполнено в рамках скоупа задачи. Если разработчик заметил, что можно выполнить рефакторинг другого класса, это лучше сделать в отдельном MR. При таком подходе ревьюеру будет проще проверять выполнение исходной задачи.
→ Пропущено через линтеры, используемые в проекте.
Большинство команд в Selectel использует pre-commit — так при каждом коммите код прогоняется через линтеры. Для Python используется black, isort, flake8, pyupgrade и autoflake.
→ Обладает хорошей читаемостью и пояснением неочевидных мест.
Лучшая практика в разработке — написание самодокументируемого кода. Если по каким-то причинам ей воспользоваться не получается, советуем оставить поясняющий комментарий: он расскажет ревьюеру, что происходит в этом месте проекта.
Но увлекаться комментариями не стоит: их количество увеличивает объем текста, который нужно будет прочитать другим разработчикам.
→ Если на проекте пишутся автотесты, решение должно ими покрываться.
Команда принимает решение об использовании автотестов для увеличения надежности сервиса. Если эта практика уже используется, ее стоит поддерживать. При выпуске патчей иногда нужно чуть переписать тест, а при минорных версиях — всегда написать новые.
→ Нет оверинжениринга (кода «на будущее»).
Часто код, который решает еще не возникшие проблемы, не пригождается и становится лишним.
Еще Дональд Кнут говорил: преждевременная оптимизация — корень всех проблем в программировании.
Советы по процессу
Первый: договоритесь о сроках и организации процесса. Например, не отдавайте правки по одной — это отвлекает. Еще не затягивайте процесс на весь день и по возможности не переключайтесь между задачами — это время- и энергозатратно.
Хорошая практика — восприятие код-ревью как отдельной задачи без переключения на другие. Правда при условии, что у остальных дел невысокий приоритет.
Второй: используйте префикс NIT (сокращение от nitpick) для комментариев, которые даны в качестве рекомендаций. Помните: они не обязательны к исполнению, автор волен сам решать, следовать им или нет.
Пример комментария.
«Разработчики часто спорят о названиях переменных. Бывает, что эти нововведения никак не влияют на итоговое решение. Поэтому делать пометку в начале — нормальная практика: она говорит разработчику, что решение о внесении в код изменений остается за ним. Эти изменения не обязательны — решение и так хорошее», — рассказывает разработчик Selectel Антон Щербак.
Третий: помните о том, что обратная связь должна быть балансной. Ее цель — не обидеть человека, а аргументировано (например, с помощью примеров кода и ссылок на паттерны) подсветить зоны для улучшений.Кроме того, не зацикливайтесь только на ошибках: если видите интересное решение или нестандартный подход, похвалите его. Так лишний раз покажите коллеге, что у вас одна цель, и снимите напряжение.
Так не стоит комментировать → «Ты тут ошибся, поправь по моему примеру. Вот ссылка».
Так лучше → «Мы уже сталкивались с аналогичной ситуацией. Вот как эта проблема решилась тут → ссылка. Можем ли реализовать нечто подобное?»
Четвертый: количество раундов в код-ревью не ограничено, но лучше стремиться к сокращению их количества. Каждая новая серия правок отнимает силы участников ревью и тормозит релиз.
Удобные инструменты для код-ревью
-
— используем в Selectel.
По ссылке конкретные гайдлайны, которыми пользуются в GitLab. В них описано, как построена практика код-ревью в компании.
Создание правильного чек-листа для инспекции кода
Инспекция кода — это неотъемлемая часть процесса разработки, придуманная для снижения технических недоработок и обеспечения постоянства кодовой базы. Все, кто пишет код, допускают ошибки. Поэтому крайне важно отловить эти ошибки до выхода рабочей версии и возникновения ошибок.
Гарантом обеспечения предельно простой и эффективной инспекции кода является создание чек-листа, в который попадет все, что кажется вам важным для надежности кода. В чек-лист можно включить соответствие лучшим практикам, правильность форматирования кода, актуальность проводимых тестов и версий, а также реализация вашей командой правил и политик написания кода. При использовании чек-листов у вас и команды появится кодифицированная отметка о качестве кода, что упросит и конкретизирует процесс инспекции.
Не существует какого-то единого стандарта для чек-листов. У каждого разработчика свои требования к коду, поэтому, пожалуйста, не относитесь к данной статье, как к единственно верному руководству. Мы писали ее только для того, чтобы показать вам, что именно мы считаем важными составляющими чек-листов. Используйте эти критерии при создании собственного списка.
Отправная точка
Для начала определитесь с главными элементами чек-листа. В первую очередь проверьте реализацию кода и убедитесь в том, что свои задачи он выполняет лаконично и эффективно. Если вам кажется, что существует более короткое решение, или же реализация выглядит не столь эффективной, то внесите нужные правки до того, как пойдете дальше по списку.
Удаляйте ненужный код, которым пользовались в прошлых версиях и сборках. Так вы получите «чистую» базу кода, понятную для команды, а заодно ускорите процесс отладки.
Стоит также подумать об анализе общей производительности кода. Например, есть ли библиотеки, которыми можно заменить функции или улучшить код? А, может, найдутся другие решения, способные сократить количество дополнительных функций? Проверьте, не остался ли код с фазы отладки (например, логи или старые функции) и удалите его — так вы максимально ускорите код.
Дублирование кода
Эта проблема куда более частая, чем кажется. Поищите копии кода, выполняющие одно и то же в разных функциях. При наличии дублей, создайте для них собственный метод. Данное решение подходит только для двух экземпляров. Поэтому если один и тот же код с аналогичными функциями повторяется в пяти разных файлах, то стоит преобразовать его в единую и унифицированную функцию.
Оформление
Код должен быть не просто эффективным, но и понятным для других — так, чтобы новый разработчик с легкостью разобрался в назначении каждой строки кода. Возможно, в дальнейшем ваш код потребуется изменить, удалить или провести рефакторинг. Читабельность и понятность важны для того, чтобы разработчик мог работать с кодом, не задавая вам лишних вопросов.
Ряд вопросов, которые необходимо задать себе при форматировании кода:
- Насколько осмыслены имена переменных?
- Есть ли в коде комментарии, разъясняющие сложные функции?
- Есть ли в коде длинные методы с многочисленными ответвлениями, которые можно вынести в более простой метод?
- Соблюдаете ли вы установленные в группе правила отступов и табуляции, правильно ли вы пользуетесь пробелами?
Безопасность
Безопасность в разработке программ должна стоять на первом месте. Это особенно важно при работе с личной информацией или сторонними библиотеками/утилитами. Если пользователь делится конфиденциальной информацией (например, паролями), то она должна шифроваться и проходить все нужные проверки. Кроме того, API-ключи, сеансовые идентификаторы и средства авторизации должны реализовываться в безопасном режиме. Если пользуетесь OAuth, то обязательно следуйте его стандарту.
Тестирование
По мере разрастания кодовой базы, тестирование (особенно нагрузки и производительности) становится все более значимой процедурой для проверки функциональности и предельной эффективности кода. Попробуйте провести тестирование с непрерывной интеграцией и удостоверьтесь в том, что эти тесты пройдены. Подумайте, нужно ли добавлять в текущие средства тестирования дополнительную проверку, и действительно ли они проверяют всю кодовую базу. Проверяют ли модульные тесты нужные функции? Охватывает ли тестовый фреймворк все потенциальные примеры использования?
Важность ревизии
Правила оформление кода внутри команды могут периодически меняться, поэтому чек-лист должен подстраиваться под эти изменения. Не забывайте дорабатывать чек-лист по мере разрастания кода и наработки опыта для предельного охвата всех областей кода. Если вы реализовали новую процедуру линтинга или добавили процесс тестирования, то включите их проверку в чек-лист.
Планирование и создание собственного метода инспекции кода занимает много времени. Ваше время крайне ценно, поэтому инспекцию кода можно отдавать на аутсорсинг. Так вы будете меньше беспокоиться о качестве кода, и получите больше времени на написание кода и поставку продукта — от этого выиграет и команда, и компания!
Надеемся, данный чек-лист помог вам лучше разобраться в процессе инспекции кода. Но помните, что этот список не является универсальным. И последний совет: не забывайте дорабатывать чек-лист под свои нужды. Успехов в разработке!