Code review: принципы проведения, основные правила и рекомендации

Привет! Меня зовут Станислав Фелинский, и я Software Architect (Solution) в Gaming Unit’e компании Innovecs. В этой статье я поделюсь нашим опытом code review в контексте GameDev, его принципами и рабочими методами. Материал будет интересен не только авторам и рецензентам кода, но и всем инженерам команды.

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

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

Иллюстрация Алины Самолюк

Зачем нужна проверка кода

Для начала хотелось бы поделиться нашими критериями к код-базе проекта:

  • Гибкость (или же стойкость к изменениям).
  • Оптимизация (или же скорость работы).

У проверки кода несколько ключевых задач:

  • Предоставление обратной связи (feedback) автору для помощи в решение его задачи, при этом чтобы код-база проекта соответствовала критериям, описанным выше.
  • Обмен знаниями как общими, так и конкретно по проекту, между автором и рецензентом.

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

Как проходит проверка и на чем базируется

В нашей команде процесс проверки кода состоит из таких этапов:

  1. Pull request.
  2. Review с комментариями:
    1. Merge, если замечаний нет;
    2. ToDo и правки, затем следует возврат к первому пункту.

Субъективно я бы сформулировал его следующим образом: Code review — как незаменимая часть рабочего процесса.

Для качественного ревью мы в команде используем такие подходы:

  • Атомарные задачи. Каждая задача должна быть сформулирована таким образом, чтобы она затрагивала небольшую часть проекта. Мы используем формат User Story. В таком случае и автор, и рецензент фокусируются на небольшом участке кода, а следовательно, ревью проходит быстрее и качественнее.
  • Gitflow. Одна задача решается в пределах одной ветки для одного проекта. Так мы можем гибко контролировать изменения код-базы.
  • Ожидаемый объем работы — scope of work в пределах определенного отрезка времени. Для инженера важно понимать, какие глобальные задачи ему необходимо решить. Исходя из этого он сможет понять суть небольших атомарных задач и написать код для этого, а также прогнозировать время их выполнения с учетом ревью. С этой целью наша команда использует Sprints/Milestones.

Что касается инструментов проверки, то мы пользуемся Bitbucket и GitHub — выбор зависит от нюансов проекта, его особенностей. Эти сервисы имеют отличный, постоянно улучшающийся инструментарий для ревью кода. Кроме этого, они хорошо интегрируются с другими сервисами, которые являются частью нашего рабочего процесса.

Преимущества проверки кода

1. Последовательность в дизайне и реализации. Каждый инженер обладает своим уникальным стилем программирования и архитектурным мышлением. Применение разных стилей и архитектурных подходов существенно повышает энтропию проекта, что в итоге мешает совместной работе и замедляет прогресс в целом. Code review помогает с выбором тактик и методов в процессе работы над задачей. Такой подход стандартизирует исходный код, что позволяет всем инженерам (даже новым) легко его изучать и понимать. Также процесс полезен в долгосрочной перспективе, поскольку с ростом проекта масштабируется и команда. Поддержание согласованного стиля и архитектурного подхода поможет будущим инженерам инвестировать больше времени в работу над новым функционалом, а не в анализ существующего кода.

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

3. Мониторинг качества и требований проекта. Code review ассистирует инженерам команды, определяя области, в которых можно улучшить и оптимизировать работу приложения. Иногда автор может не знать о нюансах проекта или тонкостях оптимизации. Процесс проверки кода помогает им получить фидбэк от коллег и, следовательно, отточить свои навыки. Кроме того, в процессе code review можно выявить критические ошибки, которые в конечном итоге могут привести к серьезным багам. Можно сделать вывод о том, что ревью кода существенно повышает ценность цикла разработки для создания качественных продуктов.

Если говорить о недостатках ревью, то, на мой взгляд, их нет. Вероятно, можно сказать, что проверка тормозит рабочий процесс. Но я бы ответил на это следующим образом: это инвестиция времени, которая в будущем полностью окупится, ускорит процесс разработки и улучшения продукта. Задачи без code review формально можно закрыть быстрее, но в конечном итоге получить код-базу проекта, которую невозможно поддерживать. Иногда из-за этого проект может так и не стать продуктом.

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

Культура обратной связи: лайфхаки

В 99% случаев code review в нашей команде проходит в письменной форме. Поэтому крайне важно правильно формулировать свои мысли в комментариях — читающий человек может неправильно воспринять ваши тон, настроение и посыл. Иногда это приводит к напряженным отношениям и конфликтным ситуациям, что плохо сказывается на результатах.

В нашей команде используют следующие подходы:

  • В команде нет «ты» и «я», команда — это «мы». Только командная работа может привести к хорошему результату. Не стоит переходить на личности, это относится не только к code review, но и к отношениям в команде в общем. Например: «Нам стоит разделить этот интерфейс...» вместо «Тебе нужно это исправить...»
  • Все комментарии должны быть обоснованы. Любое замечание нужно подкрепить ссылкой на литературу (статью, документация и так далее) или примером из личного опыта. Например: «...этот класс противоречит принципу L из SOLID...» или «...на предыдущем нашем проекте это привело к падению FPS до 5...»
  • Не указывать только на проблему — предложить решение. Нельзя забывать, что автор приложил огромные усилия при решении задачи, а цель рецензента — помочь. Потому комментарий «Это нужно переделать» не имеет смысла. Очень важно предложить свой вариант решения. Например: «Этот класс противоречит принципу L из SOLID, потому нам стоит сделать еще один уровень наследования».
  • Хороший способ описать решение — предоставить свой пример кода. Любому инженеру пример кода объяснит ход ваших мыслей лучше тысячи слов. Иногда бывает так, что решение не приходит сразу, но опыт и интуиция подсказывают, что участок кода требует внимания. В этом случаем мы обсуждаем проблему вместе с коллегами и ищем решение или же заводим задачу для этого на будущее.

И еще пару советов напоследок

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

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

👍НравитсяПонравилось10
В избранноеВ избранном3
LinkedIn
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Автор и рецензент обязаны подходить к проверке кода ответственно.

А ось цікаво, як ви досягаєте відповідальності при code review? Адже рецензент може формально поставитися до code review?

Если говорить о недостатках ревью, то, на мой взгляд, их нет.

У code review є маса мінусів (приведу декілька):
1) Він гальмує потрапляння змін на staging/production. Уявіть собі, що у вас весь код перевіряє менеджер замовника, який дуже зайнятий і може витратити до тижня на code review. Потім виправлення, знову code review і т.д.
2) Для того, щоб зробити якісний code review, потрібно мати достатні знання в програмуванні та проектi. Якщо у вас на проекті middle і кілька junior, то на це розраховувати не доводиться
3) Рецензент не несе відповідальності за code review. Якщо він пропустить (серйозний) баг, то винен все одно буде автор коду

А ось цікаво, як ви досягаєте відповідальності при code review? Адже рецензент може формально поставитися до code review?

Питання тільки до автора?
У нас — якщо систематично резензент не помічає проблем — це почитає бути помітним зі сторони, і веде до орґвисновків.

1) Він гальмує потрапляння змін на staging/production. Уявіть собі, що у вас весь код перевіряє менеджер замовника, який дуже зайнятий і може витратити до тижня на code review. Потім виправлення, знову code review і т.д.

Тоді у проблемах з затримкою винен цей менеджер.
Але звичайно перевіряють найближчі колеги, і повинна бути політика уваги до замовлень на ревʼю (наприклад: до 1/4 робочого часу, якщо є що ревʼюїти).

2) Для того, щоб зробити якісний code review, потрібно мати достатні знання в програмуванні та проектi. Якщо у вас на проекті middle і кілька junior, то на це розраховувати не доводиться

Джуніори можуть перевіряти інші аспекти — від формального стилю коду і до грамотности у англійській в коментарях і описах.

А для ревʼю за суттю можна і з сусьднього відділу когось запитать.
Ну а якщо у вас bus factor == 1, то це само собою незадовільно.

Рецензент не несе відповідальності за code review. Якщо він пропустить (серйозний) баг, то винен все одно буде автор коду

Де не несе, а де і несе. У нас якщо хтось поставив +2 — він отримує свою долю відповідальности (наприклад, в половину від автора), а якщо ніхто не дав +2, то це теж привид для висновків (і ревʼю висить і чекає, а начальник грозно дивиться на всіх).

До висновків: всі ваші уявлення про ревʼю кода засновані на якихось локальних, обмежених і хибних практиках, і не мають місця у загальному випадку.

В нашей команде процесс проверки кода состоит из таких этапов:

Pull request.
Review с комментариями:
Merge, если замечаний нет;
ToDo и правки, затем следует возврат к первому пункту.

А якщо наприклад, автор комміта, не згоден із зауваженнями? Або згоден, але вважає їх несуттєвими, що не впливають на якість коду? Як тоді бути?

А якщо наприклад, автор комміта, не згоден із зауваженнями?

Я не розумію головної мети таких питань.

Зрозуміло, що завжди є потенційна можливість конфліктів, і їх якось вирішують. В кожному відділі, в кожному департаменті якісь свої правила, як таке вирішувати.

Але практично завжди краще всі потенційні проблеми виявити на етапі ревʼю, аніж коли код піде у продуктин. Чи ви з цим не згодні?

Чи вам цікави саме деталі, як це вирішується? А чи будуть вони портабельні?

Моє питання було до автора поста, а не до вас.
Я в теорії прекрасно розумію, як і навіщо робиться code review. Але написати статтю, де одна теорія — повна нісенітниця, тому що вже є 100500+ статей краще.
Цікаво про те, як автор вирішує ті чи інші проблеми, пов’язані з code review, у себе на проектах.
Тепер ви краще розумієте сенс моїх запитань?

Моє питання було до автора поста, а не до вас.
Я в теорії прекрасно розумію, як і навіщо робиться code review.

Це публічний форум і будь-які такі запитання автоматично ставляться і до інших.
Справа в тому, що я дійсно не бачу, як навіть подробний розпис практики конкретно автора допоміг би вам, при суттєво іншій специфіці.
Так, шанси на це є, але мінімальні.
І можна було побачити, що автор не налаштований спускатись до таких деталей. Тому у таких статей цікавіше обговорення, а не початкова стаття.

Тому і моє питання — як ви визначатимете, які практики застосовні?

Але написати статтю, де одна теорія — повна нісенітниця, тому що вже є 100500+ статей краще.

Тут я не розумію, що таке «одна теорія».

Мене теж стаття спочатку здивувала. Але якщо таке пишуть, то, мабуть, дивлячись на тих, у кого такої практики немає?

Я теж таке (не на ту ж тематику) раніше критикував, але зараз, дивлячись на те, які місцями слабкі підходи, думаю, що і така «одна теорія» про ревʼю може бути корисною для багато кого...

Тепер ви краще розумієте сенс моїх запитань?

І так, і ні. Так — тому що на них могла бути відповідь.
Ні — тому що її не було :)

и какие практики помогают нам извлечь из него максимум пользы

На жаль, ні практик, ні прикладів з реального проекту я в статті не побачив

Для начала хотелось бы поделиться нашими критериями к код-базе проекта:

Гибкость (или же стойкость к изменениям).
Оптимизация (или же скорость работы).

Оптимізація — це процес, як це може бути критерієм для коду? І чия швидкість роботи? Девелопера або додатка? І чому у вас немає такого важливого критерію, як читабельність коду? Якщо код не читабельний, як можна робити його code review?

В нашей команде мы договорились о времени суток для ревью — утро каждого рабочего дня.

Т.е. если ревью не прошёл — ждёшь следующего дня?

Кто не читал, вот нетленка от Michael Lynch про code review с обеих сторон. ИМХО, полностью раскрывает тему. Мы даже используем эти статьи как часть онбординга новых членов команды.
mtlynch.io/human-code-reviews-1
mtlynch.io/human-code-reviews-2
mtlynch.io/code-review-love

В тему — я делала большущий пребольшущий чеклист на что нужно смотреть при code review (мы проводили в компании техток на эту тему):
github.com/...​/code-review-checklist.md

Возможно кому-то будет полезно. Часть из этих проверок можно заменить автоматическими средствами проверки кода (линтеры и другие code quality tools) а не смотреть глазами.

И там еще внизу полезные ссылки на статьи насчет качественных код ревью
github.com/...​klist.md#useful-resources

о чем эта статья?
просто рассказ о каком-то абстрактном ревью кода? все (ну или почти все) кому нужно ревьювать код и так это знают
лучше бы описали на что обращаете внимание при ревью, как происходят ревью функционала затрагивающего разные куски системы (ну не бывает такого чтобы не приходилось рефакторить или внедрять более-менее глобальные интеграции). Вся эта фигня про маленькие атомарные таски это больше про проекты уровня мобильных приложений. Если у вас огромный E-commerce или B2B решение, то так или иначе будете выходить за эти рамки атомарных задачек.
Как поступаете в случае если нужно идти на компромиссы (классический пример — есть задача А, для нее нет хорошего решения и рецензент написал плохое, но работающее решение, в простонародье костыль. Можно потратить условно x человеко-часов на поиск хорошего решения (что возможно нужно отдельно согласовать с заказчиком и он может не аппрувнуть доп расходы на разработку), или пустить в прод текущее решение. Как вы поступите? Будете до последнего не пускать в продакшин код? или встанете на сторону зла?)
Интересуют примеры хорошего тона при ревью или разные граничные кейсы.
это про заглавие статьи, если что, вот про это вот

В этой статье я поделюсь нашим опытом code review в контексте GameDev

Где тут опыт, как это вообще относиться к GameDev?

По самой статье я очень сомневаюсь что есть люди, которые:
— вместо описания проблемы в комменте к ревью пишут просто «все х-ня, переделать»
— переходят на личности в комментах к PR
— просто пишут что это плохо без описания причины или примера failure, пускай сам угадывает
— (рецензенты) отправляют на ревью не глядя, даже не пробегая по коду глазами
Это как выдать статью что не нужно пить воду из лужи. Вроде и правильно, но адекватные люди сами понимают что так делать не нужно.

В нашей команде мы договорились о времени суток для ревью — утро каждого рабочего дня

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

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

это воообще как? вы каждый PR пулите, компилируете и запускаете на локальной машине, а затем тестируете/дебажите всю логику? Вот сколько не ревьюваем в команде, самые опасные баги — незаметные, которые чаще всего без автотестов или регрессий не заметишь. И каким образом такие баги увидеть на Github? Можно на каждый PR тратить огромное кол-во времени и всю логику и кейсы воспроизводить в голове, либо действительно запускать. Но это вообще не юз-кейс для ревью)

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

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

Возможно, автор просто выполняет очередную цель в performance appraisal плане, чтобы на performance review иметь +1 аргумент для получения +200 к зп)

И в этой ситуации важно само наличие статьи, а не ее качество.

Подписаться на комментарии