Зачем и как проводить 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, проведение которого было согласовано командой, были учтены пожелания или рекомендации сотрудников и оговорены определенные правила его проведения, о которых я расскажу дальше. Сперва разберем плюсы и минусы ревью.

Плюсы

  1. Одна голова хорошо, но чем больше голов, тем лучше. Даже самый опытный разработчик совершает ошибки. Это человеческий фактор. Конец рабочего дня, усталость, частая коммуникация с заказчиком/командой, которая отвлекает, и может теряться логическая нить написания кода и так далее. В таких ситуациях вам всегда придут на помощь ваши напарники и помогут исправить уязвимые участки кода. Также это нормально, что напарники в некоторых аспектах могут разбираться лучше вас и наоборот. И из этого выплывает второй плюс.
  2. Регулярный обмен опытом и повышение технического скила. Вы обязательно будете впитывать опыт ваших тиммейтов и делиться своим. Это ли не прекрасно?
  3. Повышение софт скилов. В процессе CR вам регулярно придется коммуницировать с напарниками. Учиться критиковать их работу, при этом не задевая чувства, либо учиться принимать критику в свой адрес. Помимо этого, умение находить компромиссы и доносить доступно свое видение — тоже дорогого стоит!
  4. Доверяй, но проверяй! Я думаю, большинство из нас ловили себя на мысли «Блин, костыль какой-то написал, но вроде все работает... Если что, потом отрефакторю, а пока пусть будет, а то работы еще много». И в итоге на таких костылях и держится проект :) Зная, что код будут смотреть — разработчик 100 раз подумает, как бы написать так, чтобы не завалили правками напарники и чтобы не опозориться в их глазах. Это будет мотивировать его писать лучше свой код и стараться его анализировать на понятность, читаемость и масштабируемость глазами других людей.
  5. Помогает разработчикам лучше ориентироваться в коде проекта и держать все в одном стиле написания.

Минусы

  1. Может вызывать конфликты между разработчиками в моменты, когда не получается прийти к консенсусу.
  2. Может занимать довольно много времени на обсуждение реализации и ее исправление, вследствие чего будет страдать скорость разработки продукта.
  3. Несерьезное отношение к важности данного процесса сводит на 0 все плюсы и попросту отбирает время.

Как вы видите, плюсов больше, чем минусов, и я не спроста в первых пунктах минусов написал «может». Этим я имел в виду то, что эти недостатки можно свести к минимуму, а то и вовсе их ликвидировать, если наладить правильно процесс проведения 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 — это как раз тот процесс, в котором эти качества тесно переплетаются.

👍НравитсяПонравилось10
В избранноеВ избранном6
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
Техническая документация. Тщательно изучить документацию проекта, чтобы быть в контексте реализуемого функционала в будущем.

А якщо на проекті немає технічної документації. Що тоді робити?

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

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

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

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

Если будете задерживать ревью или писать по одному комментарию после очередной правки, то получите негатив в свой адрес от автора PR.

А якщо у вас багато термінової роботи і у вас немає часу робити CR кода на 1000 рядків?

А якщо на проекті немає технічної документації.

То это низкопробный проект, на котором не стоит работать. Нет документации — будете мальчиком на побегушках за всеми бегать и каждый сих узнавать.
И будете всегда всё делать не правильно. Потому что правильный способ — только в документации. Которой нет. А остальные способы — мнения разных людей, за которые они никак не отвечают.

А чому девелопер повинен обурюватися?

Потому что его заставляют по надуманным причинам вносить много сложных изменений в уже написанный рабочий код без внятного результата. Потому что какой-то «художник» «так видит».

А якщо у вас багато термінової роботи і у вас немає часу робити CR кода на 1000 рядків?

Ставишь аппрув и делаешь дальше свою работу.

То это низкопробный проект, на котором не стоит работать

Легким движением руки Вы только что отправили любой более-менее крупный Legacy проект в ранг «низкопробных». Причем сюда, я думаю, Вы смело отнесете и те проекты, по которым документация неактуальна.

Потому что правильный способ — только в документации

Нет, это в корне неверно. Правильный на 100% способ — в кодовой базе. Которая есть всегда. И которую нужно уметь читать/знать/помнить.

Потому что какой-то «художник» «так видит».

А разве это не является «документацией» для разработчика? Стандартов кодирования нет? Каждый пишит код, как видит? Странно, выше Вы топите за документацию ПО, и что эта документация — истина в последней инстанции, но при этом отказываете в таких же привилегиях «внутренней документации для разработчиков»? Вы как минимум непоследовательны...

Ставишь аппрув и делаешь дальше свою работу.

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

Вы только что отправили любой более-менее крупный Legacy проект в ранг «низкопробных».

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

Правильный на 100% способ — в кодовой базе.

Да? А если там баг на баге?

Стандартов кодирования нет? Каждый пишит код, как видит? Странно, выше Вы топите за документацию ПО, и что эта документация — истина в последней инстанции

Как должно работать приложение — решает бизнес. Как именно это будет достигнуто — решает технический специалист. Если у него будут сомнения, он может сам проконсультироваться у коллег. Но когда коллеги без спроса лезут в его работу — это маразм.

То есть, что такое «ответственность за ревью кода» Вам неизвестно

Ответственность за код — на том, кто его написал. Нефиг размазывать её по всей команде.

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

Так обычно и работают — что описано в таске, то и реализуется.

Потом отдаёшь на ревью более опытному коллеге — и получаeшь от него больше инфы по коду, если что-то реализовано не так.

Потом отдаёшь на ревью более опытному коллеге — и получаeшь от него больше инфы по коду, если что-то реализовано не так.

Это в теории. На практике чаще всего на код ревью будет дрочь в стиле «давай эти Ивенты переименуем в Экшены». А они используются ещё в 155 других классов. А потом в каждом из этих классов он будет искать, что можно улучшить, раз уж мы сюда залезли.
А потом появляется второй эксперт, который начинает вещать, что этот класс вообще не нужен и можно объединить его с другим, создав цепочку наследования. А третий эксперт скажет, что нужно переименовать наследника, а то его название созвучно с классом из другого пакета.
В это время наступает восьмой день спринта и продакт вежливо так напоминает, что у нас остаётся 2 дня на тестирование и интеграцию и поинтересуется, почему я так долго делаю таску на 2 сторипоинта. При альтернативно одарённых господ строчащих комменты в код ревью он ничего слушать не будет.

То это низкопробный проект, на котором не стоит работать. Нет документации — будете мальчиком на побегушках за всеми бегать и каждый сих узнавать.
И будете всегда всё делать не правильно. Потому что правильный способ — только в документации. Которой нет. А остальные способы — мнения разных людей, за которые они никак не отвечают.

Зачем документация, если есть исходный текст?

Документация на код, нужна тем же людям, которым нужны комменты в коде — джунам, неумеющим читать (и писать) код и поэтому бесполезным на проекте.

П.С. Зачем за кем-то бегать, когда у тебя есть код и отладчик?

Зачем документация, если есть исходный текст?

Документация нужна только высокоуровневая. Data flow, слои архитектуры, основные сущности, фабрики. Эти связи гораздо легче увидеть на 10 схемах, чем расковыривать очередные 200 тысяч строчек кода.

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

Документация нужна только высокоуровневая

Документация нужна в виде требований как_оно_должно_работать.
А не какая строчка кода что должна делать, разумеется.
Потому что если нигде не записано, какие требования предъявляются к ПО, никто никогда не сможет доказать, что они не выполняются. И программист становится не нужен — всё и так работает как надо.

Документация нужна в виде требований как_оно_должно_работать.

Требования это одно, а отображение архитектуры в простом и наглядном виде — другое. Это ликвидирует незаменимость сотрудников и упрощает learning curve для новобранцев.

Зачем документация, если есть исходный текст?

Очень просто.
Заводит QA баг.
Говорит:"При нажатии на кнопку мы попадаем на экран А, а должны попадать на экран Б«.
На что я резонно отмечу, что никому ничего не должен. И потребую основание для фикса.
Потому что если его не будет, в тот же день получу тикет с «мы попадаем на экран Б, а должны попадать на экран А».
Код не может являться источником требований. Им должна быть джира.
За эти требования собственники бизнеса должны нести ответственность.
Потому как я могу 3 недели пилить таску, которую мне сказал делать один ко-оунер. А потом он улетает на встречу с клиентом и другой спрашивает, что я 3 недели делал. Что я ему отвечу? Первый ко-оунер всегда сможет сказать, что ничего мне не говорил, или что я не так его понял, это нужно делать не сейчас, или не нужно вообще.
Если это не зафиксировано на бумаге, этого не было. И это очень лихой способ не заплатить мне за 3 недели работы, т.к. по мнению ко оунеров, я это время в чатах сидел и никакой пользы их бизнесу не принёс.

Второй кейс.
Есть в Андроид такая замечательная библиотека RxJava. Сейчас её уже сложно встретить в проектах и все на неё плюются. Но лет 5 назад она считалась очень крутой и молодёжной.
Почему так получилось. Там Овер 100500 мощных операторов, каждый из которых может делать с данными натуральную чехарду. Все их не знает ни один живой программист на планете. Многие из них отличаются друг от друга какими-то мелкими нюансами. И когда у тебя в коде длинная борода из цепочки 20-30 таких операторов, большинство из которых на память не помнит никто в команде а доки по ним можно курить хоть целый спринт напролёт, ни одна живая душа никогда не поймёт, что эта борода должна была делать изначально после того, как оно всё накрылось медным тазом и кидает эксепшены после изменения структуры ответа сервера. Дебажить эту хрень можно целый спринт всей командой без внятного результата.
А имея документацию, один девелопер за спринт-два перепишет этот же функционал на корутинах без всего этого нечитабельного ужоса.

Код не может являться источником требований. Им должна быть джира.

Ок, ты имеешь в виду не описание кода в виде документации, а спецификацию?

Ок, спецификации нужны, ясное дело. С другой стороны, если требуется фича и она не описана в спецификации — тогда фичу в спецификацию придётся вписать самому.

Либо, накрайняк, описание в таске будет «спeцификацией» — на которую ссылаться, при последующих разбирательствах.

Микита, ви так топите за Code Review, що я ризикну зробити CR вашого поста. Ви ж не проти?

Может занимать довольно много времени на обсуждение реализации и ее исправление, вследствие чего будет страдать скорость разработки продукта.

Це якщо чесно дивний мінус. Адже реалізація повинна обговорюватися ПЕРЕД початком роботи, а не під час CR. Я маю на увазі не те, як написати ту чи іншу функцію, а загальний алгоритм дії.

Несерьезное отношение к важности данного процесса сводит на 0 все плюсы и попросту отбирает время.

Цікаво було б прочитати в вашому пості, як ви мотивуєте людей в своїй команді робити якісний CR.

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

А навіщо це робити на CR? Є плагіни, які автоматично форматують код перед commit.

PR: Зачем и как проводить Code Review
Status: Rejected with comments:

Более того, нет такого CR, который смог бы ускорить вашу разработку. В любом случае вам придется потратить время на создание Pull Request (PR), исправление багов после ревью, проверку на работоспособность функционала после исправления и так далее.

да ну? видел огромное кол-во примеров, когда таску нужно было откатывать из релиза и переделывать как раз из-за того что человек не отправил ее на ревью, а подкинул в релиз и релиз гай то пропустил. После этого тратиться много времени и сил на исправление проблем (например, созданные 10к неправильных заказов, или 2кк дубликатов продуктов), а также фикс логики в самой задаче.
второй момент — PR можно создавать не только когда закончишь задачу, но и *сюрприз* в процессе, итерируясь небольшими подзадачами, получая фидбек и советы по реализации в процессе. Не знаю где вы там что ревьюваете, но что-то пошло не так.

Когда поймете что есть что-то за пределами кода и на самом деле задача состоит в решении бизнес проблемы (которую нужно решить в том числе качественно, без багов и огромных архитектурных дыр), а не просто в быстром написании какого-то кода, который вроде работает и покрывает 2 тест кейса в Jira, то откроете для себя интересный мир.

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

Эмм. Допустим я человек отвечающий за ревью. У нас есть общие принципы проведения ревью, проверенные годами и кровью/потом поколений разработчиков до нас. На этом этапе оно еще правильное.
Ко мне в команду приходят 2 джуна, которые не согласны с какими-то принципами ревью (например, «зачем, я и так все знаю, вы мне не доверяете? тратить время, мог бы поиграть в теннис, бла-бла»). Внезапно, CR в нашей компании становится не правильным, ибо 2 джуна с этим не согласны?)

Может вызывать конфликты между разработчиками в моменты, когда не получается прийти к консенсусу.

за 7 лет разработки, из них 5 лет с код ревью, последние 3 года помогаю с ревью и ревьюваю сам. Еще не видел ситуаций когда не приходили к консенсусу и были холивары по реализации.
Хотяяя, 1 раз когда 2 архитектора друг на друга по приколу батхертили в комментах, но думаю это не нужно считать. Может проблема не в CR, а в команде?

Может занимать довольно много времени на обсуждение реализации и ее исправление, вследствие чего будет страдать скорость разработки продукта.

Нет, не может. Почему? Потому что первичное и основное обсуждение реализации должно быть на этапе планирования. Если ваша финальная реализация настолько плоха, возможно проблема также не в CR, а в подходе к разработке и стадии планирования?

И опять же, сделанный код != разработка (развитие) продукта. Опять же, нужно смотреть на уровень выше. А решает ли этот код проблемы бизнеса? Или после релиза вылезет 200 сайдеффектов и багов, которые принесут бизнесу много потерь в деньгах и репутации.

чем больше изменений за раз, тем сложнее откатывать код при такой необходимости.

VCS в помощь, делается 1 кнопкой во всех популярных IDE, даже не нужно в терминал заходить.

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

вспоминаем про планирование и обсуждение задачи с более опытными коллегами до реализации

Не взваливайте на себя слишком много ревью.
Не задерживайте ревью.

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

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

опять же, раньше было отличать хотелки от необходимостей. Если вы готовы пропустить кусок кода в продакшин, возможно, это не необходимость. Простейшее решение — топаем к PM, говорим что есть проблема и нужно Х часов на доп разработку, выносим в отдельную задачу? Все, проблема решена, дальше идем пить смузи и кататься на моноколесах по офису

Что-то слишком много расплодилось пустых статей про CR. Формулу как проводить код ревью мы не сформировали.
Вместо всей этой полотнины с проблемами, которые вообще никак не связаны с CR, можно было описать все в 2х предложениях:
Не вдавайтесь в крайности, будьте вежливы, советуйтесь с опытными коллегами во время реализации. Выработайте практику код ревью которая будет работать для вашей команды и следуйте ей.
Остальное люди и так должны понимать, взрослые же, не?

видел огромное кол-во примеров, когда таску нужно было откатывать из релиза и переделывать как раз из-за того что человек не отправил ее на ревью, а подкинул в релиз и релиз гай то пропустил. После этого тратиться много времени и сил на исправление проблем (например, созданные 10к неправильных заказов, или 2кк дубликатов продуктов), а также фикс логики в самой задаче.

И ваши QA это пропустили???
QA lead после этого не был уволен???
При чём тут вообще код ревью?

Внезапно, CR в нашей компании становится не правильным, ибо 2 джуна с этим не согласны?)

Кто со мной не согласен — тот джун?

А решает ли этот код проблемы бизнеса?

Причём тут код ревью?

И ваши QA это пропустили???
QA lead после этого не был уволен???
При чём тут вообще код ревью?

Учимся мыслить логически. Мой коммент был на то, где человек говорит что ни один CR не ускорит разработку. Перефразирую мои слова — если бы таска прошла код ревью, а не была докинута где-то в конце релиза тайком, то этого бы *скорее всего* не было. И не нужно было тратить огромное кол-во времени на откат, фикс багов и переделку.
А при чем тут вообще QA? У вас есть 20 фич в спринте, вы их сделали, QA проверили эти 20 фич. Все чотко. Тут перед самым релизом человек в тот же PR что был проревьюван докидает какой-то рефактор. Бабах, релиз пошел по жопе. Или у вас QA еще перед релизом регрессию всего затронутого функционала делают? Очч интересно)

Кто со мной не согласен — тот джун?

Опять же, невнимательно/бездумно читаете. Я в примере привел ситуацию с джунами. Окей, замените на любое другое слово. 2 миддла пусть будет. Сильно поменяло смысл? При чем это вообще?) Имелось ввиду, что если хоть кто-то в команде становится не согласен с ревью то считать его неправильным — откровенно глупо.

Причём тут код ревью?

Хмм, блин, забыл, на логику ж не нужно смотреть. На реализацию тоже. Задача ревьювера — быть линтером, проверять скобочки и точки с запятыми. Хз, у нас на проекте ревьювер старается вникнуть в логику того что написано и делает ли этот код вообще то что задумывалось.

Перефразирую мои слова — если бы таска прошла код ревью, а не была докинута где-то в конце релиза тайком, то этого бы *скорее всего* не было.

Дык тут проблема уже в ваших процессах, что в прод пошёл билд, которого не видели QA.

Хмм, блин, забыл, на логику ж не нужно смотреть. На реализацию тоже

Не всегда глядя на код можно на 100% понять, как именно он будет работать во всех мыслимых сценариях использования.
Увидеть это могут лишь только мануальные тесты.
Я видел 100500 мобильных приложений, работавших идеально, пока им не повернули 2 раза экран телефона, выключив и включив интернет в процессе.

1. «Ревью» служит для улучшения работающего кода — а не для превращения неработающегов в работающее. Соответственно, нет повода не чекинить работающие реализации, без какого-либо ревью. А уже дальше, запрашивать ревью чейнджсетов/ревижинов — и улучшать их, сколько влезет.

2. Комментарии в коде не нужны. Сам код должен быть само-документируемым. Лишь сложные/нетривиальные места (нетривиальность, на усмотрение накодившего) — достойны комментариев.
Понятно, имеются в виду комментарии в теле функций, а не регламентируемые «кодинг правилами» общие комментарии на публичные вещи, итп...

3. Ревью имеет смысл, лишь для нахождения проблем в зачекиненном коде («принцип 4-х глаз»).

4. (Следствие из 3). Ревью имеет смысл отдавать не каждому. Скажем, станете ли вы приглашать на «ревью» случайного чела с улицы — лишь из-за того, что пройденный ревью — это обязательная стадия приёмки таска? Чел из другого проекта и даже другой подсистемы этого проекта (если он не в курсе вашей подсистемы) — тот же чел с улицы.
Ревьюить должен чел, который в теме и с опытом — т.к. лишь такой сможет быстро въехать в код (зная, что происходит в этом месте системы) и сможет углядеть потенциальные проблемы нового кода.

5. Всё, обсуждённое на словах (ваш пункт про «голосовое общение») — относится к нигде не зафиксированным обещаниям (ваш пункт про «обещания»)

Комментарии в коде не нужны. Сам код должен быть само-документируемым.

Да откуда ж вы лезете...

Ок, комментарии в коде нужны. Лишь джунам, неумеющим читать (и писать) код — и поэтому бесполезным на проекте.

Якщо код (і його роботу) не можна зрозуміти без коментаря, то яка цінність такого коду?

Маю вибачитися за надто різкий комент, але ця тема болить.

Цінність у тому, що воно якось працює.
Коли ви, шановні коллеги, зіткнетеся з якимось довбаним легасі, пам’ятайте, що його створили люди (не дурні люди бо воно працює), які вважали свій код самодокументованим і самозрозумілим.
Звісно, все це набуває значення для великих і складних проектів і того самого кривавого ентерпрайзу коли код перетворюється на легасі десь роки за півтора-два і вже не знайти тих людей які його писали. Але якщо у вас інтернет-магазин з продажу корма для котів, то там навряд чи є що коментувати.

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