Зачем и как проводить Code Review

Итак, встречайте, король разногласий и недопониманий между разработчиками, катализатор вымирания нервных клеток и просто вещь, из-за которой ваш эстимейт может оказаться не таким уже и завышенным — Code Review (CR).

Меня зовут Никита Мазур, я работаю на позиции Front-end Engineer. За свой опыт работы я встречал много различных способов проведения CR, и ни один из них не являлся идеальным. Более того, нет такого CR, который смог бы ускорить вашу разработку. В любом случае вам придется потратить время на создание Pull Request (PR), исправление багов после ревью, проверку на работоспособность функционала после исправления и так далее. Также это не отменяет того, что написанный вами код может совершенно не устраивать других разработчиков, и тогда, возможно, вам вообще придется все переписать с нуля и вы сможете смело свой эстимейт умножить на два. Даже время, заложенное на прохождение CR, не может вам гарантировать выполнения задачи в поставленные сроки. Поэтому возникает вопрос — нужен ли CR, если с ним все не так однозначно, а порой даже действительно сложно? И ответ есть: однозначно нужен!

Чем больше я углублялся в изучение данного вопроса, тем больше удивлялся от того, сколько информации и статей уже есть на данную тему. И тут даже не вопрос в паре страниц. Есть целые книги, посвященные данной тематике! Проблема заключается в том, что нет единой истины, как должен проходить процесс CR, как должен создаваться PR, как должно проходить обсуждение изменений по этому же PR... Каждая статья или книга базируется на собственном опыте, набитых шишках и представлениях, как это должно происходить на идеальном проекте, с идеальными сотрудниками и при прочих идеальных условиях. Собрав достаточно информации + проанализировав свой опыт, я постараюсь донести некоторые рекомендации, которые действительно помогли бы упростить процесс прохождения CR и в целом улучшить качество вашего проекта.

Для чего нам Code Review

Итак, вернемся к тому, зачем нам нужен CR, если он такой не хороший? Ответ заключается в том, что действительно ценные сокровища никогда не лежат на поверхности. В нашем случае сокровищами являются опыт, знания и софт скилы, без которых в нашем направлении никак. И именно эти качества правильный CR* позволяет развить лучше всего как ревьюеру, так и автору созданного PR. А если еще писать комментарии на английском, это за одно повысит ваш уровень знания языка! В общем, одни плюшки :)

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

Как вы видите, плюсов больше, чем минусов, и я не спроста в первых пунктах минусов написал «может». Этим я имел в виду то, что эти недостатки можно свести к минимуму, а то и вовсе их ликвидировать, если наладить правильно процесс проведения CR. Для этого я бы хотел предложить советы, которые смогут с этим существенно помочь.

Рекомендации по проведению можно разбить на три части: подготовка к CR, создание PR для дальнейшего ревью, проведение CR.

Подготовка к проведению CR

Среда проведения CR. Прежде всего нужно определиться со средой проведения ревью. Наибольшей популярностью пользуются GitHub, GitLab, BitBucket. Все три могу рекомендовать к использованию, так как у каждого из этих сервисов есть нужный набор инструментов для проведения качественного ревью, а также история изменений и обсуждений.

Понять важность проведения CR. Объяснить всей команде важность сего мероприятия. Каждый член команды должен понимать, насколько важно внимательно проверять написанный код и давать качественный фидбэк, так как от этого будет зависеть качество будущего продукта.

Ответственные лица за ревью. Определиться с тем, кто будет ревьюить PRы. В идеале это должно быть минимум два коллеги вашего профиля, помимо вас (1 — архитектор, 1 — разработчик).

Стиль написания кода. Обсудить код-стайл на проекте, стиль нейминга переменных/функций/классов, отступы, максимальную длину строк кода и так далее. Если подобные правила установлены на уровне компании, то обязательно ознакомиться с ними на стадии подготовки к проекту!

Настроить линтер кода. Настройка данного помощника поможет избежать большинства минорных исправлений, так как на стадии разработки этот пакет будет помогать вам следовать оговоренному код-стайлу.

Паттерны разработки. Определиться с подходящими паттернами для вашего проекта и договориться их придерживаться.

График проведения CR. Согласовать частоту и выделяемое время на CR. По статистике лучшим сочетанием является ежедневные CR с выделяемым временем на них не более 30–60 минут на день.

Техническая документация. Тщательно изучить документацию проекта, чтобы быть в контексте реализуемого функционала в будущем.

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

Дополнительные помощники. По возможности связать вашу систему контроля версий с таск-менеджером.

P. S. Именно подготовка является важнейшим атрибутом успешного проведения процесса CR. Так как при должном планировании можно будет избежать большинства разногласий и количества исправлений в будущем ревью.

Создание PR для дальнейшего ревью

Четкое и лаконичное название. Если вы используете таск-менеджер, например Atlassian Jira, и при этом данный сервис связан с системой контроля версий, то будет правильно указать номер задачи в названии PR. Чтобы было понятно, с какой задачей связаны данные изменения в коде. Также в задаче можно будет ознакомиться с технической документацией к данной реализации. После номера задачи рекомендуется указывать краткое описание разработанного функционала (что конкретно было реализовано).

Комментарии в коде. Это является важным атрибутом при создании PR. Комментарии должны содержать краткое объяснение, почему была применена такая реализация. Правильное написание комментариев в коде позволит существенно уменьшить время на обсуждение реализации между сотрудниками и тем самым ускорит процесс проверки PR. Также это улучшит в целом понимание написанного кода на проекте и позволит в будущем новым разработчикам быстрее внедряться в процесс разработки.

PR должен быть как можно меньше, потому что:

• маленький PR можно быстро проверить;
• проверка будет более осмысленной;
• меньше вероятность упустить баг;
• не так обидно, если весь PR будет отклонен. Ведь очень плохо, когда сделана большая работа, а потом выясняется, что все это вообще было не нужно;
• проще вливать изменения, меньше конфликтов;
• легче добиться хорошего качества кода;
• чем больше изменений за раз, тем сложнее откатывать код при такой необходимости.

Старайтесь делать PR независимым и самодостаточным. Вся новая логика, которая используется в этом PRе, должна быть описана в этом PRe. PR не должен ничего ломать в текущем функционале и не должен зависеть от мерджа других PRов.

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

Не спешите возмущаться на очередную партию правок по вашему PR. Зачастую, когда работа сделана и отправлена на код-ревью, психологически сложно принять тот факт, что придется еще и что-то исправлять. Поэтому постарайтесь не воспринимать комментарии ревьюера в штыки, подумайте, возможно, он прав, и код станет в результате лучше.

Однако если ревьюер все же не прав, смело пишите об этом, снабдив ответ аргументами и фактами.

Учитесь на своих ошибках. Делайте выводы после каждого пройденного CR. Улучшайте свой код и не допускайте подобных ошибок в следующих PR, и вы вскоре обязательно увидите, как быстро проходят ревью PRы.

Проведение CR

Не бывает идеальных PRов. Помните о том, что ваше видение может кардинально отличаться от видения напарников. И это не есть плохо. Прежде всего вы должны понимать контекст написания данного решения. Если вы увидите картину в целом, возможно, написанный код оправдает себя в ваших глазах. В любом случае сутью каждого PRa является добавление нового функционала или улучшение текущего. Если PR этого не несёт, то нужен ли он тогда? (статья Google)

Не бойтесь делать замечания, но... Вы не ограничены в количестве правок, но научитесь отличать необходимое от желаемого. Помните о том, что правильно работающая логика лучше персональных предпочтений. Завалив человека персональными «хотелками», вы можете на существенный отрезок отодвинуть реализацию нужного проекту функционала. Лучшей практикой тут будет в обязательном порядке писать критические замечания, а персональные пожелания помечать приставкой PP (personal propose) или nit (взято со статьи Google). Например: «PP: please rename variable isName to name». Это в целом никак не повлияет на работу функционала, но если таких правок будет, например, 20–30, то человек уже может не попасть в свои эстимейты и отодвинуть работу других разработчиков на период исправлений.

Миксуйте дискуссию в комментариях с голосовым общением. Порой у вас может возникнуть много вопросов о реализуемой логике в PR. На обсуждение всех вопросов в комментариях может понадобиться много времени и будет проще скоро обсудить все голосом. Это может в разы ускорить процесс прохождения CR.

Не взваливайте на себя слишком много ревью. Помните о том, что с каждой проверенной вами строчкой ваше качество ревью будет ухудшаться. Понимаю, что ситуации бывают разные, но старайтесь ревьюить не более 300–400 строк кода за один подход. После этого обязательно сделайте перерыв. Если вам прислали PR на ревью, в котором 600–800 строк кода, возможно, его можно разбить на более мелкие. Обязательно скажите об этом автору PR.

Не задерживайте ревью. Это очень важно! Не забывайте, что от вашего ревью будет зависеть скорость реализации задач на проекте. Если будете задерживать ревью или писать по одному комментарию после очередной правки, то получите негатив в свой адрес от автора PR. Если уже взялись за ревью, то завершите его за один раз и обязательно сразу проверьте его после правок автора.

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

Не ведитесь на обещания. Если видите проблемные места и разработчик кормит вас обещаниями «Я это обязательно исправлю в следующем PR» или «Я создам себе отдельную задачу на правку», как показывает практика, 99%, что за этот функционал забудут, так как проект не стоит на месте и новые задачи никуда не пропадут. Настаивайте на всех важных правках здесь и сейчас.

Будьте вежливы. Бывает, что разработчик расстраивается, если ревьюер настаивает на каких-то изменениях. Однако чаще всего разработчики расстраиваются из-за формы замечания, а не из-за содержания. Будьте вежливы, объясняйте аргументами, и, скорее всего, всё будет ок.

Вывод

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