Code review что это
Code review что это
Что такое код-ревью и кто им занимается?
Эффективный способ заботиться о качестве кода
Проверка кода — это как базовое правило гигиены. Разработчики могут создавать запутанный и тяжелый код. Его сложнее обслуживать, а сбои появляются там, где не ждешь. Поэтому важная часть работы над продуктом — код-ревью, когда более опытные разработчики проверяют качество кода. Мы поговорили с Андреем Строговым, который руководит код-ревьюерами в Яндекс.Практикуме, о главных качествах такого профессионала и бесполезных комментариях к коду.
Задачи код-ревьюера
Код-ревью — это этап разработки кода. Чаще всего его проводят другие разработчики из той же команды. Так более опытные кодеры контролируют качество работы джуниоров или стажеров. Ревьюер на отдельных компонентах может показать, как сделать код проще и понятнее. Например, предложит взять функцию, которая уже написана для другого фрагмента. Проверка кода особенно важна для работы больших команд.
«В масштабных проектах код очень объемный и каждый разработчик знает только свой фрагмент. Люди часто не в курсе, что происходит в других компонентах и модулях. Это не слишком устойчивая ситуация, потому что автор кода может уйти в отпуск или по разным причинам перестать поддерживать свой фрагмент. Этап код-ревью добавляет второго человека, который понимает код и может с ним работать», — говорит руководитель команды код-ревью Андрей Строгов.
Код-ревью — это хороший способ договориться внутри команды, как писать код. Например, запутанный код сложно поддерживать в рабочем состоянии и масштабировать. Этап код-ревью помогает обмениваться знаниями, находить новые решения, делать лучше весь процесс разработки.
В отличие от тестирования, на код-ревью важнее разобраться в логике решения, чем найти ошибки. А еще — донести суть проблемы до разработчика. Для этого понадобится умение точно формулировать проблему и сообщать о ней без лишних эмоций.
«Когда мы проверяем код, не надо тратить время на мелкие ошибки — названия переменных, опечатки. Это плохо влияет и на того, кто пишет код, и на проверяющего. В первую очередь автору нужна обратная связь по логике кода. Проверку мелких ошибок легко автоматизировать», — говорит Андрей Строгов.
Этапы работы и инструменты
Код-ревьюер движется от общего к частному. Сначала ему нужно понять, какую задачу решал автор кода. Для этого проверяющий смотрит техническое задание и уточняет детали у разработчика. Дальше нужно оценить архитектуру кода и посмотреть, грамотно ли он написан. Это самый ценный этап код-ревью, он помогает избежать грубых ошибок и сэкономить время команде тестирования.
Когда ревьюер разобрался с задачей и логикой решения, он смотрит на функции, отдельные алгоритмы и их эффективность. Проверяет, можно ли заменить их другими методами и будет ли это лучше для всего продукта.
«На этих этапах не нужно никаких специальных инструментов. Только экспертные навыки и знание задачи. Код-ревьюеру понадобятся некоторые инструменты среды лишь для того, чтобы посмотреть, как работает код, и обнаружить грубые ошибки», — говорит Андрей Строгов.
После проверки ревьюер оставляет комментарии для разработчика. Его задача на этом этапе — объяснить, почему важно исправить ошибку. А еще проверяющий может подсказать решение или дать ссылки на материалы, с которыми разработчик быстрее приведет код в порядок. Весь процесс проверки не должен тормозить остальную разработку — хорошо, если код-ревью удается закончить за время рабочего дня.
«Наша задача в том, чтобы разработчик понял, в чём заключается комментарий и почему важно исправить код в соответствии с ним. Для этого недостаточно сильных технических знаний, нужны хорошие soft skills. Если ревьюер дал полезный комментарий, а разработчик почему-то не захотел исправлять — это будет выглядеть глупо», — говорит Андрей Строгов.
Полезный комментарий помогает исправить и улучшить код. А еще в нём нет субъективной оценки или нотаций. Поэтому критически важно, чтобы код-ревьюер умел давать качественную обратную связь. Иначе автор примет замечания на свой счет, и никакой эффективной работы не получится.
«Не стоит оставлять комментарии в духе “бред какой-то” или “тут ты не подумал”. Нужно формулировать проблему корректно. Субъективное мнение тоже не помогает улучшить код. Лучше найти что-то точнее, чем “это непонятный код”», — говорит Андрей Строгов.
Кроме комментариев код-ревьюер может оставлять рекомендации. Это некритичные замечания, их можно взять в работу, если у разработчика останется время. Например, проверяющий видит, что код выглядит запутанным. Задача решена, и весь функционал в порядке, но кажется, что работа сделана небрежно. Тут можно обойтись рекомендацией — обратить внимание человека на эту особенность его решения.
«Вы сэкономите время команде, если выделите критичные замечания. Это замечания, касающиеся фрагментов, которые могут привести к некорректной работе кода или помешают расширить его в будущем. Еще сюда относятся ошибки, из-за которых код трудно поддерживать и редактировать», — говорит Андрей Строгов.
Знания и навыки
Чтобы проверять код, нужно в нём разбираться. Хорошо, чтобы ревьюер уже решал такие задачи, писал подобный код и был знаком с тем стеком технологий, который используют в команде. Тогда ревьюер сможет дать разработчику полезные комментарии.
«Нужно отучить себя от того, что ты обязательно должен написать комментарии после ревью. Если с кодом всё в порядке, он может вернуться к автору без замечаний, которые оставляют ради самих замечаний», — говорит Андрей Сторогов.
Важно смотреть на код с позиции автора. У ревьюера может быть свой способ работы с кодом или другое решение для конкретной задачи. Но вся ценность его работы — предложить улучшения, ориентируясь на методы работы автора кода.
«Для команды хорошо, когда ревьюер может искренне похвалить удачное решение, — говорит Андрей Строгов. — Разработчики делятся на две категории. Одни считают, что пишут идеальный код, другие — что их код плох. Поэтому важно научиться искренне хвалить за хорошие решения. Это приятно автору кода и укрепляет отношения в команде».
Стать хорошим ревьюером помогает практика. Если в вашей команде пока нет такой деятельности, можно искать подходящие проекты на GitHub и оставлять комментарии там. «Код-ревью влияет на качество кода уже самим фактом своего существования, —говорит Андрей Строгов. — Когда знаешь, что твой код посмотрят, тщательнее к нему относишься. Например, постараешься его понятно оформить, не будешь использовать запутанную логику, в которой не смог бы разобраться другой разработчик. Это полезная социальная составляющая, которая мотивирует делать более понятный код».
Вещи, которые должен делать каждый: Code Review
Самая значимая вещь, которая делает код в компании Google таким хорошим проста — code review (далее CR). Google не единственная компания, использующая CR. Всем известно, что это хорошая идея и множество разработчиков делают это. Но я не видел ни одной другой большой компании, в которой CR был бы так грамотно внедрен. В Google ни одна линия кода не уходит в production пока не получит позитивную оценку на CR.
Вы должны уделять внимание CR. Это непременное условие для разработки любого программного обеспечения на серьезном уровне. CR должен применяться не только для кода который уходит в production, но вообще для всего. Это не такая большая разница во времени как вы думаете, но разница в качестве получается огромна.
Что же вы получите от использования CR?
Очевидное: одна голова хорошо, а две лучше. Как минимум еще один человек просмотрит ваш код перед тем, как он будет проверен в реальных условиях (баги). Это наиболее часто упоминающийся профит от CR. Но мой опыт говорит, что это как раз менее всего ценно. Коллеги действительно находят ошибки в вашем коде, но подавляющее большинство таких ошибок найденных в процессе code review, прямо говоря, тривиальные вещи, которые автор кода найдет за несколько минут. Баги, на которые действительно нужно потратить время не ловятся на CR.
Самое большая польза от code review носит социальный характер. Если вы программист и вы знаете, что ваши коллеги обязательно посмотрят ваш код, вы размышляете по-другому. Вы будете писать аккуратный, хорошо документированный и организованный код, потому как вы знаете, что люди, мнение о коде которых вам важно, внимательно проверят отправленный на review код. Без CR, вы также знаете, что другие люди посмотрят ваш код. Но вы понимаете, что это произойдет не сразу, поэтому этот факт не оказывает такого подсознательного эффекта.
Другое большое преимущество — это распространение знаний. Во многих командах, у каждого отдельного программиста есть кусок кода в проекте, за который он ответственен, и этот программист сфокусирован именно на этот код. Пока коллеги не поломают что-то связанное с этим кодом, они на него не обращают внимания. Побочный эффект от этого в том, что для каждого компонента существует только один человек, который полностью понимает как он работает. Если этот человек не укладывается в сроки или — упаси Боже — покидает компанию, не останется никого, кто знаком с кодом компонента. С CR будет как минимум два человека, которые знакомы с кодом — автор и reviewer. Reviewer не знает код так, как его знает автор, но он знаком со структурой и архитектурой кода, что очень значимо.
Конечно же, это не просто. Из моего опыта могу сказать, что нужно время чтобы наладить хороший процесс CR. Я знаю несколько подводных камней, которые могут вызвать проблемы. Пока эти камни периодически вылезают CR может оставаться на плохом уровне, быть в тягость и препятствовать принятию в ежедневный workflow.
Главное правило — целью CR является нахождение проблем в коде перед тем как он будет закоммичен, т.е. правильность. Самая распространенная ошибка в CR у новичков это проверять код, основываясь на том как бы это сделал он. Обычно существует несколько путей решения одного и того же. Помимо этого, есть миллион способов написать задуманное в виде кода. Задача reviewerа, не сделать так, чтобы написанный код был похож на то, чтобы он написал, нет — он никогда не будет таким. Задача reviewerа — удостовериться в том, что код, написанный автором корректен. Когда нарушается это правило, вы в итоге придете к эмоциям и переживаниям, что не является тем, к чему вы стремитесь.
Дело в том, что это совершенно естественная ошибка. Если вы программист, когда вы смотрите на проблему в коде, вы можете сразу видеть способ ее решить и вы думаете, что то, что вы увидели и есть решение. Но это не так, и чтобы стать хорошим reviewerом нужно это понять.
Второй по значимости подводный камень то, что люди чувствуют необходимость сказать что-либо. Вы знаете, что автор потратил достаточно времени и сил, работая над кодом и вроде как вы должны высказать свое мнение.
Вполне нормально просто сказать «Выглядит хорошо». Если вы постоянно пытаетесь найти что-нибудь покритиковать, тогда все чего вы добьетесь так это подорвать свой авторитет и поломать отношения в команде. Когда вы критикуете код только для того чтобы найти что сказать, тогда люди чей код вы проверяете поймут, что вы пишете только для того чтобы что-то написать и ваши комментарии не будут воспринимать всерьез.
Третье — скорость. Не нужно сломя голову бежать проверять только что посланный на review код, но с другой стороны нужно делать это быстро. Ваши коллеги ждут вас. Если вы и ваши коллеги не уделяете время тому, чтобы CR был сделан, причем сделан быстро, тогда CR может стать причиной недовольства людей в коллективе. Может показаться, что это переключение фокуса — взяться за CR. Это не совсем так. Не нужно бросать все в тот момент, когда новый код послан на review. Но в течение нескольких часов вы совершенно точно сделаете паузу для того, чтобы попить что-нибудь, сходить в туалет или просто походить. Когда вы вернетесь с паузы уделите внимание CR. Если вы возьмете это в привычку, то никто в команде не будет подолгу ожидать ваш review и он не будет доставлять никакого дискомфорта в работе.
Code Review – зачем и как использовать в команде?
Что такое Code Review
Зачем нужен Code Review
Code Review может являться частью процесса выполнения задачи (частью workflow). Может показаться, что ревьювить должен только тимлид или старший разработчик, но хорошей практикой является если в процессе ревью задач участвуют все разработчики. Таким образом можно не только распределить нагрузку от ревью, но и составить у команды более широкое представление о выполняемых задачах. Также это помогает делиться best practices внутри команды.
Положительные эффекты в команде от Code Review:
понижает bus factor: больше людей в команде в курсе выполняемой задачи, в случае необходимости внесения изменений в задачу как минимум два человека смогут это сделать. Задача больше не завязана на одного разработчика.
помогает найти и выявить баги и недоработки на этапе разработки задачи: так как задача сразу проверяется как минимум двумя разработчиками, это повышает вероятность нахождения упущенных кейсов, которые без код ревью могли бы попасть на бой.
обучаемость сотрудников: разные реализации и подходы к решению задач могут заимствоваться участниками команды друг у друга во время код ревью
развитие и поддержание здоровой культуры в команде: участники команды учатся друг у друга и учатся давать качественную обратную связь, повышается взаимодействие внутри команды.
при разработке задачи на реализацию тратится чуть больше времени
в задаче задействованы как минимум два разработчика (тот, кто делал задачу и тот, кто ее ревьювил)
Рекомендации по организации Code Review
Code Review может быть организован по-разному в разных командах. Главное, чтобы команда заранее обговорила и утвердила свои внутренние правила, которых она хочет придерживаться и с которыми все согласны, чтобы каждый раз не возвращаться к этому вопросу.
Избегать рутинных проверок Code Style людьми: автоматизировать такие проверки. Можно использовать для этого любые подходящие вам инструменты для автоматической проверки code style используемого вами языка программирования. Например, для PHP это может быть PHP Coding Standards Fixer https://cs.symfony.com/ Не нужно тратить время разработчиков на то, что можно автоматизировать. Также обратная связь по поводу code style от людей воспринимается как “придирки” и может создать не очень позитивную атмосферу в команде.
Code Review должен проводиться для каждого участника команды, вне зависимости от уровня. Не должно быть такого, что ревьювят только задачи, которые сделали Junior разработчики, тем временем Senior разработчики не отдают свои задачи на ревью. Необходимо, чтобы ревью проводилось для задач всех разработчиков.
Code Review является частью процесса и необходим каждой задаче. Это правило избавляет от лишних споров и холиваров насчет небольших задач. Ревью проходят все задачи без исключений.
Code Review проводится перед релизом задачи и перед передачей ее в тестирование. Это помогает избегать повторного тестирования, а также соблазна оставить все как есть, ведь “и так работает”. К задачам, которые уже на бою, никто не захочет повторно возвращаться.
Избегать слишком больших объемов кода в одном Code Review. Если задача большая, то необходимо отправлять ее на ревью частями. Есть рекомендуемое число 200-400 строк в одном ревью. При увеличении количества строк, эффективность и продуктивность ревью резко падает.
Если нет возможности внести какие-то правки после ревью, то необходимо завести задачу в трекере задач, а в коде оставить ToDo с ее номером
Чего следует избегать:
Если Code Review непостоянная часть процесса разработки, то это приведет к нестабильному ревью, его будут откладывать и команда не получит всех плюсов этого процесса.
Старшие разработчики рвьювят новых и младших разработчиков. Это создает плохую культуру в команде, а также не позволяет младшим разработчикам увидеть какие-то решения старших разработчиков, на которых они могли бы поучиться и узнать что-то новое.
Не автоматизировать процесс ревью и не использовать сторонние инструменты для ревью. Никто не любит заниматься рутинными процессами. Нужно автоматизировать все, что можно автоматизировать.
На что обращать внимание во время Code Review
Чеклист для разработчика перед отправкой на ревью:
Проверить, что реализация соответствует всем указанным в исходной задаче условиям
Проверить соответствие Code Style и другим принятым в команде гайдлайнам, например, наличию unit-тестов и документации
Протестировать задачу локально и убедиться, что она работает, как нужно
Подготовить описание для ревьювера, если какой-то информации в задаче не хватает
Проверить, нужны ли какие-то комментарии в самом коде и добавить при необходимости
Чеклист для ревьювера:
Ознакомиться и понять цель и суть задачи
Проверить общую архитектуру и подход к решению
Проверить мелкие детали (имена функций и переменных и т.д.)
Проверить наличие тестов и документации по необходимости
Список ToDo: изменения, которые необходимо внести в код после ревью
Вопросы: обозначить свои вопросы по частям кода, которые непонятны после ревью
Предложения по улучшению: внести свои предложения и пожелания по коду задачи и/или связанных задач. Например, договориться о создании задачи по обновлению старого метода в других участках кода на новый и завести на это ToDo и задачу в трекере задач и поставить ее в тех. долг команды.
Также отдельно хочется отметить, что если вы ревьювите чью-то задачу и видите какие-то хорошие подходы и решения, то скажите об это автору. Положительная обратная связь тоже очень важна.
Инструменты для Code Review
Поищите инструменты для вашего языка программирования. Используйте тот, который больше всего подойдет вашей команде.
Код-ревью для начинающих: советы и ориентиры из практики
Привет, Хабр! Сегодня публикуем не совсем обычный для нас текст: решили сделать полезный гайд для новичков в код-ревью. Разобрались, кому нужна и не нужна эта практика, а еще — каких ошибок лучше избежать на старте.
Текст пригодится разработчикам и лидам, которые еще близко не знакомы с код-ревью или хотят упорядочить свои знания, узнать лайфхаки из практики. Если вы тоже захотите внести свою лепту, что-то посоветовать или рассказать историю из опыта — пишите в комментариях.
Что такое код-ревью и зачем его проводят?
Код-ревью — это процесс проверки кода, который позволяет:
выявить → ошибки, пропуски, уязвимости и стилистические недочеты (с точки зрения проекта или принятых в команде правил).
→ читаемость и понятность кода для других разработчиков.
→ архитектурные решения. Например, разбиение на модули, 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
Помните о том, что обратная связь должна быть балансной. Ее цель — не обидеть человека, а аргументировано (например, с помощью примеров кода и ссылок на паттерны) подсветить зоны для улучшений.
Кроме того, не зацикливайтесь только на ошибках: если видите интересное решение или нестандартный подход, похвалите его. Так лишний раз покажите коллеге, что у вас одна цель, и снимите напряжение.
Так не стоит комментировать → «Ты тут ошибся, поправь по моему примеру. Вот ссылка».
Так лучше → «Мы уже сталкивались с аналогичной ситуацией. Вот как эта проблема решилась тут → ссылка. Можем ли реализовать нечто подобное?»
Количество раундов в код-ревью не ограничено, но лучше стремиться к сокращению их количества. Каждая новая серия правок отнимает силы участников ревью и тормозит релиз.
Удобные инструменты для код-ревью
GitLab — используем в Selectel.
По ссылке конкретные гайдлайны, которыми пользуются в GitLab. В них описано, как построена практика код-ревью в компании.
GitHub. Подробнее об инструменте — по ссылке.
Больше текстов про развитие карьеры читайте в блоге Selectel. А еще подписывайтесь на наш Телеграмм-канал с вакансиями и полезными карточками про soft и hard skills.
Code review по-человечески (часть 1)
В последнее время я читал статьи о лучших практиках code review и заметил, что эти статьи фокусируются на поиске багов, практически игнорируя другие компоненты ревью. Конструктивное и профессиональное обсуждение обнаруженных проблем? Неважно! Просто найди все баги, а дальше само сложится.
Так что у меня случилось откровение: если это работает для кода, то почему не будет работать в романтичных отношениях? Итак, встречайте новую электронную книгу, которая поможет программистам в отношениях со своими возлюбленными (обложка на иллюстрации слева).
Моя революционная книга обучит вас проверенным техникам по выявлению максимального количества недостатков в своём партнёре. Книга не затрагивает следующие области:
• Обсуждение проблем с сочувствием и пониманием.
• Помощь партнёру в устранении недостатков.
Насколько я могу понять из чтения литературы по code review, эти части отношений настолько очевидны, что вообще не стоят обсуждения.
Как вам нравится такая книжка? Предполагаю, что она вам не очень по душе.
Так почему мы проводим code review таким образом?
Могу лишь предположить, что прочитанные мною статьи — из будущего, где все разработчики являются роботами. В этом мире ваши коллеги приветствуют беспечную критику своего кода, потому что обработка такой информации согревает их холодные металлические сердца.
Я собираюсь сделать смелое предположение, что вам хотелось бы улучшить code review в настоящем, где вы работаете с людьми, а не с роботами. Даже более смелое предположение, что хорошие отношения с коллегами — это главная цель, а не просто переменная для ускорения исправления ошибок (минимизации параметра cost-per-defect). Как бы изменились ваши методы ревью с учётом этих обстоятельств?
В этой статье обсудим техники, которые предполагают, что code review — не только технический, но и социальный процесс.
Что такое code review?
Термин “code review” может означать разные действия, от простого чтения какого-то кода из-за спины разработчика до совещания на 20 человек, где вы разбираете код строчка за строчкой. Я здесь имею в виду формальную и письменную процедуру, но не отягощённую рядом совещаний для инспекции кода.
В код ревью принимают участие автор, который написал код и отправил его на ревью, и рецензент, который анализирует код и принимает решение, когда тот готов для добавления в общую кодовую базу проекта. В процессе может участвовать несколько рецензентов, но для простоты предположим, что он один.
Перед началом ревью автор должен создать список изменений, где перечисляет все изменения, которые хочет внести в общую кодовую базу.
Ревью начинается, когда автор отправляет список изменений рецензенту. Оно происходит в несколько раундов. Каждый раунд — это один полный цикл приёма-передачи между автором и рецензентом: автор присылает изменения, рецензент отвечает отзывом на эти изменения. Каждая рецензия кода состоит из одного или нескольких раундов.
Ревью завершается, когда рецензент одобряет изменения. Обычно этому сопутствует отправка сообщения LGTM, сокращённой фразы “looks good to me”.
Почему это так трудно?
Если программист присылает вам список изменений, которые он считает потрясающими, а вы отвечаете ему подробным перечислением причин, почему он не прав, такое общение требует определённой деликатности.
«Это одна из причин, почему я не скучаю по IT, ведь программисты — весьма малопривлекательные люди… Например, в авиации те, кто слишком переоценивают свои способности, уже мертвы».
Филип Гринспан, сооснователь ArsDigita, из книги «Основатели за работой».
Автору очень легко воспринять критику своего кода в том смысле, что он некомпетентный программист. Рецензия кода — это возможность обмениваться знаниями и принимать осмысленные инженерные решения. Но это невозможно, если автор воспринимает обсуждение как персональную атаку на него самого.
Как будто и без этого недостаточно сложностей, здесь вами приходится выражать свои мысли в письменной форме, что ещё больше повышает риск недопонимания. Автор не слышит тон вашего голоса, не может воспринять язык тела, поэтому так важно аккуратно подбирать формулировки. Для автора, который занял оборонительную позицию, безобидное примечание вроде «Ты забыл закрыть файловый дескриптор» может звучать как «Не могу поверить, что ты забыл здесь закрыть файловый дискриптор! Ты такой идиот!»
Техники
Отдайте компьютерам скучную часть работы
Когда вас постоянно отвлекают совещания и почтовые письма, трудно выкроить время для вдумчивого анализа кода. Ментальных сил ещё меньше, чем времени. Чтение кода коллеги — когнитивно тяжёлая задача и требует высокого уровня концентрации. Не транжирьте эти ресурсы на задачи, которые может выполнить компьютер, особенно если он выполняет их лучше.
Очевидный пример — ошибки с пробелами в отступах. Сравните, сколько усилий требует выявление такой ошибки от человека, по сравнению с применением автоматического инструмента форматирования:
Правая часть пустая, потому что автор использовал редактор кода, который автоматически форматирует пробелы каждый раз при нажатии кнопки «Сохранить». Ну худой конец, когда автор отправляет свой код на проверку, система непрерывной интеграции сообщает о неправильных пробелах. Автор исправляет проблему ещё до того, как рецензент её заметил.
Посмотрите на другие механические действия, которые можно автоматизировать, вот наиболее часто встречающиеся:
Задача | Автоматизированное решение |
---|---|
Проверить билды | Система непрерывной интеграции, такая как Travis или CircleCI. |
Проверить прохождение автоматических тестов | Система непрерывной интеграции, такая как Travis или CircleCI. |
Проверить, что отступы и пробелы соответствуют корпоративному стилю. | Инструмент форматирования кода, такой как ClangFormat (для C/C++) или gofmt (для Go). |
Идентификация неиспользуемых модулей или неиспользуемых переменных | Статические анализаторы кода, такие как pyflakes (линтер для Python) или JSLint (линтер для JavaScript). |
Автоматизация помогает вам как рецензенту внести более ценный вклад. Если игнорировать целый класс проблем, вроде порядка импорта модулей или соглашения о присвоении имён для файлов исходников, то вы можете сконцентрироваться на более интересных вещах, таких как функциональные ошибки или недостаточная читаемость кода.
Автоматизация выгодна и автору. С её помощью он может обнаружить небрежные ошибки за несколько секунд, а не за несколько часов. Мгновенный фидбек ускоряет обучение и упрощает исправление ошибки, потому что релевантный контекст у автора ещё в рабочей памяти. Кроме того, автор гораздо легче воспринимает сообщение о глупой ошибке от компьютера, а не от вас.
Нужно всем вместе поработать, чтобы внедрить автоматические проверки такого рода в рабочий процесс code review (например, хуки перед коммитами в Git или вебхуки в GitHub). Если процесс ревью требует от автора запускать такие проверки вручную, то вы лишаетесь большей части преимуществ. Автор неизбежно иногда будет забывать о проверке, из-за чего вам придётся возиться с с простыми ошибками, которые могла исправить автоматическая проверка.
Оформите аргументы по стилю в виде руководства по стилю
Обсуждение стиля — это потерянное время в процессе код ревью. Разумеется, единообразный стиль важен, но обсуждение кода — неподходящее время для прений, где поставить фигурные скобки. Лучший способ исключить споры и стиле из всего процесса — завести руководство по стилю.
Хорошее руководство по стилю определяет не только внешние элементы оформления вроде соглашения о присвоении имён или правила отступов, но и как использовать функции данного языка программирования. Например, JavaScript и Perl набиты функциональностью — там есть много вариантов реализации одной и той же логики. Руководство по стилю определяет Один Правильный Способ программирования, так что вы больше не увидите, что одна половина вашей команды использует один набор функций языка, а другая половина — совершенно другой набор функций.
Как только у вас появилось руководство по стилю, больше не придётся тратить циклы ревью на обмен сообщениями с автором в спорах, каким способом лучше именовать файлы. Просто следуйте руководству и двигайтесь дальше. Если в вашем руководстве отсутствует инструкция по определённому вопросу, обычно и не стоит о нём спорить. Если вы встречаетесь с вопросом по стилю, который не описан в руководстве, но достоин обсуждения, решите его вместе с командой. Затем запишите решение в руководство по стилю, чтобы больше никогда не возвращаться к этому обсуждению.
Вариант 1: адаптация существующего руководства по стилю
Если поискать в интернете, то можно найти опубликованные руководства по стилю — и позаимствовать их для собственного использования. Самые известные — руководства по стилю от Google, но можно найти и другие, если эти не подходят. Адаптируя существующий документ, вы получаете все выгоды от наличия руководства по стилю, не тратя времени на написание такого документа с нуля.
Недостаток в том, что все организации адаптируют эти правила для внутренних нужд. Например, руководства Google очень консервативны относительно использования новых функций языков, потому что у них огромная кодовая база, которая должна работать на всём: от домашнего маршрутизатора до последнего iPhone. Если у вас стартап из четырёх человек с единственным продуктом, то логично выбрать более агрессивную стратегию в использовании самых последних функций или расширений языков.
Вариант 2: Постепенно дополняйте собственное руководство по стилям
Если не хотите адаптировать существующий документ, можно создать свой собственный. Каждый раз, когда во время код ревью возникает дискуссия по стилю, поднимайте перед всей командой вопрос, каким должно быть официальное соглашение. Когда достигнете согласия, закрепляйте это решение в руководстве по стилям.
Я предпочитаю держать наше руководство в формате Markdown в системе контроля версия (например, на GitHub Pages). Так все изменения проходят через стандартную процедуру рецензирования — кто-то должен явно одобрить изменения, а каждый в команде может поднять любую проблему. Вики и Google Docs тоже подходят.
Вариант 3: Гибридный подход
Сочетая варианты 1 и 2, вы можете адаптировать существующее руководство и одновременно вести локальный документ для расширения или перезаписи базы. Хороший пример — руководство Chromium C++. Там в качестве базы взяли руководство по C++ от Google, но сделали собственные изменения и дополнения.
Начинайте ревью немедленно
Расценивайте code reviews как задачу с высоким приоритетом. Когда вы изучаете код или пишете отзыв, не торопитесь, но начинайте делать это немедленно — в идеале, в течение нескольких минут.
Если коллега прислал список изменений, вполне вероятно, что его работа приостановилась до получения вашего отзыва. В теории, система контроля версий позволяет создать ещё одну ветку и продолжить работу, а затем смержить изменения в ревью с новой веткой. В реальности, в мире есть примерно четыре разработчика, которые эффективно умеют такое делать. У всех остальных распутывание трёхсторонних дифов занимает столько времени, что нивелирует любой прогресс, которого можно добиться в ожидании ревью.
Если вы начинаете ревью немедленно, то создаёте благоприятный цикл. Весь документооборот становится исключительно функцией размера и сложности списка изменений автора. Это побуждает авторов присылать небольшие, точно сформулированные списки. Их легче и приятнее рассматривать, так что ваши ревью ускорятся, а благоприятный цикл продолжится.
Представьте, что ваш коллега реализовал новую функцию, которая требует изменения 1000 строчек кода. Если он знает, что вы можете сделать ревью списка изменений на 200 строчек примерно за два часа, то разобьёт эту функцию на фрагменты примерно по 200 строчек и закончит ревью всей функции за один-два дня. Но если любые код ревью занимают у вас целый день, независимо от размера, то утверждение функции займёт неделю. Ваш коллега не хочет ждать целую неделю, так что он скорее направит вам более крупные списки, по 500-600 строк каждый. Их труднее рассматрвиать, да и отзывы станут беднее, потому что тяжелее сохранить контекст для изменения на 600 строк, чем на 200.
Абсолютным максимумом продолжительности одного раунда ревью должен быть один рабочий день. Если вы заняты проблемой с ещё большим приоритетом и не укладываетесь в один рабочий день, сообщите коллеге об этом и дайте ему возможность выбрать для ревью кого-то другого. Если вам приходится отказываться от ревью чаще, чем раз в месяц, то вероятно, что команде нужно снизить темп гонки, чтобы сохранить вменяемые практики разработки.
Начните с высокого уровня, и спускайтесь ниже
Чем больше заметок вы пишете в каждом конкретном раунде, тем больше риск, что автор будет чувствовать себя подавленным. Точный лимит зависит от разработчика, но опасная зона обычно начинается в районе 20-50 заметок на один раунд ревью.
Если не хотите утопить автора в море замечаний, ограничьте себя только высокоуровневыми правками в первом раунде. Сконцентрируйтесь на проблемах вроде перепроектирования интерфейса класса или разбиения сложных функций. Подождите исправления этих проблем, прежде чем переходить к низкоуровневым вопросам, таким как именование переменных или понятность комментариев в коде.
Ваши низкоуровневые заметки могут стать неактуальными, когда автор сделает высокоуровневую правку. Перенеся их на более поздний раунд, вы оградите себя от нетривиальной работы по тщательному подбору слов для комментариев на эти спорные темы, а автору не придётся обрабатывать необязательные замечания. Такая техника также сегментирует уровни абстракции, на которых вы концентрируетесь в процессе ревью, помогая вам и автору ясно и систематически пройтись по списку изменений.
Щедро используйте примеры кода
В идеальном мире автор кода будет благодарен за любое ревью. Для него это возможность учиться и защита от ошибок. В реальности много внешних факторов, из-за которых авторы могут негативно воспринять ревью и возмущаться, что вы делаете замечания к их коду. Может, они спешат из-за дедлайна, так что любая заминка, кроме мгновенного одобрения, кажется препятствием. Может, вы не так долго работаете вместе и они не верят, что ваше замечание сделано из благих намерений.
Хороший способ улучшить восприятие код ревью автором — найти возможность подарить ему подарок. А какие подарки любят получать все разработчики? Конечно, примеры кода.
Если вы облегчите работу автора тем, что сами напишете некоторые изменения, которые предлагаете сделать, то продемонстрируете, что вы щедро делитесь своим временем как рецензент.
Предположим, ваш автор не очень хорошо знаком с функцией списковых включений в Python. Он присылает на рецензию код с такими строчками:
Ответ в стиле «Можно это упростить с помощью списковых включений?» вызовет раздражение, потому что ему нужно потратить 20 минут, изучая функцию, которую никогда раньше не использовал.
Автор будет гораздо счастливее получить такую заметку:
Предлагаю рассмотреть списковое включение такого рода:
Эта техника не ограничивается однострочными изменениями. Я часто создаю собственную ветку кода для демонстрации автору большой концептуальной идеи, такой как разбиение крупной функции или добавление юнит-теста для покрытия дополнительной граничной ситуации.
Применяйте эту технику только для ясных, бесспорных улучшений. В вышеприведённом примере спискового включения немногие разработчики будут спорить с сокращением количества строк кода на 83%. И наоборот, если вы напишете пространный пример для демонстрации изменения, которое «лучше» на ваш личный вкус (например, изменения стиля), то такой пример кода создаст впечатление, что вы настырно проталкиваете свои предпочтения, а не проявляете щедрость.
Ограничтесь двумя-тремя примерами кода на каждый раунд. Если начнёте писать код для всего авторского списка изменений, то это создаёт впечатление, как будто вы не считаете самого автора способным написать этот код.
Никогда не говорите «ты»
Это звучит немного странно, но послушайте: никогда не обращайтесь лично к автору в процессе код ревью.
Ваши решения должны быть основаны на идеях по улучшению кода, а не на том, кто придумал эти идеи. Ваш коллега потратил немало сил на свой список изменений и код и, наверное, гордится проделанной работой. Естественная реакция на критику его работы — стать в оборонительную позу.
Подбирайте слова для отзыва таким образом, чтобы минимизировать риск возникновения оборонительной позы. Чётко обозначьте, что вы критикуете код, а не самого разработчика. Когда автор видит в комментарии личное обращение к нему, это отвлекает его внимание от кода и переводит внимание на личность. Так повышается риск, что он воспримет критику лично.
Например, вот безобидный комментарий:
Ты допустил ошибку в слове «благополучно».
Автор может интерпретировать такое замечание двумя соврешенно разными способами:
Вторая заметка — это просто исправление, а не оценка автора.
К счастью, можно легко переписать свои комментарии, избегая слова «ты».
Вариант 1: Замените «ты» на «мы»
«Мы» усиливает коллективную ответственность всей команды за код. Автор может перейти в другую компанию, как и вы, но в том или ином виде останется команда, которая отвечает за этот код. Может показаться глупым говорить «мы», когда речь идёт об изменении, которое автор явно должен сделать единолично, но лучше выглядеть глупым, чем обвинителем.
Вариант 2: Удалите из предложения субъект
Другой вариант избежать личного обращения — использовать сокращение, которое исключает из предложения субъект:
Того же эффекта можно достичь с помощью пассивного залога. Я обычно в технических текстах избегаю пассивного залога как чумы, но это может быть полезный вариант обхода проблемы с личным обращением:
Ещё один вариант — перефразировать предложение в виде вопроса, который начинается со слов «Что насчёт. » или «Как насчёт. »:
Оформляйте отзывы как запросы, а не команды
Код ревью требует большего такта и осторожности, чем обычное общение, потому что здесь выше риск, что обсуждение скатится в личный спор. Казалось бы, рецензенты должны проявлять бóльшую вежливость и учтивость в код ревью по сравнению с личным общением. Но я обнаружил страннейшим образом прямо противоположную ситуацию. Многие люди никогда не скажут коллеге, «Дай мне этот степлер, а потом принеси газировки». Но я видел множество случаев, когда рецензенты оформляют отзывы в таком командном стиле, вроде «Перенеси этот класс в отдельный файл».
Но и не будьте раздражающе вежливым в своих комментариях. Оформляйте их как запросы или предложения, а не как команды.
Сравните одно и то же замечание, оформленное двумя разными способами:
Замечание, оформленное как команда | Замечание, оформленное как запрос |
---|---|
Перенеси класс Foo в отдельный файл. | Мы можем перенести класс Foo в отдельный файл? |
Людям нравится контролировать свою работу. Такой запрос даёт автору чувство самостоятельности.
Такие запросы также повышают шансы, что автор ответит вежливо. Может быть, у него есть свои причины сделать такой выбор. Если вы оформляете замечание в виде команды, то любое несогласие автора принимает форму неповиновения. Если оформить замечание как запрос или вопрос, то автор может просто ответить.
Сравните, насколько воинственной кажется беседа в зависимости от оформления первоначального замечания:
Видите, насколько более цивилизованным стало общение, когда вы сконструировали воображаемый диалог в доказательство своего тезиса оформили замечание в виде запроса, а не команды?
Обосновывайте принципами, а не мнениями
Когда составляете замечание для автора, то объясните и предлагаемое изменение, и его причину. Вместо фразы «Нам следует разделить этот класс на два» лучше сказать «Сейчас этот класс отвечает одновременно и за скачивание файла, и за его парсинг. Следует разделить его на класс для скачивания и на класс для парсинга согласно принципу единой ответственности».
Если обосновывать замечания принципами, то дискуссия принимает конструктивную форму. Когда вы цитируете конкретную причину, вроде «Нужно сделать эту функцию приватной, чтобы минимизировать открытый интерфейс класса», то автор не может просто ответить «Нет, я предпочитаю сделать по-своему». А если он так ответит, то это будет выглядеть глупо, потому что вы показали, как изменение позволяет достичь цели, а он просто заявил о своём предпочтении какому-то способу.
Разработка программного обеспечения — это одновременно искусство и наука. Не всегда можно точно сформулировать, что именно неправильно в данном фрагменте кода с точки зрения устоявшихся принципов. Иногда код просто некрасивый или излишне усложнённый, и сложно точно сформулировать, почему. В таких случаях объясните как можете, но сохраняйте объективность. Если сказать «Мне кажется, это сложно понять», то это хотя бы объективное утверждение, в сравнении с фразой «Это запутанный код», что является оценочным суждением, с которым не каждый может согласиться.
Предоставляйте подтверждающие доказательства в форме ссылок, где это возможно. Соответствующая секция руководства по стилям вашей команды — наилучшая ссылка, какую вы можете предоставить. Вы можете также сослаться на документацию для языка или библиотеки. Ответы StackOverflow с высокими рейтингами тоже подходят, но чем дальше вы отходите от авторитетной документации, тем более зыбкими становятся доказательства.
Часть 2: скоро
Через пару недель я опубликую вторую часть статьи. Будьте на связи, там рассмотрим дополнительные советы, в том числе:
Код ревью, как внедрить и не испытывать боль
Если вы работаете в продуктовой компании, то жизненный цикл почти каждого продукта будет соответствовать принципу Парето:
20% времени мы пишем новый код.
80% времени поддерживаем старый. Поддержка в себя включает фиксы багов, обновление кодовой базы (переезд на новые библиотеки например).
Во время поддержки мы хотим чтобы все разработчики как можно быстрее вникали в то, что написано. Для этого есть много способов, все они прекрасны и хорошо работают вместе.
Способы иметь хорошо поддерживаемый код:
Все эти способы нужно уметь готовить. Есть тесты, которые только мешают, бесполезная документация и бессмысленный код ревью.
Что же такое код ревью?
Что нам даёт код ревью:
Проверку кода по многим критериям.
Автор не всегда видит неочевидные места в его коде (по разным причинам).
В результате написания и переписывания может быть потеряна композиция.
Автор, находясь в контексте задачи может недостаточно оставить комментариев.
Эти и многие другие проблемы может решить код ревью.
Шаринг знаний о проекте.Не только автор знает, что там пишется в другом проекте или части проекта, а все.
Шаринг знаний о технологиях.Вы можете заметить, что кто-то напилил своих костылей, а похожая проблема в проекте уже решалась. В таком случае ревьюер может подсказать что уже использовалось и избежать ненужного кода
Сотрудники быстрее онбордятся. Новые сотрудники, проводя код ревью, быстрее узнают и понимают традиции команды, а проходя его, имеют возможность исправить ошибки, узнать больше о продукте и меньше испытывать стресс.
Способов поддерживать код «качественным» есть много, но все их нужно правильно готовить. Нужно уметь писать тесты, нужно уметь писать документацию и нужно правильно организовать код ревью.
Советы по организации процесса
Условные обозначения:
— Как делать не нужно
+ Как делать нужно
- Ревьюить только джунов и новых коллег
+ Проводить ревью для всех и всеми
Если ревью проходят не все, то в вашей команде это будет восприниматься как источник недоверия и неравенства. Такую атмосферу следует избегать.
- Обсуждать на код ревью стиль
+ Настроить у себя на проекте инструменты, которые автоматически будут исправлять стиль перед коммитом
В JS это eslint и prettier. Не тратьте время своё и коллег на споры о вкусах. Договоритесь заранее и пропишите правила. В случае разногласий голосуйте.
Не обсуждайте стиль на ревью
- Ревьюер запускает руками билд или проверяет код на баги.
+ Автор сам несет ответственность за поставленный код.
Автор тщательно проверил свой код на работоспособность и залил в удаленный git репозиторий
На сервере CI/CD проверяет тесты, сборку, работоспособность в целом.
Проверка со стороны ревьюера.
- Ревьюер не читает код
+ Обращать внимание на композицию, магические переменные, оптимизацию (там где это имеет смысл)
Обращайте внимание на код ревью.
- Агрессия, негатив, «токсичное» поведение в комментах
+ Старайтесь отмечать не только негативные стороны, но и хвалить за позитивные.
Агрессия, негатив и токсичное поведение в принципе стоит избегать во всех областях жизни. Грубое поведение во время код ревью может привести к:
Стрессу и страху совершить ошибку
Примеры плохих комментов:
Примеры хороших комментов:
«Давай попробуем сделать …» «Может попробуем вынести…»
То же самое касается и ответов на комменты автором. Если автор не согласен, то необходимо объяснять с помощью аргументов, а не фраз в духе «я так делать не буду». Если не получается текстом, иногда проще подойти или сделать короткий созвон, чтобы понять друг друга.
Чаще хвалите код в ревью
- Все комментарии обязательны к исправлению
+ Помечать необязательные комментарии
Пытаться объяснить алгоритмы на словах
Написать пример псевдокода
Особенно актуально, если автор не совсем понимает что от него хочет ревьюер. Не нужно полностью писать реализацию. Напишите небольшой пример псевдокода. Gitlab, github и bitbucket позволяют вставлять в комментарии разметку markdown. Ваш код будет хорошо отформатирован и более понятен, чем длинная цепочка комментов.
- Оставлять больше 50 комментариев
+ Оставлять до 50 комментариев
Если комментариев накапливается очень много, то у вас:
Или не настроены линтеры, и вы не определились со стилем, поэтому комменты о вкусовщине
Или переделать в коде нужно многое
Во втором случае оставьте 1 комментарий с рекомендациями как и что нужно переписать. В таком случае лучше закрыть мердж реквест, переписать код и открыть новый.
Если код будет переписан полностью, отследить изменение по комментариям практически невозможно и они не имеют смысла. Авторы тоже должны это понимать и следить, чтобы всем было комфортно.
- Отправлять огромные фрагменты кода на ревью
+ Дробить большие участки кода на несколько реквестов и вливать постепенно
Задачи бывают разными по объему. Может быть задача исправить 1 строчку кода, а может быть задача отрефакторить весь проект. Во втором случае не отправляйте реквест в котором исправлены 500 файлов и 4000 изменений. Никто в здравом уме не сможет это нормально проверить, и желания такое проверять вы тоже не найдете.
Сделайте ветку feature/epic-name
Изменения делайте в ветке feature/epic-name-implement
Pull реквесты делайте в ветку feature/epic-name. Порционно.
В конце реализации фичи подлейте feature/epic-name в мастер. Да, в этом случае пулл реквест тоже будет огромный, но всё уже будет заранее проверено
Другой вариант как этого избежать: feature flags. Вы вливаете изменения на прод, но не даете им пользоваться. Нормально это реализовано мало у кого. Поэтому с этим подходом нужно быть осторожнее.
Уважайте тех, кто будет проверять ваш PR
- Pull Request висит без ревью трое суток
+ Выработать расписание
Среди аргументов против код ревью вы услышите, что с ним у вас увеличивается время «доставки» фич. Чтобы этого не происходило, выработайте у ревьюеров расписание. Например, новые реквесты проверять до работы и перед уходом домой, а исправленные в перерывах в течении дня (например пока проект собирается или тесты гоняются).
В заключении
В этой статье я хотел рассказать и показать плюсы проведения код ревью. Будучи старшим разработчиком я всегда за то, чтобы мой код проходил code review. Лично для меня это отличный способ “увидеть” свой код чужими глазами. еще до того как ветка отправляется на ревью. Не говоря уже о том, что для команд это недорогой и эффективный способ иметь кодовую базу, с которой можно будет эффективно работать.
Если в вашей команде нет код ревью, то самое время его внедрить .
Ссылки и благодарности
По теме рекомендую почитать:Статья How to Do Code Reviews Like a Human
Спасибо лису и kirjs из @learnInPublic за ревью статьи про ревью.
Еще одна статья о code review
Что такое code review
Что можно инспектировать
Для ревью подходит любой код. Однако, review обязательно должно проводиться для критических мест в приложении (например: механизмы аутентификации, авторизации, передачи и обработки важной информации — обработка денежных транзакций и пр.).
Также для review подходят и юнит тесты, так как юнит тесты — это тот же самый код, который подвержен ошибкам, его нужно инспектировать также тщательно как и весь остальной код, потому что, неправильный тест может стоить очень дорого.
Как проводить review
Вообще, ревью кода должен проводиться в совокупности с другими гибкими инженерными практиками: парное программирование, TDD, CI. В этом случае достигается максимальная эффективность ревью. Если используется гибкая методология разработки, то этап code review можно внести в Definition of Done фичи.
Из чего состоит review
Также очень важно определиться, за кем будет последнее слово в принятии финального решения в случае возникновения спора. Обычно, приоритет отдается тому кто будет реализовывать код (как в scrum при проведении planning poker), либо специальному человеку, который отвечает за этот код (как в google — code owner).
Как проводить design review
Design review можно проводить за столом, в кругу коллег, у маркерной доски, в корпоративной wiki. На design review тот, кто будет писать код, расскажет о выбранной стратегии (примерный алгоритм, требуемые инструменты, библиотеки) решения поставленной задачи. Вся прелесть этого этапа заключается в том, что ошибка проектирования будет стоить 1-2 часа времени (и будет устранена сразу на review).
Как проводить code review
Можно проводить code review разными способами — дистанционно, когда каждый разработчик сидит за своим рабочим местом, и совместно — сидя перед монитором одного из коллег, либо в специально выделенным для этого месте, например meeting room. В принципе существует много способов (можно даже распечатать исходный код и вносить изменения на бумаге).
Pre-commit review
Данный вид review проводится перед внесением изменений в VCS. Этот подход позволяет содержать в репозитории только проверенный код. В microsoft используется этот подход: всем участникам review рассылаются патчи с изменениями. После того как собран и обработан фидбэк, процесс повторяется до тех пор пока все ревьюверы не согласятся с изменениями.
Post-commit review
Данный вид review проводится после внесения изменений в VCS. При этом можно коммитить как в основную ветвь, так и во временную ветку (а в основную ветку вливать уже проверенные изменения).
Тематические review
Можно также проводить тематические code review — их можно использовать как переходный этап на пути к полноценному code review. Их можно проводить для критического участка кода, либо при поиске ошибок. Самое главное — это определить цель данного review, при этом цель должна быть обозримой и четкой:
Результаты review
Сложности при проведении review (субъективное мнение)
Основная сложность, с которой мы столкнулись, когда внедряли review в первый раз: это невозможность контроля изменений по результатам review. Отчасти это связано с тем, что данная практика применялась без других практик — CI (это еще раз доказывает тот факт, что все инженерные практики должны применяться вместе).
Утилиты для review
Вообще, утилит для проведения review существует большое количество, как платных, так и бесплатных. Я не стал их приводить, чтобы не навязывать свою точку зрения, в интернете можно найти множество инструментов и подробное описание.
Ссылки
Пожелания, дополнения, критика приветствуется
Code review: вы делаете это неправильно
Сегодня очень многие в разработке используют ревью кода. Практика полезная, нужная. Даже если вы не делаете ревью, вы наверняка знаете, что это такое.
На рынке есть куча инструментов для ревью кода с готовыми сценариями использования, рекомендациями и правилами. GitHub, Phabricator, FishEye/ Crucible, GitLab, Bitbucket, Upsource — список можно долго продолжать. Мы в Badoo тоже в своё время с ними работали: в своей предыдущей статье я рассказывал нашу историю ревью кода и о том, как мы пришли к изобретению собственного «велосипеда» — решения Codeisok.
Информации предостаточно, можно нагуглить кучу статей про ревью кода с реальными примерами, практиками, подходами, рассказывающих о том, как хорошо, как плохо, как нужно делать, а как — не нужно, что стоит учитывать, а что — нет, и т. д. В общем, тема «обсосана до косточек».
Именно поэтому другую часть айсберга можно и не заметить.
Я надеюсь, что вы серьёзно отнесётесь к тем вещам, которые я опишу в этой статье, в некоторых случаях специально утрируя. Ревью, как и любой другой процесс, требует внимательного и обдуманного внедрения, тогда как подход «Сделаем как у всех, лишь бы было» может не только не сработать, но даже навредить.
После прочтения этого вступления у вас может возникнуть вопрос: а что сложного в ревью кода? Дело-то плёвое. Тем более что большинство инструментов, перечисленных выше, сразу и флоу ревью предлагают (из коробки: поставил, закоммитил/ запушил — и вперёд!).
Но практика показывает, что всё не так очевидно, как кажется на первый взгляд. В том числе из-за мнимой простоты процесса. Когда всё налажено и работает, есть риск, что менеджер успокоится и перестанет погружаться в процесс, передав его разработчикам. А они или будут следовать процессу, или займутся чем-то другим в ущерб решению проблем, которые процесс ревью призван решать. Менеджер может и не догадываться, что что-то идёт не так, потому что не даёт установок или не навязывает правила. В то время как для бизнеса очень важно решать задачи как можно быстрее, не тратя время на незапланированные активности.
Когда-то давным-давно, когда я работал в другой компании, мне в память глубоко запала беседа о ревью кода с одним из ведущих разработчиков. Это был p1lot. Возможно, кто-то из читателей знаком с ним (p1lot, привет :)). Сейчас он работает с нами в Badoo, и это здорово.
Так вот, PILOT тогда сказал: «Самое сложное для меня в процессе ревью — пойти на компромисс и пропустить готовую и корректно работающую задачу дальше, даже если я знаю другой способ её решения и даже если мой способ мне нравится больше». На мой взгляд, эта мысль является ключевой. И основной посыл всей моей статьи так или иначе вертится вокруг этого постулата.
Зачем нужен процесс ревью кода?
У вас в компании есть ревью? Правильно ли вы его делаете? У меня есть тест, который, возможно, заставит вас усомниться в этом.
Нужно ответить всего на три вопроса:
Если вы не знаете, сколько времени потребуется на ревью, и не минимизируете его постоянно, то как вы осуществляете планирование? Возможна ли ситуация, когда исполнитель, который делал ревью четыре часа, не пил чай половину этого времени? Можно ли быть уверенным в том, что от задачи к задаче время на чаепитие не увеличивается? А, может, ревьюер вообще не смотрит код, а просто нажимает «Code is OK»?
И, даже если ревью делается добросовестно, как добиться того, чтобы с ростом кодовой базы и компании время на реализацию задач не увеличивалось? Ведь руководство, как правило, ожидает, что процессы будут ускоряться, а не замедляться.
Если же ответ на третий вопрос сразу не приходит в голову, то есть смысл задаться ещё одним. Знаете ли вы, зачем вам на самом деле нужен этот процесс? Потому что «так принято у всех больших ребят»? Может быть, он вам и не нужен вовсе?
Если вы спросите своих лидов и разработчиков о том, что представляет из себя «правильное» ревью кода, вы очень удивитесь тому, насколько разные вещи услышите. Ответы могут варьироваться в зависимости от личного опыта, убеждений, собственной уверенности в правильности или неправильности вещей и даже от используемого технологического стека и языка разработки.
Помните про готовые инструменты для ревью кода, предлагающие свои подходы и флоу? Мне, например, интересно, как бы ответили на вопрос разработчики этих инструментов. Будут ли коррелировать их ответы о «правильности» ревью с ответами ваших сотрудников? Не уверен. Иногда я грустно наблюдаю, как кто-то внедряет у себя инструменты ревью, ни на минуту не сомневаясь, что они необходимы. То ли люди делают это «для галочки», то ли чтобы отчитаться, что «ревью кода внедрили, всё хорошо». И в итоге забывают про это.
Не хочу быть Кэпом, но тем не менее. Общаясь с сотрудниками, обращайте внимание на ответы вроде «Потому что так заведено» или «Это же best practice, все так делают». Вы и сами отлично знаете, что не нужно бездумно внедрять какой-то процесс, не понимая, зачем он нужен. Любой процесс должен быть оправдан и внедрён (если необходимо) с поправкой на нужды бизнеса и проекта, а также на проблемы, которые действительно существуют и которые действительно хочется решить. Карго-культу в компании с хорошей инженерной культурой не место.
Соответственно, менеджеру важно донести до людей то «правильное» ревью кода, которое необходимо именно в его проекте. А перед этим, разумеется, стоит критерии «правильности» сформулировать для себя, выбрать подходящие инструменты и установить уместные правила. А там уже и до контроля недалеко.
Плохое ревью кода
Так какой он, тот самый правильный процесс? Давайте разбираться. Ниже будет много моих личных рассуждений, и тем, кому не терпится узнать, к чему я всё это пишу, предлагаю сразу перейти к части «Хорошее ревью кода».
Тем же, кто остался, говорю спасибо и тем не менее предлагаю определить правильность ревью самостоятельно, исходя из нужд вашего проекта, тщательно всё обдумав. А я лишь попробую вам в этом помочь.
Чтобы выделить для себя важные и необходимые вещи в любом процессе, бывает полезно для начала отсечь очевидно ненужные, которые вредят инженерной культуре. Так и поступим.
Обмен знаниями
На вопрос «Зачем нужен процесс ревью кода?» я очень много раз слышал ответ «Для обмена знаниями». Действительно, полезная и нужная вещь. И многие согласны с тем, что важно иметь в команде процесс для обмена знаниями и экспертизой. Но уверены ли вы в том, что ревью кода подходит для этого?
Я не раз спрашивал людей, что они имеют в виду под «обменом знаниями». И в ответ слышал разное.
Во-первых, это демонстрация новичкам (в команде или затронутом компоненте) принятых правил и практик: вот так у нас принято делать, а так мы не делаем, потому-то и потому-то.
Во-вторых, это свежий взгляд новичка (опять же в команде либо затронутом компоненте) на то, как делаются обычные вещи. Вдруг они делаются неэффективно, и новый человек поможет это обнаружить?
В-третьих, это просто ознакомление с некоторой частью кода. Ведь если в будущем ревьюер столкнётся с необходимостью изменять эту часть кода, ему будет намного легче.
Давайте пройдёмся по всем пунктам и попробуем оценить, насколько они уместны в процессе ревью.
Code review как инструмент обучения новичков
В данном случае речь идёт в основном о таком сценарии: новичку даётся задание, он его выполняет, а потом опытный член команды (владелец компонента) указывает на ошибки, которые он допустил. Процесс важный и нужный, не спорю — новичкам нужно как-то показывать принятые в команде правила.
Но давайте начистоту: не поздновато ли? Не будет ли более правильным рассказать новичку об этих правилах до допуска его к коду? Ведь цена ошибки очень высока — редко новички сразу работают так, как вам нужно. А значит, задача будет возвращаться и возвращаться на доработку. А значит, продукт станет доступен пользователю позже, чем мог бы.
Получается, в процесс, продолжительность которого трудно оценить, мы добавляем ещё один, временные затраты на который ещё менее предсказуемы. Таким образом мы увеличиваем время, необходимое на доставку задачи пользователю, на ещё более неизвестную величину.
Разумеется, иногда обучение новичков через рабочие процессы имеет право на существование. Но порой мы не задумываемся о недостатках подходов, которые вошли в привычку. А допустить ошибку тут не просто легко, а очень легко.
Например, если в компании нет отлаженного процесса обучения и коучинга, менеджер вынужден «бросить в воду» новичка. У последнего при этом не остаётся выбора: нужно либо «плыть», либо уходить из компании. Кто-то реально учится на таких ситуациях, а кто-то не выдерживает. Думаю, многие сталкивались с подобным на протяжении своего профессионального пути. Мой же посыл в том, что драгоценнейшее время может тратиться неоптимально из-за такого явления. Равно как и процесс адаптации нового сотрудника в команде может выполняться неоптимально. Просто потому, что ни у кого руки не дошли организовать эффективный процесс внедрения новых сотрудников в команду, и менеджер подумал, что новичок всему научится на этапе ревью кода. А на самом деле это два разных процесса, очень нужных и важных.
Конечно, даже при условиях, что правила изначально объяснены новичку и что в компании существуют другие процессы обучения, процесс адаптации новичка нужно как-то контролировать. Ревью — это один из процессов, который может помочь. Но контроль и обучение — это разные вещи, не так ли? Тем более что в контроле нуждаются все члены команды, а не только новички. Ведь все мы можем делать ошибки, забывать о договорённостях и так или иначе влияем на ту часть системы, которой владеет вся команда (в данном случае на код).
Какой можно сделать вывод? Описанный выше процесс направлен на достижение иной цели: не на обучение, а на контроль. И этот самый контроль необходим не только новичкам, а команде в целом.
Всё это легко формулируется в такой тезис: «Ревью кода необходимо, чтобы контролировать соблюдение договорённостей и общих правил всеми членами команды«. А обучение новичков в этом случае будет не чем иным, как искусственной подменой цели, которая только запутает людей и направит процесс в другое русло.
Но даже с этим выводом, который, казалось бы, сформулирован правильно, можно наломать дров. Ревью кода — это этап, который наступает тогда, когда код уже написан. И может случиться так, что код, написанный без соблюдения общих правил, подгонять под общее лекало будет очень дорого. Наша задача состоит в том, чтобы сообщить исполнителю о том, что что-то пошло не так, как можно раньше. И поэтому я утверждаю, что иногда ревью кода — не самый подходящий процесс для контроля соблюдения общих правил. Во многих случаях проверки на соответствие правилам можно и нужно перенести на более ранние этапы.
Например, можно проверять форматирование кода в хуках системы контроля версий (равно как и использование задепрекейченных классов, договорённости о расположении файлов в соответствующих папках, использование внешних зависимостей и сторонних библиотек и др.). Таким образом минимизируется время, необходимое на ревью кода.
Продолжая разговор об обучении новичков в процессе ревью кода, проведём аналогию. Допустим, вы производите какой-то товар, например, торты. Допустим, вы получили заказ на торт для свадьбы VIP-персоны. Доверите ли вы «ревью» готового торта новичку? Готовы ли вы к тому, что новый кондитер возьмёт на себя ответственность за позор или успех всей пекарни на этой свадьбе? А может, вы хотите, чтобы он на этом этапе познакомился с правилами, принятыми в команде (как выпекать коржи, готовить крем и настаивать клюкву на коньяке)? Очевидно, что и процесс знакомства новичка с правилами, и контроль с его стороны — довольно странные вещи на данном этапе: торт-то уже испечён. Так почему же с фичами и кодом мы можем поступать по-другому? Или фичи, которые мы запускаем, не несут за собой позор или успех нашей команды в глазах пользователей и заказчиков?
Вы можете возразить, сказав, что задачи «для важных персон» мы готовим не каждый день и что новичку вполне можно доверить не очень срочную или не очень важную задачу. И будете правы.
Но во-первых, чему новичок научится на пустяковых задачах, в которых мало изменений кода (и эти изменения не в критичных местах и для бизнеса наверняка не столь важны, раз задача несрочная)? Как он может научиться печь торты, если всё время взбивает сливки? Переход от простого к сложному, конечно, нужен. Но делать это необходимо, тщательно контролируя процесс. Новичков нужно учить, а не бросать учиться самостоятельно.
А во-вторых, компании даже такой, казалось бы, полезный и правильный подход может навредить. Понять это очень просто, используя метод доведения до абсурда, чтобы сделать вывод максимально простым и понятным.
Представим, что мы открыто заявляем, что задачи, которые нам даёт продакт-менеджер, нужны для обучения новичков. Почему? А так же, как с ревью кода: мы ведь поручаем некоторые простые и несрочные задачи новичкам, чтобы они научились работать так, как у нас принято.
К чему это может привести в итоге? Ретивый исполнитель, который добросовестно делает своё дело и считает, что всё, что он делает, должно быть сделано максимально хорошо, примется за процесс обучения. Он может поставить задачу сделать несколько реализаций вместо одной, чтобы потом показать обучаемому недостатки и преимущества разных подходов. Он может читать лекции, сравнивая подходы и best practices, применяемые в компании и за ее пределами. Он может много чего ещё предпринять, чтобы обучить новичка. В результате время, требуемое на реализацию задачи, будет всё увеличиваться, ведь вместо разработки мы фокусируемся на обучении.
В случае ревью кода такой ретивый сотрудник инициирует длинную дискуссию в комментариях к ревью о пользе и вреде тех или иных реализаций, постит куски кода со Stack Overflow, показывая, что в других головах бывают другие идеи, даёт ссылки на статьи и книги, которые обучаемый должен прочитать, чтобы понять, что его реализация неидеальна.
Это нормальный процесс обучения, который может существовать, но который должен делаться правильно. И далеко не всегда стоит интегрировать его в процесс решения задачи бизнеса. Потому что это два разных процесса. Пусть новичок делает разные составляющие торта, пусть сделает несколько вариантов, пусть что-то испортит и выбросит. Пусть кто-то более опытный показывает ему, как нужно действовать и пересказывает старинные рецепты. Но учиться «на боевом конвейере», который производит ваш товар для клиентов — не лучшая идея. И если уж вы решились на такое, то «водите за ручку» новичка, не позволяя ему создавать помехи производственному процессу в ходе своего обучения.
Если ваша компания — это не институт, не школа, не техникум и не какое-либо другое образовательное учреждение, то обучение — не ваша прямая обязанность. Бизнес нанял вас для решения других задач и достижениях других результатов.
К слову, именно поэтому мы не добавляем в Codeisok функционал для загрузки картинок, оформления текста, подсветки кода в комментариях, хотя запросов на подобные фичи поступало и продолжает поступать немало. Мы пропагандируем идею того, что ревью кода — это не личный блог, не мессенджер и не сервис для обучения. Инструмент нужен для другого. Ведь для указания на недостатки кода обычного текстового комментария более чем достаточно.
Code review как свежий взгляд на код
Идём дальше. Следующий сценарий «обмена опытом» такой: мы в команде что-то делаем, соблюдая внутренние договорённости, и у нас замылился глаз, а тут пришёл новый человек (из другой компании или из другого компонента) — и, возможно, он увидит недостатки в нашей организации работы.
Идея хорошая, звучит разумно. Действительно, а вдруг мы делаем что-то не так?
Но опять вернёмся к сроку жизни задачи и наступлению этапа ревью кода. Не поздновато ли? Торт уже испечён, коржи смазаны кремом, розочки приклеены. Цена ошибки очень высока. И тут мы узнаём, что в другой пекарне в коржи добавляют соду, гашенную лимонным соком, для пышности. И что? Что делать? Переделывать?
Очевидно нет, так как: 1) мы всегда делали без соды, и результат был неплохим; 2) возможно, покупатели берут торты именно у нас, а не в другой пекарне, потому что им не нравится привкус соды; 3) торт уже готов, свадьба сегодня вечером, а новый торт испечь мы не успеем.
Тогда зачем это всё? Делиться опытом надо. Свежий взгляд нужен. Но, как мне кажется, на другом этапе. Например, перед тем как печь коржи, можно уточнить у новичка: «А как вы делали это раньше? А почему этот способ лучше?» И, наверное, стоит испечь пару тортов по-старому и по-новому, чтобы дать нашим постоянным клиентам попробовать и узнать их мнение.
Бездумное внедрение best practices, увиденных в статьях или докладах, может нанести вред компании и продукту. «Соду добавляют в коржи все крупные игроки: „Бугл“, „Трейсбук“, „Райл.ру“, „Юмдекс“. Все так делают». И что? Из-за этого Петя должен немедленно взяться за переделку задачи, которая уже готова?
Я уверен, что нет. Если изначально, перед решением задачи, вы не договаривались, что «в коржах будет сода», то очень недальновидно требовать её добавления на этапе ревью кода. У вашего бизнеса нет цели «иметь соду в коржах». Если ваши торты и так неплохо продаются, то совершенно неважно, есть в них сода или нет.
Нужно понимать, что обмен опытом — это другой процесс, никак не завязанный на ревью кода. Он может происходить раньше, позже или параллельно с ним, но он — другой. Можно устраивать взаимные мастер-классы с конкурентами. Некоторые пытаются выкрасть суперсекретный рецепт и суперсовременную методику взбивания крема перфоратором. Кто-то старается подсмотреть чужие идеи и усовершенствовать с их помощью свои процессы. Но странно делать это на работающем конвейере, на том этапе, когда ваш товар уже практически готов, а цена переделки очень высока.
Ведь мы говорим об этапе ревью. Ревью кода, который уже написан и готов к переходу на следующий этап. Этот код ждёт нажатия ревьюером кнопки «Code is OK». И ваши заказчики совсем не готовы к тому, что вы будете препарировать испечённый торт с новым поваром, чтобы показать ему, как вы печёте торты и выслушать его критические замечания.
Code review как знакомство с частью кода
Пожалуй, это выглядит как самая разумная и понятная часть, с которой многие согласны. Сценарий тут подразумевается такой: один программист написал код задачи, остальные на него посмотрели, изучили — и теперь знают, как с ним работать. Таким образом мы уменьшаем риск возникновения bus factor: если разработчик уйдёт, другие смогут успешно работать с его кодом. А также мы готовы к тому, что в следующий раз этот код может «потрогать» кто-то другой, и в этом случае он уже будет знать, как с ним работать.
Давайте подумаем, действительно ли это работает так, как задумано, и действительно ли помогает людям обмениваться компетенциями и знаниями о конкретных участках кода.
Посмотрим на ситуацию глазами того, кто написал код. В каждой ли задаче он применяет подходы и практики, которые обязательно должны усвоить все участники команды?
Допустим, он изобрёл какой-то крутой алгоритм сортировки (может, давно уже изобрёл и постоянно использует, а может быть, только что). Нужно ли, чтобы об этом знали все члены команды? Конечно, да. Нужно, чтобы люди узнали, что крутой программист Вася создал нереально быстрый и эффективный алгоритм сортировки, который будет полезен всем.
Однако, на мой взгляд, таких крутых вещей, которые непременно должны интегрироваться в общие правила и стандарты, не так уж много. И используются они далеко не в каждой задаче. Так стоит ли каждый раз останавливать весь конвейер, отрывать всех от работы и показывать им код всех задач Васи?
Очевидно, что во многих компаниях вся команда узнать о ноу-хау Васи на этапе ревью не может. Ведь ревью делает не вся команда, а какое-то количество ревьюеров. Если у вас это делают все, рано радоваться: с ростом команды это будет всё более и более неудобным, а в конечном итоге и невозможным. Этот процесс не масштабируется, и важно помнить, что временные ресурсы, которые можно тратить на ревью кода, ограниченны.
Теперь посмотрим на ситуацию со стороны других разработчиков, которые могли бы сделать ревью кода Васи и оценить его труд. Сидят ли они на своих местах без дела и ждут ли, пока Вася им покажет что-нибудь крутое в коде? Наверняка нет — они работают над своими задачами. У всех есть свои дела, и разработчик не всегда готов всё бросить и моментально переключиться на обсуждение достижений коллег, какими бы ошеломляющими они ни были.
И, наверное, разумно было бы, если бы Вася сообщил о своём изобретении всем как-то иначе. Сделал презентацию, например. Или просто написал письмо с куском кода и объяснениями, почему это круто, почему его алгоритм реально классный и почему он теперь должен стать стандартом для всех. Остальные его прочитают, каждый в своё время, зададут вопросы, согласятся или не согласятся с решением. И только после этого можно принимать или не принимать новый стандарт. Возможно, у кого-то окажется предложение лучше: более оптимальное, дешёвое, быстрее реализуемое.
Чувствуете? То, что я описываю, это не процесс ревью кода — это процесс создания и одобрения общих практик и подходов. Совершенно иной процесс.
И на этом этапе снова могут всплыть best practices от коллег и «В моей прошлой компании это делали по-другому» или «Я читал, что это делают иначе». И что? Best practices в общем случае, для сферической компании в вакууме, могут быть подходящими. Означает ли это, что все они будут полезны в вашей компании? Как минимум не всегда. И уж точно это не значит, что кто-то должен немедленно переписать свой код, на написание которого уже потрачено время.
Соответствие любым абстрактным или внешним best practices, гайдлайнам, модным тенденциям и т. д. — каким-то правилам, которые оторваны от компании/ контекста — ради самого соответствия никому не нужно. «Статические методы считаются плохим тоном», «Такое теперь надо выносить в трейты», «Синглтон — плохая практика, лучше использовать фабрику и DI». Кому надо? Кем считается?
Порой можно услышать здравые аргументы, но справедливые для абстрактных проектов и задач, а не для реальных. Даже полезные и нужные вещи обсуждать и рекомендовать на этапе, когда код уже написан, — поздно, потому что внедрение обойдётся неоправданно дорого.
Кроме того, в коде Васи может быть не новый крутой алгоритм или ноу-хау, а «костыль», грязный хак, который ему пришлось использовать по каким-то причинам. Действительно ли об этом должны знать все в команде? Если грязный хак оправдан и по-другому сделать было нельзя, то, наверное, да. Но даже в этом случае не стоит отвлекать всю команду, останавливая конвейер. Сомневаюсь, что и самому Васе эта идея понравилась бы.
Во-первых, мало кто готов «прямо сейчас» отвлечься и оценить «грязность» хака, ведь все заняты своими задачами. А во-вторых, никто ничего не запомнит. Да и люди в команде меняются, и, если кто-то в будущем наткнётся на этот хак, у человека так или иначе возникнет вопрос: почему сделано именно так? И оптимальным решением будет добавление комментария прямо в код. Комментария, объясняющего причину, которая вынудила прибегнуть к такому хаку (вспоминаем правила хорошего тона при комментировании кода: мы описываем в комментариях не что мы делаем в коде, а почему мы делаем именно так).
А если можно избежать использования этого грязного хака, то очевидно, что задачу стоит переоткрыть и заставить Васю переделать. Это прямое назначение ревью кода, и понятно, что к ознакомлению всей команды с кодом оно отношения не имеет (зачем всем знать о коде, который всё равно будет переписан?).
Третий случай — это когда в задаче ничего супернового и грязных хаков нет. Она базируется на всех принципах и правилах, принятых в команде, оформлена в соответствии с общими стандартами и т. д. Так зачем же тогда на неё отвлекаться всей команде? Ведь никакого bus factor нет, и, если кому-то другому придётся работать с этим участком кода в будущем, он без труда в нём разберётся.
Ну и, наконец, если мне всё ещё не удалось вас убедить, то у меня вопрос: как понять, что обмен знаниями в процессе ревью произошёл?
— Петя, ты прочёл код, который написал Вася?
— Прочёл.
— Всё понял?
— Всё понял.
Вот и весь процесс верификации. А где уверенность в том, что Петя всё понял правильно? Где уверенность в том, что, когда Петя в будущем будет изменять этот участок кода, он не наломает дров? Как измерить время, необходимое для погружения в задачу для разработчиков, один из которых код ранее не видел, а другой — пару раз делал ревью этого кода?
Более того, где уверенность в том, что в следующий раз работать с этим участком кода будет именно Петя, который делал ревью, а не Эдуард, который сейчас делает ревью задачи Арсения с совсем другим компонентом?
Таким образом, я утверждаю, что ознакомление с кодом чужих компонентов в процессе ревью кода работает далеко не так, как ожидается. Это только замедляет этап ревью кода и, как следствие, тормозит доставку фичи пользователю.
Если необходимо делиться знаниями между разными компонентами, то лучше подойти к этой задаче как к отдельной и самостоятельной. Следовать общим правилам для всей компании, использовать одинаковый технологический стек, делиться знаниями на внутренних митапах и семинарах, писать общую документацию (опять же по принятым стандартам и правилам), передавать задачи по соответствующим компонентам на реализацию коллегам из других компонентов, соблюдая все правила по адаптации новичков (рассказать о том, как принято в компании, что вы делаете, а что — нет, помочь составить хороший технический план и т. д.). Такое погружение в компонент гораздо полезнее для всех участников, нежели просто просмотр кода.
Ревью кода — это процесс верификации, контроля, процесс, который должен проходить максимально быстро. И примешивать к нему другие процессы (обучение новичков, обмен знаниями, демонстрация крутизны экспертизы и т. д.) просто неуместно. Это как минимум неоправданно дорого и — главное — не работает на данном этапе.
Code review как документация
Ещё один вариант ответа на вопрос «Зачем нужно ревью кода?» был про документирование. Тут может быть не совсем очевидно, что имеется в виду, поэтому поясню.
Сценарий подразумевается такой: разработчик и ревьюер кода о чём-то договорились в комментариях к ревью, и в будущем, если у кого-то возникнет вопрос о том, почему в данном месте сделано именно так, информацию об этом можно будет найти в ревью. То есть по git blame найти коммит, по коммиту найти тикет, по тикету найти ревью, в ревью найти обсуждение, изучить его и понять, почему сделано именно так.
Получается, что разработчик, у которого и так много источников информации — стандарты и соглашения, техническое задание, тикет в багтрекере, сам код (с комментариями или без) — должен лезть ещё в один. И читать весь тред, пытаясь уловить нить беседы и понять причины, побудившие сделать именно так. Причём мнения сторон в этом треде могут измениться несколько раз. Звучит странно, не правда ли?
Но вопросы действительно могут возникнуть. Что делать в этом случае?
Очевидно, что, если вопрос возник на этапе ревью, на него следует ответить. Но также нужно понимать, что если у ревьюера возник вопрос, то такой же вопрос может возникнуть и у людей, которые будут работать с кодом позже. Это может быть знаком того, что код написан в обход каких-то соглашений (и требуется пояснение, почему). В этом случае стоит оставить пояснение прямо в коде, например, в виде комментария со ссылкой на тикет — тогда любой человек, глядя на код, будет понимать, почему сделано именно так.
Если причин для игнорирования соглашений нет, то очевидно, что задачу необходимо вернуть на доработку и привести к требуемому виду. Стоит ли хранить для этого историю переписки и заставлять разработчиков в будущем всю эту историю перечитывать? Думаю, это лишнее.
Кроме того, может быть так, что код написан недостаточно понятно для ревьюера, хоть и соответствует правилам. В этом случае его следует переписать, чтобы для ревьера и будущих разработчиков он стал понятен. Опять же, нужно ли это хранить в истории для будущих поколений? Я в этом не уверен.
Даже если возникла ситуация, когда комментарии нужны и будущим поколениям стоит о них знать, в этом случае лучше оставлять их в коде или тикете — в тех источниках информации, которые у разработчика уже есть. Зачем вводить новый (тем более со всей историей переписки)?
В тикете можно написать «Решили сделать так-то по таким-то причинам» как итог всех обсуждений. Так и читать понадобится меньше, и много времени у интересующегося разработчика не отнимет. Ну и информацию при необходимости разработчик сможет получить.
Вывод напрашивается сам собой: использовать ревью кода как источник документации не стоит. Этот процесс нужен для другого.
Хорошее ревью кода
Список ошибочных вещей и процессов, которые люди имеют склонность добавлять в ревью кода, можно долго продолжать. И даже если бы я поставил перед собой цель собрать полный список, я бы не смог сделать его исчерпывающим. Ведь сколько людей, столько и мнений.
Я думаю, что в данном случае нужно иметь в виду тот факт, что смешивать несколько процессов в один — не всегда правильно. Особенно если речь идёт о процессах, продолжительность которых сложно планировать и контролировать.
Правильный процесс ревью кода — это процесс контроля. Контроля того, что задача выполнена в полном объёме. Контроля того, что соблюдены общие правила и договорённости. Контроля того, что решение не избыточно и что его легко поддерживать и развивать в будущем.
Следовательно, я бы выделил следующие моменты, которые обязательно должны быть проконтролированы на этапе ревью:
Архитектура и дизайн решения
Насколько предлагаемое решение соответствует задаче? Насколько оно сложно для понимания, поддержки, расширения? Насколько получившаяся архитектура требовательна к ресурсам и какова сложность реализованных алгоритмов? Можно ли было сделать проще, понятнее, дешевле? Сделано ли что-то лишнее, что не относится к задаче? Используем ли мы внешние зависимости и библиотеки? Насколько это оправданно? И так далее.
Не забываем, что это этап контроля. Соответственно, архитектура решения должна быть оговорена заранее. На данном этапе мы должны удостовериться, что договорённости соблюдены.
Также стоит иметь в виду, что это один из самых сложных, ответственных и трудоёмких моментов в ревью кода. Человек, выступающий в роли ревьюера, должен быть готов идти на компромиссы и помнить о том, что идеальных решений не бывает. И если он видит в задаче решение, которое по тем или иным причинам считает неидеальным, то он должен в первую очередь примерить его на нужды бизнеса, а не на свой перфекционизм.
Ревьюер может предложить свою реализацию, которую он считает более правильной с точки зрения best practices, паттернов и других вещей. Но тут очень важно понимать, что решение, которое он видит в коде, лучше его собственного уже одним лишь тем, что оно готово. Оно уже написано. Разработчик потратил своё время на реализацию, интеграцию этого куска кода с другими элементами, тестирование и написание автотестов и так далее. И исправлять код следует только в том случае, если решение заведомо неправильное.
Соблюдение правил и соглашений, принятых в команде
Все мы разные. У каждого свой взгляд на вещи, и зачастую можно обнаружить, что даже в слаженном и давно сработавшемся коллективе взгляды могут быть диаметрально противоположные. Кто-то считает, что табуляции лучше пробелов, а кто-то уверен в обратном.
Чтобы это не сказывалось на ежедневных задачах, чтобы любой участник команды мог как можно быстрее вникнуть в код и задачу другого, устанавливают командные соглашения. Начиная со стандартов оформления кода, структуры папок и файлов проекта и заканчивая формализацией API-методов и запретами на использование тех или иных конструкций языка. Таким образом обеспечивается и поддерживаемость кода, и уменьшение риска возникновения bus factor, и много что ещё.
Тут опять же важно помнить о том, что на этапе ревью мы контролируем соблюдение общих договорённостей. Учить на этом этапе стандартам кодирования уже поздно и может быть неоправданно дорого.
Корректность решения
Имеет ли решение какие-то недостатки? Опечатки в коде, магические числа и другие неопределённые константы. Магические строки и другие неожиданные данные, которые программист не учёл при решении задачи. Проверки на null и другие места, где потенциально можно «выстрелить себе в ногу». Проверка решения с точки зрения информационной безопасности и так далее.
Это очень важная часть ревью кода, и выполняться она должна очень тщательно и аккуратно. Сколько программистов у вас в команде знает, что это необходимо делать на этапе ревью? Сколько программистов это реально делает?
Тестируемость кода и наличие юнит-тестов
Данный этап проверок также очень важен, ведь он направлен на улучшение той самой пресловутой поддерживаемости кода.
Многие понимают, что означает термин code maintainability, но далеко не все могут объяснить, что это такое. А раз так, то как тогда ставить задачу команде на поддержание этой самой maintainability? Поэтому я считаю, что разработчик, подумавший о том, как его код будет тестироваться, а уж тем более тестирующий собственный код и написавший автотест для автоматизации тестирования, естественным образом будет стремиться к тому, чтобы его код удовлетворял большинству критериев code maintainability. Ведь без этого написать автотест практически невозможно.
Общей рекомендацией к любому изменению кода также может быть правильная декомпозиция задач. Чем меньше объём кода, проходящего через конвейер всех процессов от идеи до продакшена, тем легче этот код проверять на соответствия, тестировать, объединять с кодом других задач и выкладывать. Ревью кода — не исключение. Задачу с большим количеством изменённых строк во многих файлах очень тяжело читать и понимать (и удерживать в голове зависимости). Риск и цена исправления недочётов могут быть очень высокими.
Заключение
Вот, собственно, и всё. Эти четыре момента — это именно те самые вещи, которые необходимо проверять на этапе ревью. Их легко формализовать, легко донести до исполнителей, а значит, несложно и сформулировать критерии верификации и прогнозировать продолжительность. Автоматизация и другие способы минимизировать время ревью становятся более чем возможными.
И напоследок дам ещё несколько советов.
Важно помнить, что любой возврат кода на доработку не лучшим образом сказывается на качестве реализации. Даже если все действуют из лучших побуждений. В случае ревью кода это работает так: разработчик правит код, но тестирует его уже не так тщательно, как в первой итерации (зачем? Ведь он уже всё проверил, а изменил совсем чуть-чуть). Опыт показывает, что большинство таких ситуаций оборачивается багами именно в тех местах, где были правки по результатам ревью кода. Так работает человеческая психология.
В начале статьи я предлагал вам ответить на три вопроса про необходимость и правильность ревью. Теперь у вас есть алгоритм, который поможет найти ответы. Зная шаги контроля и учитывая размер задачи, вполне можно планировать время, необходимое на ревью кода, каждый раз стремясь его всё больше и больше сократить.
Внедряя любой процесс, важно помнить о целях, которые мы перед собой ставим, и о проблемах, которые хотим решить. Тогда будет гораздо проще отделять зёрна от плевел и формулировать измеряемые критерии оценки эффективности. А формулировать их нужно для себя самого и для своей команды, которая тоже остро нуждается в решении возникших проблем, но понимать их может по-своему.
Есть и ещё один важный момент: те процессы, правила и ценности, которые справедливы для одной компании, могут быть не столь полезными и работающими в другой. То, что я описал как примеры правильного ревью, работает в нашей компании. Мы пропагандируем быструю разработку, частые релизы и делаем ревью для каждой задачи. Подумайте о том, насколько это применимо к вашему проекту, и о том, что ревью — это один из тех процессов, которые пускать на самотёк нельзя.
Что такое код-ревью
Это проверка кода на ошибки, неточности и общий стиль программирования.
Послушать аудиоверсию этой статьи (6 минут):
Слушайте Что такое код-ревью на Яндекс.Музыке
Ситуация: вы разработчик. Вы отвечаете за свой фронт работы, например, за отправку данных на сервер. У команды уже есть готовый проект, вы вместе его поддерживаете и постоянно улучшаете.
Когда вы пишете новую функцию, она не попадает сразу в проект. Вместо этого ваш код отправляется на код-ревью (code review).
Что делают на код-ревью
Во время код-ревью кто-то из старших товарищей изучает ваш код на предмет:
Если проблемы есть, проверяющий отправляет код на доработку. Если всё хорошо, код переходит на следующую стадию — как правило, в тестирование.
Кто проводит
Обычно принято так: код-ревью делают программисты более старшего уровня. Джуниоров проверяют мидлы, мидлов — сеньоры, сеньоров — другие сеньоры или тимлиды. Если компания большая, то могут выделить отдельно несколько человек, которые смотрят код у всех и следят за общим стилем.
Если команда небольшая, то код-ревью делает ведущий программист — он сам следит за проектом и за качеством кода, который пишут остальные.
Как это выглядит
Зачем это нужно
Если вы пишете один или с другом, то, скорее всего, вам это не нужно. Вы и так можете обсудить код между собой и понять, как его сделать лучше.
Когда в команде программистов много, то компания сталкивается с тем, что все пишут по-разному. И даже если весь этот код работает, его потом нужно поддерживать, а ковыряться в чужом коде, если он плохо написан — это долго и дорого. Поэтому на этапе код-ревью разработчики делают так, чтобы им же позднее было проще поддерживать код и ничего не ломалось.
May the Code Review be with you
Code review может быть большой болью для команды, которая только начинает его внедрять. Вы в любом случае наступите на много граблей: будете проводить ревью дольше, чем пишете код, устраивать смертельные споры про расположение скобочек и разбираться, можно ли сливать ветку в master до аппрува команды или нет. Я собрал ряд практик, которые помогут вам сделать процесс адаптации чуть менее болезненным — по крайней мере, мне они точно помогли.
Этот материал — краткая выжимка моего опыта, накопленного за несколько лет работы в крупных командах мобильной разработки. Опыт по большей части в мобильной разработке, что оказало влияние на используемые примеры и ссылки. Для тех, кто предпочитает не читать, а смотреть, в течение пары месяцев должно появиться видео с конференции Mobius, где я рассказываю доклад на эту же тему, но с кучей подробных практических примеров.
Цели Code Review
Для начала тезисно обозначу цели, которые стоит преследовать при проведении инспекций кода. Именно этими целями и их приоритетом относительно друг друга я и руководствуюсь, советуя внедрять ту или иную практику.
Модели организации Code Review в разных командах
Команды бывают разные — где-то в одном отделе сидит тридцать человек, которые разрабатывают один проект, а где-то разработчик в одиночку поддерживает сразу десяток приложений.
Небольшая команда
Первый случай, наверное, самый часто встречающийся. Это команда из нескольких человек — скажем, до четырех, которые работают над одним проектом. Здесь уже обычно вовсю расцветает Git Flow, задачи разрабатываются независимо друг от друга в отдельных ветках, а по окончании сливаются в develop.
В таком случае лучше всего взлетает модель, при которой для слияния ветки с задачей в основную рабочую ветку требуется одобрение всех участников команды. Это помогает получить максимальные преимущества по всем перечисленным в первом разделе целям проведения ревью.
Несколько отдельных команд
Масштабируем предыдущую ситуацию и представим крупного аутсорсера, в которого входят несколько таких маленьких команд, каждая из которых работает над своим собственным проектом. В таких ситуациях иерархическая структура обычно усложняется и, помимо наличия ведущих разработчиков в каждой из команд, добавляется новая прослойка, тимлид или техлид, который отвечает сразу за несколько таких проектов.
Основной подход остается неизменным — на все новые задачи создаются ревью, в которых принимает участие вся проектная команда, но итоговую ответственность за принятые технические решения несет ведущий разработчик. Кроме того, к важным ревью нужно подключать еще и лида — таким образом он будет оставаться в курсе дел, происходящих на проекте и в нужный момент подключаться к решению возникающих проблем.
В нагрузку идет еще одна дополнительная активность — очень эффективными показывают себя периодические кросс-проектные ревью, когда разработчик знакомится с остальными проектами. Это позволяет каждому быть в общих чертах в курсе того, где какие технологии и подходы используются, знать, кто и в чем разбирается. Подходить к кросс-проектным ревью можно по-разному — вести сквозной перечень всех проектов и сортировать его по последнему ревью, или ставить каждому разработчику цель — раз в определенный промежуток времени инспектировать все остальные проекты. Такие ревью проводятся в основном в ознакомительных целях — поэтому обычно одобрения стороннего ревьюера не требуется для того, чтобы завершить задачу — человек, который смотрит код, перенимает опыт у других команд.
Большая команда
Еще одна ситуация — большая команда, работающая над одним крупным проектом. В такой ситуации хорошо оправдывает себя установление определенного порога ревьюеров, после одобрения которых можно сливать ветку. Это значение может зависеть от размера команды, но разумные рамки — 2-4 человека.
Большим вопросом остается определение конкретных ревьюеров — они могут выбираться как рандомно, кто первый откликнется, так и специально автором ревью. Можно автоматизировать этот процесс, чтобы избежать споров и быть более объективными. Как пример — определять владельца кода по данным истории системы контроля версий или системы для code review.
Разработчик-одиночка
Последний случай — самый грустный. Разработчик работает один в команде. Во-первых, сразу отпадает такая цель, как коллективное владение кодом — нет коллектива, нет и владения. Остальные цели тоже становятся довольно вырожденными — но, тем не менее, сторонний взгляд на код поможет найти часть ошибок и повысить технический уровень в целом.
В таком случае разработчику нужно обращаться за помощью в открытое сообщество. Я советую три канала.
Практики проведения Code Review
Работа по расписанию
Основная проблема заключается в таком понятии, как состояние потока, из которого вырывать разработчика — смерти подобно. Сами представьте ситуацию: вы с головой погрузились в разработку своей фичи, строите в голове дворцы абстракций, и неожиданно за плечо робко трогает коллега и просит взглянуть на его код. Ничего хорошего из этого не выйдет — разработчик и своей задачей не может больше нормально заниматься, и для ревью слишком предвзят и раздражен.
Решает ситуацию расписание. Каждый разработчик в команде определяет промежуток дня, в который он будет смотреть ревью. Расписание всей команды можно держать в одной таблице, чтобы, ориентируясь на это, можно было подбирать себе ревьюеров или рассчитывать время проверки задачи. Я, к примеру, всегда выделял час времени с утра, сразу после прихода на работу — как раз помогало войти в рабочий ритм.
Конечно, есть случаи, когда этот механизм не работает — некоторые ревью действительно нужно смотреть максимально быстро. Главное — придерживаться такого режима, чтобы возникновение подобных ситуаций не становилось привычкой.
Архитектурные review
Я уверен, что вы сталкивались с ситуацией, когда после реализации задачи выясняется, что нужно было ее сделать абсолютно по-другому. Code Review — не самое подходящее место, чтобы спорить по поводу глобальных архитектурных решений. Код уже написан, задача почти доделана, переписывать все с нуля — слишком затратно. Такие вещи нужно не оставлять на потом, а обсуждать заранее.
Как вариант — проведение архитектурного review, защиты своего компонента перед командой либо на словах, либо в виде схемы. Конечно, бывают случаи, когда кого-то осеняет уже во время ревью — тогда это предложение, если оно действительно ценное, стоит задокументировать в issue-трекере и никогда к нему больше не возвращаться.
В качестве участников можно привлекать технического лидера проекта, разработчиков, непосредственно заинтересованных в реализации этой задачи, и просто всех желающих из команды.
Ограничение размера review
Есть один замечательный парадокс — чем меньше кода на review, тем больше к нему оставляется комментариев. Это правило остается неизменным практически в любой команде. Для нескольких строк кода характерно наличие огромных и развесистых дискуссий. При увеличении объема изменений до нескольких тысяч строк количество комментариев резко падает до отметки одного-двух, на которой и остается.
Эта практика очень простая — нельзя отправлять на ревью слишком много изменений за раз. Это требование вполне коррелирует со стандартными правилами по декомпозиции задач, каждая из которых не должна превышать рабочего дня. Если залить слишком много кода, то у ревьюеров очень быстро замылится глаз — и многие проблемы будут просто пропускаться. Кстати, сюда же можно отнести и дополнительное требование — убирайте весь информационный шум, который мешает ревьюерам сосредоточиться на фактическом смысле изменений.
Self review
Большую часть фактических ошибок в своем коде автор способен увидеть сам. Поэтому перед тем, как подключать к ревью своих коллег, стоит самостоятельно один или два раза внимательно просмотреть свой код. Эта несложная процедура поможет очень сильно сэкономить время и нервы и улучшить отношения с коллегами.
Описание review
Когда проводишь code review на постоянной основе, сложно фокусироваться на фактическом смысле изменений — об этом я уже упоминал. Еще один способ помочь ревьюерам удерживать фокус — детально описывать смысл изменений. Обычно я работаю по следующему шаблону:
Чек-листы и их автоматизация
Чек-листы — это круто. Используйте их и для code review. Примеры вопросов из такого чек-листа: «Есть ли закомментированный код?», «Покрыта ли бизнес-логика тестами?», «Обрабатываются ли все ошибки?», «Вынесены ли строчки в константы?». Запоминайте все вопросы, которые часто возникают у команды на review, и заносите их в список.
Главное — не давать этому чек-листу разрастаться. Думайте о способах автоматизации каждой из перечисленных там проверок — если приложить достаточно усилий, большую часть потребностей вы сможете закрыть. Есть огромное количество инструментов, которые могут помочь в этой работе — линтеры, статические анализаторы, детекторы дублирующегося кода.
Измерение эффективности Code Review
Внедрение любого процесса должно сопровождаться сбором метрик его эффективности и их дальнейшим анализом. Сам процесс — не самоцель. Code review — не исключение, важно уметь понять, насколько сильно он помогает команде в ее работе.
Удобный вариант — исходить из целей, которых мы хотим достичь при внедрении code review. В качестве шпаргалки можно использовать те, которые я обозначил в первом разделе. Один из способов измерения эффективности достижения таких целей — использование подхода Goal-Question-Metric. Состоит он из трех этапов — определение цели, которую мы хотим измерить, постановка вопросов, которые помогут понять уровень достижения цели, и определение метрик, которые помогают ответить на поставленные вопросы. Разберем на примере.
Сначала нужно определить намеченную цель. Как пример — «Повысить уровень переиспользования кода». Можно и даже нужно использовать сразу нескольких целей — и измерять их независимо друг от друга.
Затем сформулируем вопросы, на которые нужно ответить, чтобы понять, достигаем ли мы цели. В нашем случае это «Используем ли мы общие библиотеки в проекте?», «Вынесены ли визуальные стили в отдельный компонент?», «Много ли кода дублируется?».
Ну и последнее — определение метрик, которые помогут ответить на вопрос. В нашем случае это количество общих библиотек, количество мест, где остались захардкоженные стили, уровень дублирования кода.
Использовать фреймворк довольно просто. Измеряем обозначенные метрики до внедрения процесса, определяем желаемый результат, и периодически, раз в месяц или в квартал, повторяем эти измерения. Если их можно автоматизировать — еще круче. Спустя какое-то время анализируем результат и определяем, достаточно ли изменились метрики, чтобы счесть процесс эффективным.
Почему эта метрика действительно хороша? Представьте, что большую часть времени разработчики занимались не предложениями по переиспользованию кода, а холиварами на тему пробелов или табов — с такими ревью эти ключевые показатели не сильно бы выросли, нужного результата мы бы не достигли и поняли бы, что что-то с процессом не то.
Кроме того, полезно собирать еще некоторые метрики, которые могут подсказать, с какими проблемами при использовании процесса мы столкнулись.
Если собирать такие данные по каждому ревью, то можно вполне адекватно оценить уровень вовлеченности в процесс у каждого участника команды. Ряд систем для проведения code review автоматически собирает такие данные — например, Crucible от Atlassian.
Заключение
Как я уже упоминал во введении, эта статья — краткая выжимка моего выступления, в котором я затрагиваю еще и различные инструменты для автоматизации процесса, правила и нормы поведения в команде и прочие аспекты проведения code review. Чтобы не пропустить появление записи — подписывайтесь на мой Telegram-канал, куда я обязательно выложу его по готовности. Если у вас остались вопросы — приходите на наш митап в субботу, 17 июня, и обсудим их в неформальной обстановке.
Ревью кода: успешный опыт
В Интернете найдется масса информации по ревью кода:
Этот текст взят из рекомендаций по повышению качества, составленным на основе работы с Baseline, нашим инструментом для контроля качества кода на Java. В нем рассмотрены следующие темы:
Комикс XKCD ‘Качество кода’, скопировано по лицензии CC BY-NC 2.5
Мотивация
Мы занимаемся ревью кода (РК) для повышения качества кода и хотим таким образом положительно повлиять на командную и корпоративную культуру. Например:
— Автор кода может использовать прием или алгоритм, на котором рецензенты сами чему-то научатся. В целом ревью кода помогает повысить планку качества в рамках всей организации.
— Рецензенты могут знать о приемах программирования или о самой базе кода нечто такое, что поможет улучшить или консолидировать внесенные изменения; например, кто-нибудь может в то же самое время разрабатывать или дорабатывать точно такую же возможность.
— Положительный контакт и коммуникация усиливают социальные связи между членами команды.
На что обращать внимание
Однозначно верного ответа на этот вопрос не существует, и в каждой команде разработчиков требуется согласовать собственный подход. В некоторых коллективах предпочитают проверять любое изменение, вносимое в основную ветку разработки, а в других учитывается «порог тривиальности», по превышении которого проверка является обязательной. В данном случае приходится выдерживать баланс между эффективным использованием времени программиста (как автора, так и рецензента кода) и поддержанием качества кода. В некоторых строгих контекстах порой требуется проверять в коде все, даже самые тривиальные изменения.
Правила ревью кода одинаковы для всех: даже если вы самый старший разработчик в команде, это еще не означает, что ваш код не требует ревью. Даже если (бывает и так) код безупречен, ревью открывает возможности для менторства и сотрудничества, и, как минимум, помогает взглянуть на базу кода с разных точек зрения.
Когда проверять
Ревью кода должно происходить после успешного прохождения автоматических проверок (тесты, оформление, другие этапы непрерывной интеграции), но до слияния нового кода с главной веткой репозитория. Обычно мы не прибегаем к формальной проверке суммарных изменений, внесенных в код с момента последнего релиза.
При сложных изменениях, которые должны добавляться в главную ветку кода единым блоком, но настолько обширных, что их фактически нельзя охватить за одну проверку, попробуйте поэтапное ревью. Создаем первичную ветку feature/big-feature и ряд вторичных (напр., feature/big-feature-api, feature/big-feature-testing, т.д.), в каждой из которых инкапсулировано подмножество общего функционала, и каждая такая ветка сверяется с основной веткой feature/big-feature. После того как все вторичные ветки будут слиты в feature/big-feature, создаем ревью кода для вливания последней в главную ветку.
Подготовка кода к ревью
Автор кода обязан предоставлять код на ревью в удобоваримом виде, чтобы не отбирать у рецензентов лишнего времени и не демотивировать их:
— Крупные изменения, связанные с рефакторингом, нарушают механизмы перемещения, отбора и другую «магию», связанную с контролем исходников. Очень сложно отменить такое изменение поведения, которое было внесено после завершения рефакторинга, охватившего весь репозиторий.
— Дорогое рабочее время, которое человек тратит на рецензирование кода, должно уходить на проверку программной логики, а не на споры о стиле, синтаксисе или форматировании кода. Такие вопросы мы предпочитаем решать при помощи автоматических инструментов, в частности, Checkstyle, TSLint, Baseline, Prettier и т.д.
Коммит-сообщения
Далее приведен пример грамотного коммит-сообщения, которое выдержано в соответствии с широко применяемым стандартом:
Старайтесь описать и изменения, вносимые при коммите, и как именно эти изменения делаются:
Поиск рецензентов
Обычно автор коммита подыскивает одного-двух рецензентов, знакомых с базой кода. Зачастую в таком качестве выступают руководитель проекта или старший инженер. Владельцу проекта целесообразно подписываться на собственный проект, чтобы получать уведомления о новых ревью кода. При участии троих и более рецензентов ревью кода зачастую получается непродуктивным или контрпродуктивным, поскольку разные рецензенты могут предлагать взаимно противоречивые изменения. Это может свидетельствовать о фундаментальных разногласиях по поводу верной реализации, и такие проблемы должны решаться не во время ревью кода, а на расширенном собрании, в котором лично или в режиме видеоконференции участвуют все заинтересованные стороны.
Выполнение ревью кода
Ревью кода – это точка синхронизации между разными членами команды, поэтому потенциально она может стопорить работу. Поэтому ревью кода должно быть оперативным (занимать порядка нескольких часов, а не дней), а члены и руководители команды должны осознавать, сколько времени потребуется на проверку, и соответствующим образом приоритезировать выделяемое на нее время. Если вам кажется, что вы не успеете закончить ревью вовремя, сразу скажите об этом автору коммита, чтобы он успел найти вам замену.
Ревью должно быть довольно основательным, чтобы рецензент мог объяснить изменение другому разработчику с достаточной детализацией. Так мы добьемся того, чтобы подробности о базе кода были известны хотя бы двоим, а не одному.
Вы как рецензент должны настойчиво придерживаться стандартов качества кода и держать его на высоком уровне. Ревью кода – скорее искусство, чем наука. Научиться этому можно только на практике. Опытный рецензент должен попробовать сначала подпустить к изменениям менее опытных коллег и позволить им сделать ревью первыми. Если автор следовал вышеизложенным правилам (особенно тем, что касаются самопроверки и предварительного запуска кода), то вот на что должен обратить внимание рецензент при ревью:
Реализация
Удобочитаемость и стиль
Удобство поддержки
Уважайте рецензируемого. Хотя, позиция оппонента здравая, не вы реализовали эту возможность, и не можете все за него решать. Если не удается прийти с рецензируемым к общему мнению по поводу кода, устройте консультацию в реальном времени и выслушайте мнение третьего человека.
Безопасность
Убедитесь, что на всех конечных точках API выполняется адекватная авторизация и аутентификация, соответствующая правилам, принятым в остальной базе кода. Ищите другие распространенные изъяны, например: слабую конфигурацию, вредоносный пользовательский ввод, недостающие записи в логах, т.д. Если сомневаетесь – покажите рецензируемый код специалисту по безопасности.
Комментарии: краткие, вежливые, побудительные
Рецензия должна быть краткой, выдержанной в нейтральном стиле. Критикуйте код, а не автора. Если что-то непонятно – попросите пояснить, а не считайте, что все дело в невежестве автора. Избегайте притяжательных местоимений, особенно в оценочных суждениях: «мой код работал до ваших изменений», «в вашем методе есть баг» и т.д. Не рубите сплеча: «это вообще не будет работать», «результат всегда неверен».
Старайтесь разграничивать рекомендации (напр., «Рекомендация: извлеките метод, так код станет понятнее»), необходимые изменения (напр., «Добавить Override») и мнения, требующие разъяснения или обсуждения (напр., «В самом ли деле это поведение верное? Если так — пожалуйста, добавьте комментарий, поясняющий логику»). Попробуйте поставить ссылки или указатели на подробное описание проблемы.
Закончив ревью кода, укажите, насколько подробной реакции на ваши комментарии вы ожидаете от автора, хотели бы вы повторить ревью после того, как изменения будут внесены, например: «Можете смело заливать код в главную ветку после этих небольших исправлений» или «Пожалуйста, учтите мои замечания и дайте мне знать, когда я смогу снова посмотреть код».
Реакция на рецензию
Ревью кода, в частности, призвано улучшить предложенный автором запрос на изменения; поэтому не принимайте в штыки рекомендации рецензента, отнеситесь к ним серьезно, даже если не согласны с ними. Отвечайте на любой комментарий, даже на обычные «ACK» (одобрено) или «done» (готово). Объясните, почему приняли то или иное решение, зачем у вас в коде те или иные функции, т.д. Если не можете прийти к общему мнению с рецензентом, устройте консультацию в реальном времени и выслушайте мнение стороннего специалиста.
Исправления должны фиксироваться в той же ветке, но в отдельном коммите. Если склеивать коммиты на этапе рецензирования, то рецензенту будет сложнее отслеживать изменения.
Политика слияния кода в разных командах отличается. В некоторых компаниях слияние кода доверяют только владельцу проекта, в других это может делать и участник после того, как его код будет просмотрен и одобрен.
Ревью кода тет-а-тет
Как правило, для ревью кода отлично подходят асинхронные diff-подобные инструменты, например, Reviewable, Gerrit или GitHub. Если речь идет о сложном ревью или о ревью, участники которого сильно отличаются по уровню опыта или профессионализма, ревью бывает эффективнее проводить лично — либо сидя вдвоем перед одним монитором или проектором, либо удаленно, через видеоконференцию или инструменты для совместного просмотра экрана.
Примеры
В следующих примерах комментарии рецензента в листингах вводятся при помощи
Best practices в Code Review
Правильный процесс ревью кода — это процесс контроля и итеративного улучшения качества продукта.
Контроля того:
1) что задача выполнена в полном объёме и соблюдены все критерии приемки.
2) что соблюдены общие правила и договорённости.
3) что решение не избыточно, его легко поддерживать и масштабировать в будущем.
Для начала будет хорошо задать своей команде такие вопросы:
Сколько времени занимает ревью кода для средней задачи?
Как у вас в команде понимают, что ревью задачи было сделано правильно?
Как вы пытаетесь уменьшить время ревью?
Командное владение кодом
Изменение фактора автобуса, за счет того, что каждый кусок в коде смотрела больше одной пары глаз, и знания не принадлежат одному конкретному члену команды.
Переиспользование кода
Когда несколько человек, решающих разные задачи и обладающие разными знаниями о продукте, смотрят код друг друга, то они могут заметить определенные закономерности и предложить переиспользовать 1 из уже готовых решений, либо вынести текущее в отдельный модуль/библиотеку.
Обмен знаниями внутри команды
У каждого в команде есть чему поучиться. Кто-то хорош в структурах данных и алгоритмах, другой разбирается в асинхронщине и базах данных, а кто-то мастер MVI и операционных систем. Возможность посмотреть на решения друг друга и задать вопросы это отличный способ поднять технический уровень всей команды.
Выявление ошибок
Единообразие и простота проекта
Сколько есть на свете людей, столько и разных взглядов на то, как именно нужно писать код. Табы и пробелы, файловая структура проекта — при отсутствии консистентности это может сильно усложнить процесс чтения и анализа кода.
Рефлексия или self review
Большую часть ошибок в своем коде автор способен увидеть сам.
Поэтому перед тем, как подключать к ревью своих тиммейтов, стоит самостоятельно хоть раз внимательно просмотреть свой код.
Эта простая процедура поможет сильно сэкономить время и нервы, а также улучшит отношения с коллегами)
Скорость code review
Скорость команды в целом снижается. Да, если человек не отвечает быстро на код-ревью, он делает другую работу. Тем не менее, для остальной части команды новые функции и исправления ошибок задерживаются на дни, недели или месяцы, поскольку каждый PR ждёт код-ревью и повторного код-ревью.
Медленные ревью также препятствуют очистке кода, рефакторингу и дальнейшим улучшениям существующих PR.
Разработчики начинают протестовать против процесса код-ревью. Если рецензент отвечает только через несколько дней, но каждый раз запрашивает серьёзные изменения, это может быть неприятно и сложно для разработчиков. Часто это выражается в жалобах на то, насколько «строгим» является рецензент. Если рецензент запрашивает те же существенные изменения (изменения, которые действительно улучшают работоспособность кода), но каждый раз реагирует быстро, жалобы обычно исчезают. Большинство жалоб на процесс код-ревью на самом деле решаются путём ускорения процесса
Как быстро выполнять код-ревью?
Если вы не в середине сосредоточенной задачи, то должны сделать код-ревью вскоре после его поступления.
Один рабочий день — максимальное время для ответа (т. е. это одна из первых задач на следующее утро).
Следование этим рекомендациям означает, что типичный PR должен получить несколько раундов ревью (если это необходимо) в течение одного дня.
Скорость и отвлечения
Есть один момент, когда приоритет личной скорости превосходит командный. Если вы в середине сосредоточенной задачи, такой как написание кода, не отвлекайтесь на код-ревью. Есть исследования, о том что у разработчика может занять немало времени плавный вход в контекст, после отвлечения.Тем самым, отвлечение от кодинга на деле обходится команде не дешевле, чем просьба к другому разрабу подождать код-ревью.
Вместо этого дождитесь точки останова в своей работе. Например, после завершения текущей задачи, после обеда, возвращаясь со встречи, возвращаясь с офисной мини-кухни и т. д
Комментарии к PR
Объясните свои рассуждения
Избегайте явных приказов, просто указывая на проблемы и позволяя разработчику решать
Поощряйте разработчиков упрощать код или добавлять комментарии вместо простых объяснений сложности
Хороший пример: Использование этой библиотеки в данном случае является лишним, так как она добавляет большое количество overhead’a, т.е функционала которым мы пока не пользуемся. Думаю в данном случае было бы целесобразно взять только нужную часть и использовать ее в новом модуле.
Плохой пример:
Зачем ты запилил эту либу? Откуда вообще нашел ее? Советую выпилить и самому сделать.
Чек-листы и их автоматизация (DevOps)
Примеры вопросов из такого чек-листа:
«Есть ли закомментированный код?»,
«Покрыта ли бизнес-логика тестами?»,
«Обрабатываются ли все ошибки?»,
«Вынесены ли строчки в константы?».
Запоминайте все вопросы, которые часто возникают у команды на review, и заносите их в список.
Главное — не давать этому чек-листу разрастаться. Думайте о способах автоматизации каждой из перечисленных там проверок — если приложить достаточно усилий, большую часть потребностей вы сможете закрыть. Есть огромное количество инструментов, которые могут помочь в этой работе — линтеры, статические анализаторы, детекторы дублирующегося кода
Архитектурные встречи
Code Review — не самое подходящее место, чтобы спорить по поводу глобальных архитектурных решений. Код уже написан, решение выкачено, задача почти доделана, переписывать все с нуля — слишком затратно и зачастую не оправдано. Такие вещи нужно не оставлять на потом, а обсуждать заранее.
Как вариант — проведение архитектурной встречи, защиты и аргументации своего решения для команды, либо на словах, либо в виде схемы. Зачастую, бывают случаи, когда кого-то осеняет уже во время ревью — тогда это предложение, если оно действительно ценное, стоит задокументировать в issue-трекере и вернуться к нему по мере необходимости.
Видеть хорошее
Нет идеального
Главным point’ом здесь является то, что не бывает «идеального» кода — бывает только код «получше». Не стоит требовать от автора полировать каждый крошечный кусок или фрагмент.
Скорее, рецензент должен сбалансировать необходимость дальнейшего прогресса по сравнению с важностью предлагаемых изменений. Вместо того, чтобы стремиться к идеалу, команда должна стремиться к непрерывному улучшению. Коммит, который в целом улучшает масштабируемость, читаемость и простоту системы, нельзя задерживать на дни или недели, потому что он не «идеален»
Ограничение размера code review
В среднем во всем всегда лучше найти золотую середину. На просторах интернета находится большое количество графиков соотношения размера PR к легкости его просматривания или количеству действительно найденных багов и тд. По опыту и наблюдениям из разных команд могу сказать что следует ограничить PR в своей команде до определенного количества изменений. Например 500, тем самым это поможет облегчить процесс просматривания PR и поможет членам команды научиться правильно дробить и декомпозировать задачу.
Работа по расписанию
Можно внедрить таблицу для code review по расписанию
Code review — улучшаем процесс
Представим команду, где не проводится Code review. Разработчики пишут код, и без проверок вносят все изменения в основную ветку. Спустя время расширяется функционал или находятся баги, они возвращаются к исходному коду, а там все переменные названы одной буквой, нет следования архитектуре, да и качество не самое лучшее. Этот некачественный код будет копиться и однажды наступит момент, когда, при любом мало-мальском нововведении, проект начнёт разваливаться: в лучшем случае – увеличится время разработки, в худшем – поддержка проекта станет невозможной. И это при том, что когда-то давно задача была выполнена и все хорошо работало.
Как этого можно избежать? Ответ на вопрос в названии – Code review.
Code review — это проверка кода на ошибки, повышающая стабильность работы и качество кода.
Pull request / Merge request — это запрос к команде проекта (человеку или группе людей) на одобрение и применение изменений в выбранную ветку. Как только Pull request будет создан, перед одобрением происходит обсуждение написанного кода. Во время обсуждения могут быть предложены правки. После одобрения текущие изменения попадают в выбранную ветку.
Ниже перечислены рекомендации, которые помогут ускорить Code review и повысить его качество.
Поделим вопрос на три части и рассмотрим каждую по отдельности:
Часть 1. Проверяем код
Запускаем код автора у себя
Запустите код у себя и посмотрите, как изменения работают в связке с остальным кодом. Это помогает найти проблемные места, которые не видны в web-интерфейсе. Старайтесь видеть код комплексно, избегайте фокусироваться только на локальных изменениях. Так Вы быстрее разберетесь с кодом и быстрее найдете архитектурные неточности, если такие есть.
Помним про пользовательский опыт
Смотрим на общую логику
Разработчики могут успешно решить свою задачу, но поломать работу других кусков кода. Чтобы такого не происходило, смотрите не только на то, как решается конкретная задача, но и на то, как изменения отразятся на работе других сервисов, модулей и всего проекта в целом.
Смотрим на архитектуру кода
Архитектура кода определяет, как много времени в будущем мы потратим на расширение, добавление функционала или правку бага. Также архитектура кода может повлиять на потенциальное появление багов в случае изменений в проекте. В идеале расширение и добавление нового функционала не должно приводить к рефакторингу.
Пишем проще
Обращайте внимание на переусложнение кода: чем проще код, тем легче он читается и проще его поддерживать. Избавляйтесь от сложных кусков кода.
Многопоточность
Если в проекте подразумевается работа в нескольких потоках, то смотрите, что будет если во время выполнения кода в каком-то из потоков случится задержка, и как отрабатываются подобные кейсы.
Излишняя оптимизация
Как писал классик Дональд Кнут, преждевременная оптимизация – корень всех зол. Оптимизировать лучше только то, что нужно здесь и сейчас.
Отрабатываем ошибки
Обратите внимание, как поведет себя проект, если не удалось выполнить строчку кода, блок кода или запрос на сервер. Часто разработчики прерывают выполнение функции без вывода ошибок, но подобные кейсы необходимо прорабатывать.
Соответствие договоренностям
Код должен соответствовать договоренностям, код стайлу. Единообразие кода – не прихоть, а необходимость: такой код легче поддерживать, и в таком коде легче разбираться.
Наименования и внешний вид
Помните о других программистах, которым придется разбираться в вашем коде. Читабельный код упрощает его дальнейшую поддержку. Имена должны быть понятные и точно описывать класс, объект, функцию и т.д.
Комментарии к коду
Комментарии должны отвечать на вопрос: «почему так сделано?», а не «как это сделано?». Запомните это.
Часть 2. Обсуждаем
Главное правило обсуждения: любой комментарий должен быть нацелен на код, а не на человека, который его написал. Работает и в обратную сторону – не стоит воспринимать комментарии как критику Вас лично. Задача code review сделать Ваш код лучше, ведь то, что не заметили Вы, могут заметить другие. Коллеги могут предложить альтернативные решения, и в этом заключен потенциал для профессионального роста. Важно помнить, что обсуждение кода – это не состязание в остроумии и не показательное шоу «кто больше знает», поэтому сарказм, скрытая агрессия и хамство в нем неуместны.
Как правило, pull request проводят на специальных web-хостингах (github.com, bitbucket.org, gitlab.com и др.), где есть возможность просмотреть изменения и оставить комментарий к определенному фрагменту кода.
Соблюдаем договоренности
Повторим, код должен соответствовать договоренностям. Однако если таких договоренностей не существует, не стоит просить коллегу добавить пробел или отступ в коде.
В спорных моментах можно договориться всей командой прямо в процессе code review, но просить соблюдать эти правила лучше на следующих code review, так всем будет легче их принять. Кстати, задокументированный гайдлайн по стилю написания убирает практически все споры.
Предлагаем альтернативу
Избегайте высказываний типа «ты сделал не так. », «зачем, почему ты так пишешь?» и др. Они воспринимаются как критика и ведут к оправданиям. Лучше писать комментарий про содержание кода, без обращения к личности автора. Также старайтесь избегать приказов и принуждений: люди не любят, когда им кто-то приказывает, и воспринимают такие комментарии негативно.
Можно действовать по следующей схеме:
Остаемся в рамках задачи
Часто можно увидеть комментарии к коду, который был раньше и никак не трогался. Не надо комментировать то, что не относится к задаче. Любые сторонние правки могут занять много времени, да и восприниматься могут негативно, поэтому лучше смотреть на то, как человек выполнил текущую задачу, а не просить его рефакторить проект.
Хвалим
Если видите интересное или крутое решение, не стесняйтесь хвалить. К тому же, это отличная мотивация для ваших коллег продолжать писать хороший код в дальнейшем.
Все комментарии равны
Часто бывает, что один программист технически знает больше другого, что подтверждается градацией junior, middle, senior, team lead. Не стоит выделять комментарии одной группы как более важные. Это обесценивает мнение части разработчиков, что может привести к равнодушию и формальному участию в code review. Когда мнение всех одинаково важное, code review проходит продуктивнее.
Четко выражаем свои мысли
Для продуктивного общения пишите максимально развернуто и объясняйте каждую деталь. У всех разный уровень знания, а читать мысли еще пока никто не научился.
Задаем вопросы
Не стесняйтесь спрашивать своих коллег, чем их предложенный вариант лучше текущего вашего. Это отличная возможность узнать что-то новое и вырасти профессионально.
Решаем конфликты
Случается, что человек не принимает все доводы и своих предложить не может, отказывается что-то делать. Несколько практических советов на этот случай:
Часть 3. Улучшаем процесс
Описываем, что сделали
Описываем в заголовке pull request (или выносим в отдельный комментарий) суть задачи и какие шаги были проделаны для ее выполнения.
Делим pull request на части
Большой кусок будут долго смотреть, долго обсуждать и долго исправлять. Поделите код на небольшие логические части – так процесс пойдет гораздо быстрее.
Отвечаем на все комментарии
Желательно отвечать на каждый комментарий, чтобы в команде не возникало недоговоренностей. Другие разработчики должны понимать, что Вы прочитали их комментарий, проделали необходимую работу и внесли исправления. Постоянно открывать pull request и смотреть, что было поправлено, а что нет, очень неудобно и отнимает много времени.
Искать все?
Существуют разные подходы – искать максимум из возможного или комментировать сначала важные архитектурные моменты, а после исправления обращать внимание на мелочи.
Оба способа имеют право на жизнь. Я считаю, что второй вариант более трудозатратный: представьте, что после исправления надо полностью просматривать код еще раз, комментировать и снова ждать исправлений. Куда быстрее тщательно пройтись по коду один раз, оставить комментарии и потом проверить исправления.
Если есть архитектурные неточности и понятно, что мелкие ошибки сами исчезнут после исправления архитектуры, не стоит тратить время на комментирование мелочей в этом участке кода.
Ждать всех?
Можно не ждать пока pull request одобрят все и договориться, что одобрения 80% ревьюверов достаточно для закрытия задачи. Это ускорит попадание кода в основную ветку, что несомненно более выгодно для бизнес-процессов, но может привести к разногласиям в команде и равнодушию к code review.
Второй вариант – обязательно ждать одобрения всех причастных разработчиков. Качество кода вырастет, но сам процесс замедлится значительно. Выбор в пользу скорости или качества каждая команда должна принимать самостоятельно.
Мелочи
Если к коду нет серьезных замечаний, то не нужно ждать, когда будут убраны все маленькие неточности. Их можно указать в комментарии и сразу одобрить pull request – автор кода будет чувствовать себя спокойнее, повысится его лояльность к команде, почувствует, что ему доверяют. И, конечно, скорость прохождения pull request возрастет.
Как провести код-ревью: советы по применению
Разбираемся, кому и когда нужна (и не нужна!) дополнительная проверка кода, и каких ошибок избежать на старте.
Текст будет полезен разработчикам и лидам, которые еще близко не знакомы с код-ревью или хотят упорядочить свои знания, узнать лайфхаки из практики.
Код-ревью — это процесс проверки кода, который позволяет:
→ читаемость и понятность кода для других разработчиков.
→ архитектурные решения. Например, разбиение на модули, code style решения, неверно подобранный паттерн проектирования.
→ работу в команде. Способствует диалогу между автором кода и ревьюером, дает возможность прокачать навыки и узнать что-то новое.
В код-ревью нет жестких правил о том, кто именно должен проводить проверку. Идеальный сценарий, когда ревью осуществляет более опытный коллега — например, тимлид или ведущий разработчик проекта. В реальности так выходит не всегда: часто мидл проверяет мидла.
Системная практика оценки кода внутри команды — не всегда полезное с точки зрения затраты ресурсов решение. Оно отнимает время и у автора, и у проверяющего, а еще требует глубокого погружения в процессы и сил на коммуникацию.
Причины, чтобы отказаться от код-ревью в конкретном случае:
нет человека, который обладает экспертизой в области специфики задачи;
Причины, чтобы отказаться от постоянной практики код-ревью:
у всех разработчиков одинаковый уровень знаний, навыков и уровень погружения в контекст;
Перед стартом ревьюер должен оценить объем MR и определить, сможет ли его проверить на «одном дыхании» — не теряя концентрации. Если объем MR слишком большой, советуем разбить его на части поменьше. Чем объемнее решение, тем ниже эффективность проверки.
В первом раунде проверяющему важно оценить код на предмет высокоуровневых, глобальных проблем. Это, например, неверно выбранный подход к проектированию решения или разбиение на функции, отсутствие модульности.
Если таких проблем не нашлось, уровень проверки понижается до тех пор, пока не найдутся места для улучшений.
В первом раунде не стоит акцентировать внимание на мелких недочетах. Скорее всего, автор сам их обнаружит и поправит, и ревьюеру не придется тратить время на поиск незначительных проблем.
После код возвращается автору: он вносит изменения и снова отправляет на проверку. Процесс продолжается до тех пор, пока решение не признают приемлемым — оно одобряется тем, кто проводил ревью, и после assignee выполняет merge.
«Решение не должно быть идеальным — оно должно соответствовать потребностям проекта и выполнять поставленную задачу», — отмечает разработчик Антон Щербак.
→ Оно не имеет явных ошибок.
Ревью может выявить недочеты, которые видны без запуска проекта. Обычно это:
→ Соответствует стайл-гайдам, принятым в команде.
В команде должен быть принят свод правил, по которым ведется разработка ПО. Он нужен для того, чтобы соблюдался единый стиль и было проще разобраться в контексте.
Если автор решения выходит за рамки принятых стайл гайдов или отклоняется от них, стоит указать ему на это.
→ Не затрагивает код, выходящий за рамки задачи.
Решение должно быть выполнено в рамках скоупа задачи. Если разработчик заметил, что можно выполнить рефакторинг другого класса, это лучше сделать в отдельном MR. При таком подходе ревьюеру будет проще проверять выполнение исходной задачи.
→ Пропущено через линтеры, используемые в проекте.
Большинство команд в Selectel использует pre-commit — так при каждом коммите код прогоняется через линтеры. Для Python используется black, isort, flake8, pyupgrade и autoflake.
→ Обладает хорошей читаемостью и пояснением неочевидных мест.
Лучшая практика в разработке — написание самодокументируемого кода. Если по каким-то причинам ей воспользоваться не получается, советуем оставить поясняющий комментарий: он расскажет ревьюеру, что происходит в этом месте проекта.
Но увлекаться комментариями не стоит: их количество увеличивает объем текста, который нужно будет прочитать другим разработчикам.
→ Если на проекте пишутся автотесты, решение должно ими покрываться.
Команда принимает решение об использовании автотестов для увеличения надежности сервиса. Если эта практика уже используется, ее стоит поддерживать. При выпуске патчей иногда нужно чуть переписать тест, а при минорных версиях — всегда написать новые.
→ Нет оверинжениринга (кода «на будущее»).
Часто код, который решает еще не возникшие проблемы, не пригождается и становится лишним.
Еще Дональд Кнут говорил: преждевременная оптимизация — корень всех проблем в программировании.
Хорошая практика — восприятие код-ревью как отдельной задачи без переключения на другие. Правда при условии, что у остальных дел невысокий приоритет.
Разработчики часто спорят о названиях переменных. Бывает, что эти нововведения никак не влияют на итоговое решение. Поэтому делать пометку в начале — нормальная практика: она говорит разработчику, что решение о внесении в код изменений остается за ним. Эти изменения не обязательны — решение и так хорошее.
Помните о том, что обратная связь должна быть балансной. Ее цель — не обидеть человека, а аргументировано (например, с помощью примеров кода и ссылок на паттерны) подсветить зоны для улучшений.
Кроме того, не зацикливайтесь только на ошибках: если видите интересное решение или нестандартный подход, похвалите его. Так лишний раз покажите коллеге, что у вас одна цель, и снимите напряжение.
Так не стоит комментировать → «Ты тут ошибся, поправь по моему примеру. Вот ссылка».
Так лучше → «Мы уже сталкивались с аналогичной ситуацией. Вот как эта проблема решилась тут → ссылка. Можем ли реализовать нечто подобное?»
По ссылке конкретные гайдлайны, которыми пользуются в GitLab. В них описано, как построена практика код-ревью в компании.
Подпишитесь на блог Selectel, чтобы не пропустить новые обзоры, новости и кейсы из мира IT и технологий.
PHP Profi
Квест → Как хакнуть форму
12 лучших инструментов для ревью кода для разработчиков (2020) Перевод
Эффективное ревью кода предотвращает попадание багов и ошибок в ваш проект путем улучшения качества кода на ранней стадии процесса разработки софта.
В этой статье мы объясним, что такое код-ревью и изучим популярные инструменты, которые помогают организациям с данным процессом.
Что такое процесс ревью кода?
Основная задача ревью кода заключается в проверке любого нового кода на баги, ошибки, а также на соблюдение стандартов качества, установленных организацией. Процесс ревью кода не должен состоять лишь из одностороннего фидбэка. Следовательно, нематериальная польза код-ревью заключается в коллективном улучшении навыков написания кода в команде.
Далее стоит рассмотреть временные рамки, очередности и минимальные требования принятия запросов на код-ревью.
Наконец, следует учесть как должна даваться обратная связь в процессе ревью кода. Убедитесь, что вы подчеркиваете положительные аспекты кода, попутно предлагая альтернативы в выявленных недостатках.
Ваша обратная связь должна быть достаточно конструктивной, чтобы позволить разработчику понять ваше видение и, при необходимости, начать обсуждение.
-Старайтесь сохранить информативность обратной связи
Проверяющие могут оказаться в «лимбе» достаточно просто, что приводит к более низкой эффективности и даже контрпродуктивности.
Почему ревью кода критичен?
Код-ревью критичен, потому как призван:
Ревью кода впоследствии ведет к улучшению компетенции членов команды. В то время как старший разработчик осуществляет код-ревью, младший разработчик может использовать обратную связь для улучшения своих навыков программирования.
Как выполнить ревью кода?
Существует четыре способа произвести код-ревью.
Ревью «Из-за Плеча»
Такие проверки выполняются за рабочей станцией разработчика, где опытный участник команды просматривает новый код, выдвигая свои предложения в процессе обсуждения. Это самый простой способ проведения код-ревью, который не требует определенной структуры.
Ревью такого типа проводятся до сих пор, неформально, при этом формальный код-ревью также может иметь место. Просмотр «из-за плеча» традиционно происходил лично, в то же время удаленные команды могут следовать этому методу с помощью коллаборативных инструментов.
Почтовая Рассылка
В таком процессе ревью разработчик отправляет diff изменений всей команде разработчиков, как правило, с помощью системы контроля версий (VCS), которые автоматизируют уведомления. Такое сообщение инициирует обсуждение изменений, где члены команды могут запросить будущие изменения, указать на ошибки или запросить уточнения.
—Почтовая рассылка через Google Groups на каждый новый push
Раньше почта была основным способом коммуникации ввиду ее универсальности. Организации с открытым кодом часто поддерживали публичный список рассылки, который также служил средством для обсуждения и предоставления обратной связи по коду.
Несмотря на приход инструментов для ревью кода, списки рассылок до сих пор существуют, хоть и используются они теперь в основном для анонсов и обсуждений.
Парное Программирование
—Парное программирование порой может быть неэффективно
И хотя это может служить хорошим способом просмотра нового кода и обучения разработчиков, парное программирование потенциально является неэффективным ввиду временных затрат. Такой процесс не позволяет ревьюеру заниматься любой другой продуктивной работой во время такой сессии.
С Помощью Инструментов
Такой вид код-ревью включает в себя использование специализированного программного обеспечения (ПО) в целях облегчения процесса ревью кода. Обычно инструмент помогает со следующими задачами:
Хотя эти широкие требования инструмент проверки кода, современного инструментария может обеспечить несколько других функций. Мы рассмотрим ряд инструментов анализа кода позже в этом посте.
Почему Вам стоит Использовать Инструменты для Код-ревью?
Инструмент интегрируется в ваш цикл разработки для инициации ревью кода перед тем как новый код соединен с главной кодовой базой. Вы можете выбрать инструмент, который совместим с вашим стэком технологий для «бесшовного» внедрения в Ваш рабочий процесс.
К примеру, если Вы используете Git для менеджмента кода, TravisCI для непрерывной интеграции, то убедитесь, что Вы выберете инструмент, который поддерживает эти технологии и подходит для процесса разработки.
Есть два типа тестирования кода в разработке ПО: динамическое и статическое.
Динамический анализ включает в себя проверку кода на следование набору правил и проведение unit-тестов, обычно с помощью предопределенного скрипта. Статическое тестирование кода проделывается после того как разработчик пишет новый код для присоединения к текущему коду.
Теперь давайте рассмотрим одни из самых популярных инструментов для код-ревью.
Более Детальный Разбор 12 Мощных Инструментов для Код-ревью
В этом разделе мы будем обозревать самые популярные статические инструменты для код-ревью.
1. Review Board
—Обзор Review Board
Вы можете внедрить Review Board с широким охватом систем контроля версий — Git, Mercurial, CVS, Subversion и Perforce. Также можете связать Review Board с Amazon S3 для хранения скриншотов непосредственно в инструменте.
—Обзор изменений в Review Board
Review Board позволяет выполнять как pre-commit, так и post-commit код-ревью в зависимости от требований. Если вы не интегрировали систему контроля версий, можете использовать diff-файл для загрузки изменений кода в инструменте для ревью.
Графическое сравнение изменений в коде также предоставлено. Вдобавок к ревью кода, Review Board позволяет проводить ревью документов.
Первая версия Review Board вышла более десятилетия назад, однако он до сих пор в активной разработке. Поэтому сообщество Review Board за все эти годы возросло, и вы с большой долей вероятности получите помощь, если имеются какие-либо проблемы при использовании программы.
Review Board – простой инструмент для код-ревью, который можно хостить на своём сервере. Вам стоит попробовать его, если не хотите хостить свой код на публичном веб-сайте.
2. Crucible
Crucible – это коллаборативная программа для ревью кода от Atlassian. Она представляет собой коммерческий набор инструментов, позволяющий вам проводить код-ревью, обсуждать планы и изменения, а также обнаруживать баги через множество систем контроля версий.
В обоих случаях вам предлагается 30-дневный бесплатный период без требования данных вашей карты.
Схоже с Review Board, Crucible поддерживает большое количество систем контроля версий – SVN, Git, Mercurial, CVS и Perforce. Базовая функция – позволить проводить ревью кода. Вдобавок к общим комментариям к коду, он позволяет писать inline-комментарии внутри diff view, чтобы точно указать на то, что вы хотели сказать.
Crucible хорошо внедряется с другими продуктами для организаций от Atlassian, например Confluence и Enterprise BitBucket. Хотя, возможно, вы получите наибольшую пользу от Crucible, используя его вместе с Jira, Issue от Atlassian и Project Tracker. Это позволит выполнять pre-commit-ревью и аудиты добавляемого кода.
3. GitHub
Если вы используете GitHub для поддержания ваших Git-репозиториев в облаке, то вы, вероятно, уже использовали форки (forks) и pull-запросы (pull requests) для ревью кода.
—GitHub в Pull-запросе
GitHub позволяет ревьюеру, который имеет доступ к репозиторию, «привязывать» себя к pull-запросам и завершать ревью. Разработчик, который принял pull request, может также запросить ревью у администратора.
В дополнение к обсуждению на общем pull-запросе, вы можете анализировать diff, писать строчные (inline) комментарии, и проверять историю изменений. Инструмент ревью кода также позволяет разрешать простые конфликты в Git через веб-интерфейс. GitHub даже позволяет интегрировать дополнительные инструменты для ревью через маркетплейс, для большей надежности.
4. Phabricator
Phabricator поддерживает три самых популярных системы контроля версий — Git, Mercurial, и SVN. С его помощью можно управлять локальными репозиториями и отслеживать внешне размещенные репозитории. Также, можете масштабировать его до нескольких серверов.
Свыше стандартного инструмента ревью кода
Phabricator предоставляет детализированную платформу для общения с участниками команды. Вы можете либо совершить pre-commit ревью нового сотрудника, либо провести ревью на недавно представленный код. Также можете провести ревью на присоединенный (merged) код, такая функция называется “аудит”. Вот сравнение между код-ревью и аудитом в Phabricator.
Дополнительные инструменты Phabricator помогают в общем цикле разработки программного обеспечения. Например, он предоставляет встроенный трекер, чтобы отслеживать баги и особенности. Вы также можете создать вики-страницу для своего программного обеспечения через Phriction. Для того чтобы интегрировать инструмент в юнит-тесты, можете использовать инструмент CLI. Помимо этого, вы можете строить приложения через Phabricator с помощью его API.
Подводя итог, Phabricator предоставляет массу возможностей, которые помогут сделать процесс разработки более эффективным. Есть особый смысл выбрать этот инструмент, если проект находится на ранней стадии. Если вы не обладаете необходимыми знаниями, чтобы установить его на своем сервере, следует выбрать хостируемую версию программы.
5. Collaborator
-Collaborator Review Source
Collaborator поддерживает большое количество систем контроля версий как Subversion, Git, CVS, Mercurial, Perforce, и TFS. Он хорошо справляется с интеграцией в популярные инструменты управления проектами и IDE (интегрированные среды разработки), такие как Jira, Eclipse, и Visual Studio.
Этот инструмент также позволяет делать отчеты и анализировать ключевые показатели, характеризующие эффективность код-ревью. Кроме того, Collaborator помогает в управлении аудитом и отслеживании багов. Если ваш стек технологий включает в себя корпоративное программное обеспечение, и если вам нужна поддержка для настройки процесса ревью кода, стоит попробовать Collaborator.
6. CodeScene
CodeScene является инструментом ревью кода, который выходит за рамки традиционного статического анализа кода. Он осуществляет поведенческий анализ с помощью добавления временного измерения для анализа развития вашей кодовой базы. CodeScene в двух формах: облачное решение и локальное решение.
-Анализ в ревью кода CodeScene
CodeScene обрабатывает историю контроля версий для визуализации кода. Вдобавок к этому, он применяет алгоритмы машинного обучения для выявления социальных закономерностей и скрытых рисков в коде.
Через историю контроля версий CodeScene профилирует каждого члена команды, чтобы отрисовать диаграмму их базы знаний и создания внутрикомандных зависимостей. Он также вводит концепцию хот-спотов (hotspots) в репозитории путем определения файлов, которые подвергаются наиболее активной разработке.Эти хот-споты требуют высокого внимания в дальнейшем.
-Knowledge-диаграммы в CodeScene
Если вы ищете инструмент, который выходит за рамки стандартного, диалогового инструмента ревью кода, однозначно попробуйте бесплатную пробную версию CodeScene. Чтобы узнать больше о лежащей в основе логике CodeScene в поведенческом анализе кода, взгляните на этот документ про Сценарии ис пользования CodeScene.
7. Visual Expert
Бесплатная пробная версия доступна, но для этого нужно отправить запрос, чтобы узнать цену.
В дополнение к традиционному код-ревью, Visual Expert анализирует каждое изменение в коде, чтобы предвидеть возможные проблемы с его исполнением в связи с изменениями. Также, инструмент может автоматически генерировать полную документацию приложения из кода.
Если вы используете PowerBuilder, SQL-сервер или Oracle PL/SQL и хотели бы специализированный инструмент для ревью кода для ваших потребностей, стоит попробовать Visual Expert.
8. Gerrit
Gerrit сочетает в себе функциональность багтрекера и инструмент для код-ревью. В ходе ревью изменения отображаются бок-о-бок в едином diff, с возможностью начать обсуждение по каждой добавленной строке кода. Этот инструмент работает как промежуточный этап между разработчиком и центральным репозиторием. Кроме того, Gerrit также включает в себя систему голосования.
Если вы обладаете техническими знаниями для установки и настройки Gerrit и ищете бесплатный инструмент для ревью кода, он станет идеальным решением для ваших проектов.
9. Rhodecode
Rhodecode позволяет команде эффективно взаимодействовать через итеративный, диалоговый код-ревью, для повышения качества кода. Инструмент дополнительно содержит слой управления доступом для защищенной разработки.
Кроме того, визуальный changelog (история изменений) помогает вам ориентироваться в истории вашего проекта в различных ветках. Онлайн-редактор кода также предоставлен для внесения небольших изменений через веб-интерфейс.
Rhodecode легко интегрируется в существующие проекты, что делает его отличным выбором для тех, кто ищет веб-инструмент для код-ревью. Следовательно, комьюнити-версия идеально подходит для людей с техническими знаниями, которые ищут бесплатный и надежный инструмент для код-ревью.
10. Veracode
Veracode предоставляет набор инструментов для код-ревью, которые позволяют автоматизировать тестирование, ускорить разработку, интегрировать процесс обновления и повысить эффективность проекта. Набор инструментов для код-ревью от Veracode позиционируется как решение по безопасности, которое ищет уязвимости в ваших системах. Они представляют собой набор из двух инструментов для ревью кода:
Ревью кода является частью Анализа Состава ПО, и вы можете выбрать демо-версию Veracode перед полным переходом на данный инструмент. Вот ссылка для запроса котировки.
11. Reviewable
Если хотите взглянуть на типичный ревью в Reviewable, можете попробовать демо код-ревью.
Интересный момент в Reviewable заключается в том, что он преодолевает некоторые недостатки ревью кода в pull-запросах GitHub. Например, комментарий на строке кода автоматически скрывается GitHub’ом, когда разработчик меняет строку, потому что GitHub предполагает, что проблема была устранена. Но, в реальности, это не всегда так.
Кроме того, GitHub имеет сравнительно низкие лимиты строк для отображения diff ‘ов в файле.
12. Peer Review для Trac
Если вы пользуетесь Subversion, Peer Review Plugin для Trac предоставляет бесплатный и открытый вариант проведения код-ревью на проектах. Плагин Peer Review интегрируется в Open-source-проект Trac, который представляет собой Вики-страницу и систему отслеживания вопросов (issues) для разработки проектов.
—Обзор Peer Review Plugin для Trac (Источник)
Trac внедряет Вики и issue-трекер в ваши код-ревью, чтобы обеспечить конечное решение. В то время как базовый функционал сравнения изменений и обсуждения всё так же доступен, плагин позволяет создавать настраиваемые рабочие процессы для ваших проектов.
Например, вы можете назначить задачи, которые можно решить, на такие триггеры, как принятие изменения или подтверждение в код-ревью. Вы также можете создавать настраиваемые репорты на свои проекты.
Ревью кода играет ключевую роль, когда речь идет о повышении эффективности деятельности вашей организации. В частности, использование правильного инструмента поможет устранить избыточность в цикле разработки.
Мы присмотрелись к самым популярным инструментам для код-ревью, доступным в 2020 году, и вот что мы нашли:
Теперь ваша очередь: какой инструмент для ревью кода используете? Почему? Расскажите нам в комментариях!
Как правильно делать код-ревью?
В жизни разработчика наступает момент, когда он начинает проводить код-ревью. Как правило, это один из признаков роста программиста: к его мнению начинают по-настоящему прислушиваться и доверять более серьезные задачи. Конкретный момент, когда разработчику предлагают заняться ревью зависит от структуры компании: есть команды, где это могут делать более-менее все по истечению некоторого времени работы, а есть такие, где процесс ревью целиком лежит на самых старших и опытных коллегах.
Вместе с тем, редко кто рассказывает о том, каких принципов необходимо придерживаться для проведения качественного ревью: каковы главные цели этой процедуры, за чем смотреть в первую очередь, насколько быстро его нужно проводить.
Всвязи с этим, «Руководство компании 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 и с чего начинать ревью.
Как проводить Code Review по версии Google
Вопросы код-ревью меня интересуют очень давно. Много раз возникали те или иные проблемы то с качеством кода, то с климатом в коллективе. И действительно, code review — это если не единственное, то одно из самых главных мест для возникновения конфликтов в коллективе разработчиков.
И вот недавно при подготовке к очередному выпуску подкаста «Цинковый прод» я узнаю, что Google опубликовал свод правил по проведению Code Review, битком набитый ценными мыслями. Весь материал довольно объемный и не влезет в одну статью, поэтому я постараюсь выделить наиболее интересные (мне) мысли.
Термины, используемые в оригинальной статье:
CL — changelist. Но обычно это называют Merge Request или Pull Request, поэтому в статье я буду использовать сокращение MR
LGTM — Looks Good to Me. Короче, когда жмут кнопку «approve». Я буду использовать термин «апрув», как более понятный населению.
Идеальность MR
На практике можно встретить различные крайности при рассмотрении MR. Кто-то всей командой задалбывает замечаниями, пока всё до мелочей не будет исправлено, кто-то смотрит примерно логику и жмёт «апрув».
В Гугловском документе написана интересная мысль. MR не должен быть идеальным, но он должен улучшать кодовую базу. Т.е с каждым привносимым изменением код должен становиться лучше и лучше. И если MR добавляет много хорошего, то не надо придираться к мелочам; выгоднее, чтобы это улучшение попало в код побыстрее.
Никакой MR не должен делать код хуже. Единственное исключение — это если MR является срочным фиксом чего-нибудь.
Свобода делать замечания
Несмотря на то, что нельзя задалбывать мелочами, тем не менее можно свободно эти мелочи писать хоть к каждой строчке. Противоречие с предыдущим пунктом решается просто: мелочи и придирки ревьювер помечает префиксом «nit:» от англ. nitpick (придирка). Такие замечания фиксить необязательно, однако автор MR может захотеть что-то исправить или, даже если нет, учесть на будущее некоторые моменты.
Факты важнее персональных предпочтений
Почти всегда стандартные принципы построения ПО (software design), базирующиеся на лучших практиках, лучше, чем хитровымученные велосипеды. Поэтому предпочтение нужно отдавать им.
Если можно применить несколько стандартных подходов, то это на усмотрение автора.
Прямая цитата для лучшего понимания:
Aspects of software design are almost never a pure style issue or just a personal preference. They are based on underlying principles and should be weighed on those principles, not simply by personal opinion. Sometimes there are a few valid options. If the author can demonstrate (either through data or based on solid engineering principles) that several approaches are equally valid, then the reviewer should accept the preference of the author. Otherwise the choice is dictated by standard principles of software design.
Если ревьювер и автор MR не согласны друг с другом
Сначала идет попытка достигнуть консенсуса в комментариях к MR. Если это не получается, то личное обсуждение. Если все равно не пришли к единому мнению, то привлекать членов команды. Но главное, MR не должен застревать надолго из-за несогласия двух человек.
Скорость проверки MR
Скорость экстремально важна. Если раз в несколько дней давать по одному комменту, то автор MR будет жаловаться, что к нему придираются и не дают работать.
Если MR проверяется очень быстро, то любые замечания не будут являться большой проблемой — ведь их фиксы тут проверят, и таск пройдет дальше.
В Гугле советуют не отвлекаться от фокусировки над своей задачи, но если уж отвлеклись, то просмотреть, нет ли чего-то поревьювить. Например, вернулся с обеда — поревьювил. Пришел на работу — поревьювил. И т.д.
Порядок просмотра MR
Сначала надо просмотреть MR в целом. Надо ли это вообще, в правильном ли месте код (или это должно быть в отдельной библиотеке), есть ли какие-то глобальные проблемы.
Т.е. нет смысла смотреть какие-то детали реализации, если код вообще не туда и не для того.
Вообще, порядок просмотра важен, чтобы как можно быстрее выдавать автору фидбек (смотри предыдущий пункт).
Когда посмотрели MR вцелом, надо пройтись по основным файлам, т.е. проверить самое главное (если непонятно, что самое главное, можно спросить у разработчика).
Опять же, о любых проблемах нужно сообщать сразу, даже если вы еще не досмотрели MR и решили вообще досмотреть его позже.
Далее надо выбрать логичную последовательность просмотра оставшихся файлов. Ни один файл не должен быть пропущен.
На что обращать внимание при просмотре
Слишком большие MR
Слишком большие MR нужно просить разбивать на части. Почти всегда это возможно.
Как писать комментарии на код ревью
Если с вашим мнением не согласны
Нужно разобраться детально. Возможно, вы чего-то не понимаете, запросите больше информации. Возможно наоборот. В любом случае, зачастую понимание приходит только после пары раундов объяснений с обеих сторон.
Боязнь расстроить автора MR
Бывает, что разработчик расстраивается, если ревьювер настаивает на каких-то изменениях. Однако, чаще всего разработчики расстраиваются из-за формы замечания, а не из-за содержания. Будьте вежливы, объясняйте аргументами, и скорее всего всё будет ок.
«Я исправлю это потом»
Если разработчик согласен с тем, что в коде есть проблема, но просит заапрувить MR, обещая, что исправит это в другой раз, то, по мнению ребят из Гугла, это «потом» чаще всего никогда не наступает. Поэтому если MR — не какой-то срочный багфикс прода, то нужно настаивать на исправлении.
Выводы
Мне очень понравился этот документ от Google. Особенно лайфхак со словом «nit», акцент на скорости code review, а также то, что MR не должен быть идеальным. Вроде бы просто, но пока это явно не обдумаешь, до дела не доходит.
Если эта статья покажется интересной читателям и наберет какое-то количество плюсов, то я напишу следующую часть: процесс code review со стороны автора MR.
Алсо, в ближайшем выпуске «Цинкового прода» мы подробно обсудим код ревью с разных сторон. Поэтому не забудьте подписаться на наш подкаст о разработке!
Как провести код-ревью: советы по применению
Текст будет полезен разработчикам и лидам, которые еще близко не знакомы с код-ревью или хотят упорядочить свои знания, узнать лайфхаки из практики.
Что такое код-ревью и зачем его проводят?
Код-ревью — это процесс проверки кода, который позволяет:
выявить → ошибки, пропуски, уязвимости и стилистические недочеты (с точки зрения проекта или принятых в команде правил).
→ читаемость и понятность кода для других разработчиков.
→ архитектурные решения. Например, разбиение на модули, code style решения, неверно подобранный паттерн проектирования.
→ работу в команде. Способствует диалогу между автором кода и ревьюером, дает возможность прокачать навыки и узнать что-то новое.
В код-ревью нет жестких правил о том, кто именно должен проводить проверку. Идеальный сценарий, когда ревью осуществляет более опытный коллега — например, тимлид или ведущий разработчик проекта. В реальности так выходит не всегда: часто мидл проверяет мидла.
Когда не нужно проводить код-ревью?
Системная практика оценки кода внутри команды — не всегда полезное с точки зрения затраты ресурсов решение. Оно отнимает время и у автора, и у проверяющего, а еще требует глубокого погружения в процессы и сил на коммуникацию.
Причины, чтобы отказаться от код-ревью в конкретном случае:
Причины, чтобы отказаться от постоянной практики код-ревью:
Как организовать процесс проверки кода?
Раунды
Перед стартом ревьюер должен оценить объем MR и определить, сможет ли его проверить на «одном дыхании» — не теряя концентрации. Если объем MR слишком большой, советуем разбить его на части поменьше. Чем объемнее решение, тем ниже эффективность проверки.
Заряд батарейки — количество энергии участников ревью от раунда к раунду.
В первом раунде проверяющему важно оценить код на предмет высокоуровневых, глобальных проблем. Это, например, неверно выбранный подход к проектированию решения или разбиение на функции, отсутствие модульности.
Пример комментария.
Если таких проблем не нашлось, уровень проверки понижается до тех пор, пока не найдутся места для улучшений.
«В первом раунде не стоит акцентировать внимание на мелких недочетах. Скорее всего, автор сам их обнаружит и поправит, и ревьюеру не придется тратить время на поиск незначительных проблем», — отмечает разработчик Selectel Антон Щербак.
После код возвращается автору: он вносит изменения и снова отправляет на проверку. Процесс продолжается до тех пор, пока решение не признают приемлемым — оно одобряется тем, кто проводил ревью, и после assignee выполняет merge.
«Решение не должно быть идеальным — оно должно соответствовать потребностям проекта и выполнять поставленную задачу», — резюмирует Антон Щербак.
Как понять, что решение готово?
→ Оно не имеет явных ошибок.
Ревью может выявить недочеты, которые видны без запуска проекта. Обычно это:
→ Соответствует стайл-гайдам, принятым в команде.
В команде должен быть принят свод правил, по которым ведется разработка ПО. Он нужен для того, чтобы соблюдался единый стиль и было проще разобраться в контексте.
Если автор решения выходит за рамки принятых стайл гайдов или отклоняется от них, стоит указать ему на это.
→ Не затрагивает код, выходящий за рамки задачи.
Решение должно быть выполнено в рамках скоупа задачи. Если разработчик заметил, что можно выполнить рефакторинг другого класса, это лучше сделать в отдельном MR. При таком подходе ревьюеру будет проще проверять выполнение исходной задачи.
→ Пропущено через линтеры, используемые в проекте.
Большинство команд в Selectel использует pre-commit — так при каждом коммите код прогоняется через линтеры. Для Python используется black, isort, flake8, pyupgrade и autoflake.
→ Обладает хорошей читаемостью и пояснением неочевидных мест.
Лучшая практика в разработке — написание самодокументируемого кода. Если по каким-то причинам ей воспользоваться не получается, советуем оставить поясняющий комментарий: он расскажет ревьюеру, что происходит в этом месте проекта.
Но увлекаться комментариями не стоит: их количество увеличивает объем текста, который нужно будет прочитать другим разработчикам.
→ Если на проекте пишутся автотесты, решение должно ими покрываться.
Команда принимает решение об использовании автотестов для увеличения надежности сервиса. Если эта практика уже используется, ее стоит поддерживать. При выпуске патчей иногда нужно чуть переписать тест, а при минорных версиях — всегда написать новые.
→ Нет оверинжениринга (кода «на будущее»).
Часто код, который решает еще не возникшие проблемы, не пригождается и становится лишним.
Еще Дональд Кнут говорил: преждевременная оптимизация — корень всех проблем в программировании.
Советы по процессу
Первый: договоритесь о сроках и организации процесса. Например, не отдавайте правки по одной — это отвлекает. Еще не затягивайте процесс на весь день и по возможности не переключайтесь между задачами — это время- и энергозатратно.
Хорошая практика — восприятие код-ревью как отдельной задачи без переключения на другие. Правда при условии, что у остальных дел невысокий приоритет.
Второй: используйте префикс NIT (сокращение от nitpick) для комментариев, которые даны в качестве рекомендаций. Помните: они не обязательны к исполнению, автор волен сам решать, следовать им или нет.
Пример комментария.
«Разработчики часто спорят о названиях переменных. Бывает, что эти нововведения никак не влияют на итоговое решение. Поэтому делать пометку в начале — нормальная практика: она говорит разработчику, что решение о внесении в код изменений остается за ним. Эти изменения не обязательны — решение и так хорошее», — рассказывает разработчик Selectel Антон Щербак.
Третий: помните о том, что обратная связь должна быть балансной. Ее цель — не обидеть человека, а аргументировано (например, с помощью примеров кода и ссылок на паттерны) подсветить зоны для улучшений.Кроме того, не зацикливайтесь только на ошибках: если видите интересное решение или нестандартный подход, похвалите его. Так лишний раз покажите коллеге, что у вас одна цель, и снимите напряжение.
Так не стоит комментировать → «Ты тут ошибся, поправь по моему примеру. Вот ссылка».
Так лучше → «Мы уже сталкивались с аналогичной ситуацией. Вот как эта проблема решилась тут → ссылка. Можем ли реализовать нечто подобное?»
Четвертый: количество раундов в код-ревью не ограничено, но лучше стремиться к сокращению их количества. Каждая новая серия правок отнимает силы участников ревью и тормозит релиз.
Удобные инструменты для код-ревью
По ссылке конкретные гайдлайны, которыми пользуются в GitLab. В них описано, как построена практика код-ревью в компании.
GitHub. Подробнее об инструменте — по ссылке.
Ревью кода в распределенной команде
Здесь описаны мои исследования, как сделать ревизию кода в команде более приятным занятием, которое может дать новый опыт всем участникам. У нас полностью географически распределённая команда, все коммуникации выполняются через интернет, и зачастую асинхронно. Мы используем Trello для описания возможностей продуктов, поодиночке создаём код, отправляем в GitHub пулл-реквесты, а также пользуемся встроенной в GitHub функцией их ревью. Это отличается от просмотра кода лицом к лицу в офисе и даже по видеочату.
Если не подходить к делу всерьёз, то асинхронная и письменная ревизия кода может стать причиной катастрофы в команде, приведя к ухудшению взаимодействия и сотрудничества. Но если все участники будут стараться делать всё хорошо, то такой подход может работать очень эффективно.
Для чего нужно ревью кода?
Прежде чем начать рассуждать о том, как делать и как не делать ревью, полезно подумать о том, для кого в компании оно нужно, и в соответствии с этим вырабатывать требования. И хорошо бы не забывать, что ревью — это не просто поиск багов и проблем в структуре кода.
Ревью нужно для улучшения качества кода и морального духа в команде.
Анализ кода друг друга — один из главных путей взаимодействия между разработчиками во время обычного рабочего дня. Это должен быть вдохновляющий процесс, чтобы все участники ждали его и активно участвовали в нём.
Вот основные задачи, решаемые с помощью ревизий:
Что не надо делать при ревью
Возможно, эта часть покажется вам довольно негативной, но зато после неё идёт часть с приятными вещами!
Есть много разных способов сделать ревью кода неудачным. Воинственные, злые и непредсказуемые комментарии способны лишить команду удовольствия от работы и превратить ревизию в эмоционально тяжёлую обязанность.
Понимание, чего следует избегать при ревью, как раз и отличает ценную часть работы команды от жестокого и неприятного наказания. Эрик Дитрих, «Чего нужно избегать при ревью кода»
Не проводите ревью формально и с минимальными комментариями
(aka узнать, насколько плохо думают о вас коллеги)
Не нужно всего лишь указывать на ошибки, ничего к этому не добавляя.
Разработчики тратят много времени на код и гордятся им, и ревью — их шанс продемонстрировать свою работу. А при формально-минималистичном подходе всё, на что вы можете надеяться, — это комментарии в стиле «Выглядит неплохо, смерджил». Такое ревью достойно названия «Узнай, насколько плохо о тебе думают».
Не думайте, что достаточно лишь «критиковать код, а не кодера»
Это один из самых популярных советов: критикуйте код, а не кодера. Он хорош (если вы критикуете коллегу, а не его код, то у вас проблемы). Но этого недостаточно. Написание кода — творческий процесс, в который разработчик вкладывает сердце и душу. И если вы жестоко и тупо критикуете чей-то код, то тем самым вы критикуете самого человека. Нужно уметь критиковать. Найдите более позитивные способы анализа и вносите свои предложения, как улучшить код.
Следите за тоном
Не переходите на личности. Фразы вроде «Почему ты сделал именно так?» воспринимаются как нападки. Плохой вариант: «То, как ты написал эту функцию, затрудняет её чтение, добавь больше комментариев» (личное обращение подразумевает, что человек что-то сделал не так). Лучше скажите: «Тебе не кажется, что стоит добавить в код дополнительные комментарии, это облегчит чтение функции?» (звучит гораздо корректнее и позволяет сосредоточиться на улучшении кода).
Обойдитесь без требовательных или вызывающих выражений. Избегайте фраз, смысл которых заключается в «ты неправ». Не говорите людям, что они неправы или что они сказали/сделали что-то не то, им будет неприятно. В таких случаях люди могут уходить в защиту, перестают долго и творчески работать и учиться. Избегайте фраз наподобие:
Не надо оскорблений. Избегайте ругательств вроде «дурак» или «идиот», даже если лично вы не считаете их обидными. У подобных слов есть определённый спектр значений, и он точно не улучшит настроение автора кода.
Не используйте нетерпеливых или пассивно-агрессивных оборотов. Всегда будьте сдержанны и дружелюбны, избегайте неприятных фраз, демонстрирующих раздражение и заставляющих людей чувствовать, что они плохо справляются:
Не будьте непрошеным советчиком
Сдерживайте себя и не предлагайте внести в код все изменения, которые вы хотели бы внести. Вы можете деморализовать автора кода, предложив поменять столько, что придётся всё переписать. Так что выбирайте только самое важное и лучшее.
Одна из задач ревью — найти и исправить баги и проблемы в структуре кода. А вовсе не заставить автора написать код так, как это сделали бы вы. Не забывайте, что в программном обеспечении многие вещи можно решить по-разному, в зависимости от вкусов разработчика, и у каждого подхода есть свои плюсы и минусы. Собираясь предложить очередную правку, спросите себя: может быть, это просто моё мнение? Если автор выбрал другие способы, обосновав свой выбор, то лучше поддержите его. Уважайте право автора на собственные решения.
Даже если кто-то написал код не так, как это сделали бы вы, то это ещё не означает, что код написан неправильно. Главное — качественный, удобный в сопровождении код. Если он удовлетворяет этим критериям и принятым в команде правилам, то этого достаточно. Роберт Бог, «Эффективные ревью без боли»
Вы никогда не сможете заставить других написать именно такой код, какой написали бы вы, не надо и пытаться (если вам это так важно, то лучше сразу напишите код сами). Эрик Дитрих, «Чего нужно избегать при ревью кода»
Не нарушайте правило «Никаких сюрпризов»
Избегайте неожиданностей. Создайте для команды общее задокументированное соглашение о pull request’ах и стандартах кода и во время ревью опирайтесь на него, а не на своё мнение.
Не говорите только для того, чтобы услышать свой голос
Прежде чем написать отзыв, подумайте, чего вы хотите этим достичь. Все ли ваши комментарии необходимы? Помните, что вы пытаетесь конструктивно помочь, а не просто высказать свою точку зрения. Прежде чем опубликовать отзыв, перечитайте свои комментарии и спросите себя, все ли они действительно полезны и важны? Лишние удалите. Лучше анализировать свои заметки после просмотра кода, постфактум. Пока вы разбираетесь с кодом, пишите сколько угодно комментариев, а потом спокойно просмотрите их и решите, что оставить.
Оставлять комментарии только для того, чтобы подчеркнуть свою правоту, не всегда полезно (или разумно). Иногда лучше держать своё мнение при себе, чтобы сохранить длительные хорошие отношения с человеком. Кэтрин Дэниелс, «Как давать и получать отзывы»
Вы не обязаны находить проблемы в ходе каждого ревью
Если код хорош и может быть влит в кодовую базу без изменений, то всё прекрасно!
Что надо делать при ревью
Ревью можно улучшить. Как именно? Ниже вы прочтёте предложения, собранные в интернете.
Хвалите хороший код
Не жалейте времени на то, чтобы похвалить сильные стороны кода, прежде чем указывать на недостатки и вносить предложения.
Человеческая природа такова, что нам нужно знать о своих успехах, а не только о неудачах. Разработка — это творческий процесс, в который люди вкладывают душу, они часто принимают многое близко к сердцу. Поэтому похвалы для них ещё более важны. Роберт Бог, «Эффективные ревью без боли»
Вы же хотите, чтобы участники команды воспринимали ревью как приятный и полезный опыт, а не как чистое критиканство. Положительные отзывы снижают напряжённость в отношениях и помогают людям лучше реагировать на критику.
Важно писать позитивные комментарии так, чтобы это не выглядело слащаво или фальшиво. Не должно создаваться впечатление, что вы просто стараетесь подсластить пилюлю.
Избегайте сомнительных комплиментов. Фразы вроде «Хорошая работа, но…» (дальше — список изменений, которые вы предлагаете сделать) выглядят наигранными.
Как сделать похвалы более честными?
Сначала — положительные моменты
Сразу задайте настроение, в первую очередь рассказав о том, что вам понравилось. Теперь это легко можно сделать благодаря новым возможностям ревью кода на GitHub, создавая многострочные комментарии и публикуя их скопом (вместо публикации по мере сохранения), а также делая краткое резюме (отображается в начале отзыва) до внесения комментариев:
Избегайте неизбежных обвинений
Спрашивайте, а не говорите. Лучше задавать вопросы, а не делать заявления.
Заявление — это обвинение. Фраза «Здесь ты не соблюдал стандарты» — это обвинение, намеренное или нет. Задайте вопрос: «Почему ты использовал здесь такой подход?» Роберт Бог, «Эффективные ревью без боли»
Если задавать вопросы, то это улучшит общий настрой, восприятие ревью изменится благодаря приглашению к диалогу и обучению, а автор кода с удовольствием объяснит причину или поищет более удачный способ решения.
В идеале в ответ на корректный вопрос автор сам найдёт баг или предложит внести в код улучшения.
Если вы видите какие-то моменты, которые могут быть ошибками, не надо сразу говорить людям, что они неправы. Обычно достаточно спросить: «Что будет, если я передам этому методу ноль?», и человек наверняка сам скажет: «У меня были сомнения, исправлю». Позволить ему самому решить проблему и предложить улучшить код — гораздо лучше, чем давать указание по исправлению несовершенств. Эрик Дитрих, «Используйте ревью кода для казни»
Будьте скромнее
Задавайте вопросы, а не требуйте. Вместо того чтобы сказать автору, какие нужны изменения, задайте ему вопрос о коде или внесите предложение и спросите автора, что он думает по этому поводу. Так вы передаёте мяч на его сторону поля и выказываете уважение его свободе воли, давая автору шанс объяснить своё решение или высказаться, считает ли ваше предложение полезным для кода.
Вместо «Давай назовём эту переменную username, потому что иначе непонятно» лучше перефразировать предложение в виде вопроса: «Может быть, назвать её username? Текущее имя выглядит не слишком понятно для меня, потому что оно также используется в другом контексте в someotherfile.js.». Дэниел Бадер, «7 способов избежать ухудшения отношений при ревью кода»
Когда вы что-то предлагаете, обязательно объясняйте причину, по которой это изменение может улучшить код.
Используйте личные примеры. Покажите автору кода, что не только он совершал такую ошибку. Это отличный способ смягчить критику: «Мне самому это тяжело далось…», «Я делал то же самое».
Не на каждый вопрос нужно отвечать. Примите для себя сразу, что не на каждый ваш вопрос автор кода должен дать ответ. Так вы сможете задавать в своём отзыве вопросы, заставляющие задуматься. На них необязательно отвечать, чтобы влить код в кодовую базу, но зато они подстёгивают мысль разработчика и в долгосрочной перспективе повышают качество его работы.
Не нарушайте правило «Никаких сюрпризов»
Наверняка каждый участник вашей команды раз за разом совершает одни и те же десять ошибок. Причём труднее всего обнаружить пропуски каких-то элементов. Так что чек-лист — самый простой способ избежать наиболее распространённых ошибок и снизить вероятность пропусков. SmartBear, «Лучшие методики ревью кода»
Конечно, чек-лист не может покрыть все возможные случаи. Но в хороший список достаточно включить все требования, которым должен удовлетворять pull request, чтобы быть слитым с кодовой базой. Это полезный инструмент и для кодера, и для рецензента. Чек-лист может помочь улучшить код, сделать его более последовательным, избежать нарушения правил и стандартов. Новым участникам будет очень полезно законспектировать требования к коду, прежде чем отправлять свой первый pull request и проводить первую ревизию.
Пример чек-листа, с которого можно начать:
Стандарты программирования — это общее соглашение, заключаемое между разработчиками. Набор обязательных для всех рекомендаций по написанию качественного и удобного в сопровождении кода. Стандарты — фундамент ревью, потому что во время ревью мы проверяем код на соответствие стандартам. Вместо того чтобы предъявлять к коду требования, не входящие в стандарты, сделайте запрос на их включение в список рекомендаций.
Без доступного всей команде эталонного проекта стандартов разработчики могут даже не понимать, где обнаружатся проблемы при следующем ревью. А это не добавляет эффективности их работе.
Стандарты должны быть исчерпывающими. Можно опираться на стиль форматирования кода PEP 8, но это недостаточно полный стандарт для использования во время ревью.
Анализируйте то, что нужно, а остальным займутся инструменты
Во время ревью практически никогда не должны возникать проблемы с форматированием кода. Ревизии должны провоцировать появление продуктивных мыслей и дискуссий, а траты времени на стиль и форматирование в этом не помогут. Для подобных вещей используйте инструменты, например линтеры и средства автоматического форматирования.
Заключение
Это статья о том, как давать положительные заключения и правильно критиковать, как успешно вносить предложения во время ревью кода. Я предлагаю вам сделать сжатую версию этого руководства с выделенными ключевыми моментами и раздать её каждому участнику команды. Положите инструкцию в общий репозиторий, чтобы любой мог начать её разбор и предложить свои изменения, и тогда вся команда будет вовлечена в обсуждение.
Также рекомендую подумать о том, как у вас создаётся код, который потом попадает на ревью. Вероятно, вы хотите, чтобы кодер и рецензент имели похожие представления о техническом дизайне до того, как код окажется на ревью. Заранее решите, кто будет писать код для конкретной фичи, а кто будет рецензировать, и поощряйте их работать вместе, чтобы эти двое обсуждали структуру кода и его промежуточные варианты, прежде чем проверять финальную версию. Нам же нужна качественная архитектура (две головы лучше), но также нужно избегать излишних дебатов во время анализа, когда оба разработчика предлагают совершенно разные решения (одному из них придётся полностью переписать уже готовый код).
Если ещё до ревью между автором и рецензентом установится крепкое сотрудничество, то во время ревью, скорее всего, всплывёт лишь несколько мелких проблем.