Code review что это такое простыми
Что такое код-ревью
Это проверка кода на ошибки, неточности и общий стиль программирования.
Послушать аудиоверсию этой статьи (6 минут):
Слушайте Что такое код-ревью на Яндекс.Музыке
Ситуация: вы разработчик. Вы отвечаете за свой фронт работы, например, за отправку данных на сервер. У команды уже есть готовый проект, вы вместе его поддерживаете и постоянно улучшаете.
Когда вы пишете новую функцию, она не попадает сразу в проект. Вместо этого ваш код отправляется на код-ревью (code review).
Что делают на код-ревью
Во время код-ревью кто-то из старших товарищей изучает ваш код на предмет:
Если проблемы есть, проверяющий отправляет код на доработку. Если всё хорошо, код переходит на следующую стадию — как правило, в тестирование.
Кто проводит
Обычно принято так: код-ревью делают программисты более старшего уровня. Джуниоров проверяют мидлы, мидлов — сеньоры, сеньоров — другие сеньоры или тимлиды. Если компания большая, то могут выделить отдельно несколько человек, которые смотрят код у всех и следят за общим стилем.
Если команда небольшая, то код-ревью делает ведущий программист — он сам следит за проектом и за качеством кода, который пишут остальные.
Как это выглядит
Зачем это нужно
Если вы пишете один или с другом, то, скорее всего, вам это не нужно. Вы и так можете обсудить код между собой и понять, как его сделать лучше.
Когда в команде программистов много, то компания сталкивается с тем, что все пишут по-разному. И даже если весь этот код работает, его потом нужно поддерживать, а ковыряться в чужом коде, если он плохо написан — это долго и дорого. Поэтому на этапе код-ревью разработчики делают так, чтобы им же позднее было проще поддерживать код и ничего не ломалось.
Еще одна статья о 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 – зачем и как использовать в команде?
Что такое 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
Поищите инструменты для вашего языка программирования. Используйте тот, который больше всего подойдет вашей команде.
Как сделать код-ревью быстрее и эффективнее
Как обычно происходит код-ревью? Вы отправляете пул-реквест, получаете обратную связь, вносите исправления, отправляете фиксы на повторный ревью, затем получаете одобрение, и происходит мерж. Звучит просто, но на деле процесс ревью бывает очень трудоемким.
Представьте, что у вас есть пул-реквест с сотнями строк изменений. Ревьюер должен потратить немало времени, чтобы полностью прочитать код и понять предлагаемые изменения. В результате весь процесс от создания пул-реквеста до его утверждения может занять несколько дней — в этом мало приятного и для ревьюера, и для автора изменений. И велики шансы, что в итоге ревьюер все равно что-то упустит. Или проверка может быть слишком поверхностной, а в худшем случае пул-реквест вообще может быть сразу отклонен.
Получается, что чем объемнее пул-реквест, тем меньше пользы будет от его проверки.
Как избежать таких ситуаций? Как сделать пул-реквест проще и понятнее для ревьюера и оптимизировать весь процесс?
Переводим статью нашего бэкенд-разработчика Сергея Жука про то, как устроен процесс код-ревью у команды мобильного приложения Skyeng.
Категории изменений
Давайте представим, что у вас есть задача — реализовать новый функционал в проекте. Пул-реквест, над которым вы работаете, может содержать разные категории изменений. Конечно в нём будет какой-то новый код. Но в ходе работы вы можете заметить, что какой-то код нужно предварительно порефакторить, чтобы он способствовал добавлению нового функционала. Или с этим новым функционалом в коде появилось дублирование, которое вы хотите устранить. Или вы вдруг обнаружили ошибку и хотите ее исправить. Как должен выглядеть окончательный пул-реквест?
Сначала давайте разберемся, какие категории изменений могут происходить с кодом.
Стратегии ревью
Очень важно понимать, что каждая из этих категорий ревьюирутеся по-разному и при их рассмотрении ревьюер должен фокусироваться на разных вещах. Можно сказать, что каждая категория изменений предполагает свою собственную стратегию ревью.
Функциональные изменения. Это самый длительный процесс, потому что он предполагает изменения доменной логики. Ревьюер смотрит, решена ли проблема и проверяет, является ли предложенное решение наиболее подходящим или его можно улучшить.
Структурный рефакторинг. Этот процесс требует гораздо меньше времени, чем функциональные изменения. Но здесь могут возникнуть предложения и разногласия по поводу того, как именно код должен быть организован.
При проверке остальных категорий в 99 % случаев мерж происходит сразу же.
Почему нужно разделять изменения по категориям?
Мы уже обсуждали, что разные категории изменений ревьюируются по-разному. Например, функциональные изменения мы проверяем, отталкиваясь от требований бизнеса, а при структурном рефакторинге — проверяем обратную совместимость. И если мы смешаем несколько категорий, то ревьюеру будет трудно держать в уме одновременно нескольких стратегий ревью. И, скорее всего, ревьюер потратит на пул-реквест больше времени, чем необходимо, и из-за этого может что-то упустить. Более того, если пул-реквест содержит изменения разного рода, при любом исправлении ревьюеру придется пересмотреть все эти категории еще раз. Например, вы смешали структурный рефакторинг и функциональные изменения. Даже если рефакторинг выполнен хорошо, но есть проблема с реализацией функционала, то после исправлений ревьюер должен будет просмотреть весь пул-реквест с самого начала. То есть проверить заново и рефакторинг, и функциональные изменения. Так проверяющий тратит на пул-реквест больше времени. Вместо того, чтобы чтобы сразу смержить отдельный пул-реквест с рефакторингом, ревьюер должен еще раз просмотреть весь код.
Что точно не стоит смешивать
Переименование/удаление класса и его рефакторинг. Здесь мы сталкиваемся с Git, который не всегда правильно понимает такие изменения. Я имею в виду масштабные изменения, когда меняется много строк. Когда вы рефакторите класс, а затем перемещаете его куда-то, Git не воспринимает это как перемещение. Вместо этого Git интерпретирует эти изменения как удаление одного класса и создание другого. Это приводит к куче вопросов во время код-ревью. И автора кода спрашивают, почему он написал этот уродливый код, хотя на самом деле этот код был просто перемещен из одного места в другое с небольшими изменениями.
Любые функциональные изменения + любой рефакторинг. Мы уже обсуждали этот случай выше. Это заставляет ревьюера держать в голове сразу две стратегии рецензирования. Даже если рефакторинг выполнен хорошо, мы не сможем смержить эти изменения, пока функциональные изменения не будут утверждены.
Любые механические изменения + любые изменения, произведенные человеком. Под «механическими изменениями» я подразумеваю любое форматирование, выполненное с помощью IDE или генерации кода. Например, мы применяем новый code style и получаем изменений на 3000 строк. И если мы смешаем эти изменения с какими-либо функциональными или любыми другими изменениями, произведенными человеком, мы заставим ревьюера мысленно классифицировать эти изменения и рассуждать: это изменение, произведенное компьютером, — его можно пропустить, а это изменение, сделанное человеком, — его нужно проверить. Так ревьюер тратит очень много дополнительного времени на проверку.
Пример
Вот пул-реквест с функцией метода, который аккуратно закрывает клиентское соединение с Memcached:
Даже если пул-реквест небольшой и содержит всего сто строк кода, его все равно сложно проверять. Как видно по коммитам, он содержит различные категории изменений:
В результате ревьюер должен просмотреть весь код и
1. Рефакторинг: извлечение класса
Здесь всего два файла. Ревьюер должен проверить только новый дизайн. Если все в порядке — мержим.
2. Следующим шаг — тоже рефакторинг, мы просто перемещаем два класса в новое пространство имен
Такой пул-реквест довольно просто проверять, он может быть смержен сразу.
3. Удаление лишних блоков док-блоков
Здесь ничего интересного. Мержим.
4. Сам функционал
И теперь пул-реквест с функциональными изменениями содержит только нужный код. Так ваш ревьюер может сосредоточиться только на этой задаче. Пул-реквест небольшой и его легко проверить.
Заключение
Не создавайте огромных пул-реквестов со смешанными категориями изменений.
Чем больше пул-реквест, тем труднее ревьюеру понять предложенные вами изменения. Скорее всего, огромный пул-реквест с сотнями строк кода будет отклонен. Вместо этого разбейте его на маленькие логические части. Если ваш рефакторинг в порядке, но функциональные изменения содержат ошибки, то рефакторинг можно спокойно смержить, и таким образом вы и ваш ревьюер сможете сосредоточиться на функционале, не просматривая весь код с самого начала.
И всегда выполняйте следующие шаги до отправки пул-реквеста:
От редакции: Сергей много пишет интересного про программирование и PHP, а мы иногда что-то переводим: сервер потокового видео, рендеринг HTML файлов. Смело задавайте ему вопросы в комментариях к этой статье — он ответит!
Ну а также напоминаем, что у нас всегда много интересных вакансий для разработчиков!
Код ревью, как внедрить и не испытывать боль
Если вы работаете в продуктовой компании, то жизненный цикл почти каждого продукта будет соответствовать принципу Парето:
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 за ревью статьи про ревью.