Что такое Code Review
Code Review - это процесс проверки и анализа кода задачи разработчиком перед ее релизом. CR (Code Review) выполняется не тем человеком, который делал задачу, а другими членами команды. Результатом CR является обратная связь по выполненной задаче: необходимость внести правки, либо готовность задачи к последующему тестированию и релизу.
Зачем нужен 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
Поищите инструменты для вашего языка программирования. Используйте тот, который больше всего подойдет вашей команде.
Примеры инструментов:
GitHub Code Review https://github.com/features/code-review/
Collaborator https://smartbear.com/product/collaborator/overview/
Beanstalk https://beanstalkapp.com/
Комментарии (31)
nanallew
04.10.2021 13:09+2Полезные заметки по процессу и путях улучшения Code Review.
Не хватает реального опыта вместо теории.
А то у нас в команде процесс ревью - это скорее то, что никто не хочет делать (скучно) и если бы автор поделился реальным опытом, о том как сделает проще и быстрее, то эта статья была бы гораздо полезнее.
maria_yudina Автор
04.10.2021 13:50+1А что именно про опыт? Часть вот этих чеклистов мы, например, использовали в команде, прям писали их даже во внутренней документации для новых сотрудников, которые раньше не делали ревью. Сейчас используем Pull Requests в GitHub для самого процесса ревью, большие задачи стараемся комитить мелкими частями. Эта статья именно больше про теорию и задумывалась. Но могу и про опыт написать отдельно, мне просто кажется, что там будет мало.
nanallew
04.10.2021 17:23Опыт автоматизации, примеры проблема - решение.
Был бы интересен такой формат :
Проблема :
В команде процесс код ревью растягивал релиз на +1 или +2 недели, часто давали заниженный эстимейт. Как результат : негативный фидбек от клиента и настроение команды.
Решение :
Взяли за правило добавлять 20% от начального эстимейта в качестве буфера для код ревью. Да, шанс промахнутся из-за код ревью остался, но разница будет существенно ниже. Как результат : время на ревью больше, результат ревью качественней, команда в положительном настроении, клиент не переживает.
maria_yudina Автор
04.10.2021 17:55+1Как раз уже думаю над примерно таким форматом статьи - пройтись по кейсам и описать, как решали. Судя по комментариям, актуально будет в виде такого формата сделать.
Спасибо!
Sensimilla
04.10.2021 13:09"на реализацию тратится чуть больше времени" - во сколько вы оцениваете это "чуть"?
maria_yudina Автор
04.10.2021 14:01Если задачи хорошо декомпозированы, то временные затраты обычно не большие (+ час-два к задаче). Еще, например, у нас сейчас в команде договоренность, что задачи в статусе ревью висят не более 24 часов (1-2 дня условно) - то есть задержка релиза тоже небольшая. Если задача маленькая, то ревью вообще практически не добавляет затрат - полчаса или меньше на ревью.
ivanych
04.10.2021 23:15+2Если задача маленькая, то ревью вообще практически не добавляет затрат - полчаса или меньше на ревью.
Вы хотели сказать:
24 часа ожидания ревью
полчаса ревью, на ревью возникают замечания
24 часа ожидания исправления замечаний
полчаса исправления замечаний
24 часа ожидания ревью
полчаса ревью
повезло, если замечания действительно исправлены и не возникло новых замечаний, иначе идём в пункт 3.
maria_yudina Автор
05.10.2021 01:221-2 дня - это максимум на ожидание ревью. Обычно в тот же день или на следующий ревью проходит. Вы описали прям суровый вариант, когда что-то сильно пошло не так (причем еще до ревью), что аж несколько раз пришлось править замечания. В реальности я даже такого не припомню, обычно одного раунда правок достаточно, максимум двух.
Если задача очень большая, то перед тем, как ее делать, также можно обсудить какие-то архитектурные моменты в команде, чтобы как раз не править потом глобально после ревью уже готовую задачу.
Ну и также в момент разработки тоже можно советоваться с коллегами по каким-то вопросам, чтобы не возникало потом каких-то больших переделок на ревью.
Самые большие ревью с большим количеством правок возникают обычно в ситуациях:
в команду пришел новый человек и это его первая задача (и возможно она была сразу большая)
задача была большая, плохо декомпозирована и возникла проблема в реализации, требуется много правок
Если такое происходит, то главное сделать выводы из ситуаций и в будущем скорректировать процессы, чтобы их избежать. Например, давать новым сотрудникам задачи поменьше, обсуждать реализацию до начала разработки задачи (общие моменты: вроде "вот тут добавляем поля в БД, здесь делаем такие-то классы, вот этот функционал будет реализован аналогично такому-то"), дробить задачи на более мелкие.
maria_yudina Автор
05.10.2021 01:24Ну и еще вариант - если задача сделана, всё работает, а задачу очень ждут и времени мало, то замечания по ревью (если они не критичные) можно вынести в отдельную задачу с техдолгом и взять ее в следующий спринт (если у вас спринты).
DMGarikk
04.10.2021 13:30+3не увидел одного существенного минуса в ревью, который иногда реально существует (сталкивался два раза, причем один раз был крайне токсичным)
а именно — навязывание ревьювером решения на основе его личного видения задачи
поясню
у нас в одной конторе было ревью, всё как в статье, все смотрели код друг друга, писали замечания улучшали качество и т.п.
но у нас был один товарищ (чуть выше по иерархии остальных), который считал что его точка зрения на реализацию любого сервиса гораздо важнее, всегда отписывал на каждом ревью огромные комменты, вплоть до того что 'а вот я тут в своей веточке все сам сделал, у вас както плохо вышло'
причем он был везде в списке аппруверов и насмотртя на правило 'мы даем только рекоммендации если не нравится реализация но она работает', он никогда не ставил аппрув если чтото ему не нравилось
в итоге вся комманда писала код так чтобы этот единственный чел был доволен.
и еще разок сталкивался с подобным человеком уже в другом месте, он сильно придирался к форматированию кода и тоже постоянно тянул ревью по времени… буквально пару часов в комментах можно было спорить по поводу очередности блоков кодаmaria_yudina Автор
04.10.2021 13:54Про это как раз написано в месте - что все ревьювят всех. Я считаю, что не должно быть такого, что кто-то один в команде принимает все решения. Вы правы - это бывает очень токсично и непродуктивно + вредит продукту скорее всего. Команда все-таки должна уметь договариваться и уважать мнения друг друга. Если есть аргументы, почему какое-то решение человек считает подходящим, то можно донести это при ревью и ревьювер может пропускать код, даже если был изначально не согласен с чем-то. Спасибо, что вынесли этот кейс в комментарий.
Protos
04.10.2021 16:45Добавите упоминание кейса в текст статьи?
maria_yudina Автор
04.10.2021 17:06+1Я думаю, что имеет смысл на основе вопросов в комментах сделать отдельную статью и в ней практические ситуации разобрать, т.к. это уже прям конкретные особенности команды и культуры в ней
Protos
04.10.2021 19:28Значит предлагаете на вас подписаться?
maria_yudina Автор
04.10.2021 20:06Да нет, я думаю оставлю тут ссылку потом. Если хотите, можете подписаться, но не заставляю)
Affdey
04.10.2021 13:40-1Заметка справедлива, как мне кажется, для однотипных решений в коде и боле-менее похожих задач. И когда программистов много. Когда программистов мало и они делают абсолютно разные проекты (иногда в разных ЯП), то затраты времени на вникание в код нерационально велики. В таком случае можно договориться о каких-то правилах, основных именах, функциях, для возможной поддержки кода не автором, но не проверять всё. Просто вводится персональная ответственность автора. Подозрительные моменты можно найти при проверке чужого кода, но так ли код работает, как надо по ТЗ - выяснится только на тестовых запусках программы.
Опишите ваш опыт, потому что я не понимаю, как это делать в малой команде.
maria_yudina Автор
04.10.2021 13:57+1Мне кажется, что в небольшой команде наоборот легче всего ревьювить. И быстрее всего. Вот когда продукт разделен на разные части и там разные команды отвечают за код, а вы отдаете в ревью что-то другой команде - то вот тут могут быть случаи, что ревью зависло и надо отдельно напоминать про него другой команде. Уже второй комментарий про опыт, наверное действительно стоит сделать отдельно статью про реализацию)
Ghedeon
04.10.2021 13:56Как вы боретесь с просьбами в личке "заапрусь позязя, релиз горит, PO копытом землю бьет"? Это самая уродливая часть всех code review поголовно, когда остается такая брешь для человеческого фактора и начинаются подковерные игры. "Ноет в личке, но отказать нехорошо, ведь через пару дней на его месте буду я" или "о, мудила тянул мой PR до последнего, пусть теперь подождет, проникнется". Как быть, если я вообще не хочу ни с кем обсуждать пулл-реквесты в привате, потому что для меня это рабочий процесс, а не предмет личной услуги?
maria_yudina Автор
04.10.2021 15:17У нас такого, к счастью, нет) Мы в целом договорились что ревью за 1-2 дня проходит.
Но я сталкивалась раньше с такими случаями. Мы решали такие вопросы с тимлидами и в целом с четким воркфлоу в командах. Например, у нас были случаи, когда другая команда вносила правки в код нашей команды в рамках их задачи и ревью было на нас - мы договорились, что у таких ревью приоритет выше, чем у всех остальных ревью и старались их делать быстрее. Ну и в обратную сторону - аналогично. Другие команды старались наши ревью смотреть быстрее и приоритетнее.
Вообще ситуация "срочное ревью" возможна только если это багфикс чего-то на бою и обычно там не очень много кода и ревью можно быстро провести. В остальных случаях по идее не должно быть такой срочности. Если она есть, то, по-моему, тут что-то не так с процессами, возможно, на уровне приоритетов задач от PM.
Ну и в игры "тянул мой PR, я тоже потяну" - для начала просто не вступайте, это непродуктивно и никому от этого хорошо не будет. В целом это вопрос про культуру в командах. Если есть возможность, можно это обсудить на ретроспективах или донести до тимлидов команд. Чтобы был процесс, который все понимают и не было необходимости отвлекать кого-то в личке. Ну и посмотреть, почему вообще с какими-то задачами возникает такая срочность именно на этапе ревью и действительно ли она возникает или это просто разработчик хочет быстрее закрыть тикет, а то такое тоже бывает.
ookami_kb
04.10.2021 14:23Прекрасный гайд по проведению код ревью: https://google.github.io/eng-practices/review/
Интересные вопросы здесь в комментариях, аж захотелось написать статью с разбором этих вопросов.
У нас в компании очень серьезный подход к код ревью, они обязательные и двухуровневые. Если интересно – поделюсь опытом. Пишите наболевшие вопросы в комментариях, это будет практичнее.
Areso
05.10.2021 09:47В чем заключается двухуровневость? И как она реализована?
maria_yudina Автор
05.10.2021 11:26Я подозреваю, что имеется в виду, что ревьювят минимум два человека – то есть нужно два апрува, прежде чем задача пройдет дальше.
ookami_kb
05.10.2021 13:15+1Не просто 2 человека, а именно 2 уровня ревьюеров, т.е. первый – это может быть джун/мид, второй – сениор/тех лид.
RaspopovVladimir
06.10.2021 11:30Большое спасибо за статью, информация очень хорошо изложена. Данная тема очень актуальна.
KseniaKichka
06.10.2021 23:13Спасибо за статью! Как по мне, код ревью это замечательный процесс, если он налажен и отработан в команде, и все стараются выполнить его флоу
ilnoor
07.10.2021 07:25Не надо пихать код-ревью в процессы, если без него можно обойтись. Код ревью (даже двойное и тройное) ничего не гарантирует, но зато дорого стоит, дает почву для конфликтов и много другого нежелательного
maria_yudina Автор
07.10.2021 09:08Согласна с тем, что Code Review ничего не гарантирует, но не согласна, что его не стоит использовать. По-моему, плюсов гораздо больше, чем минусов.
Насчет почвы для конфликтов – это опять же про культуру в команде и soft skills. Если люди не могут договориться, не могут корректно давать обратную связь и воспринимать ее, то тут проблема не в Code Review.
Видео посмотрю, спасибо за ссылки!
saboteur_kiev
Есть ощущение, что спутан процесс code review и непосредственно анализ изменений, как процесс аналитический. Инструменты приведенные в конце списка - IMHO из разных категорий.
Было бы хорошо описать, зачем нужны именно эти инструменты, так как изначально code review system - это немного другие инструменты, а именно github, bitbucket, gerrit. И если взять и сравнить атлассиновские Bitbucket, Crucible (сюда можно еще и Fisheye добавить), то все они могут использоваться для просмотра и сравнения кода (code review). Но если говорить о code review как часть процесса в SDLC, то в современной разработке основное все-же BitBucket, и если говорить про пулл реквесты и код ревью пулл реквестов, статья должна быть немного другая.
Другими словами - в статье описан code review, который должен являться частью SDLC (и описан поверхностно), а инструменты приведены для другой задачи - более глубокой аналитике во просмотра изменений в коде, причем не обязательно связанных с пулл/мерж реквестами. Например при помощи Crucible можно сравнить не изменения в конкретном коммите, а два бренча, которые давно разошлись друг с другом и имеют сотни изменений.
maria_yudina Автор
Не соглашусь. Здесь именно про Code Review и инструменты в конце статьи могут использоваться для Code Review в командах (это не значит, что они только для этого). Более того, был опыт использования большинства из них при ревью в разных компаниях. Кстати, про Fisheye хорошее дополнение.
Но вся суть – не в инструментах, а в подходе. Можно ревьювить код задачи без специальных инструментов, а просто переключаясь в ветку задачи и смотря комиты по ней в в вашей текущей IDE. Инструменты я привела просто в качестве примеров, если вдруг кому-то нужно будет.
saboteur_kiev
Во вы перечислили инструменты аналитики, а ниже в комментариях пишете, что на самом деле в текущем проекте используете обычные пулл реквесты и
Но это же немного другое.
Пулл реквест это функционал, который встраивается в CI/CD систему, на пулл реквест можно навешать различные тесты, анализаторы и так далее. И да, конечно там тоже есть просмотр диффа, возможность комментирования аппрувалы.
Но вот список софта, который приводился в статье - он в первую очередь об удобом сравнении множества разных коммитов для просмотра их человеком.
Например банальный вещь, которая реализуется через pull request - автоматическая проверка названия фича бренча, чтобы он соответствовал нейм конвеншену, и проверка коммит сообщения, чтобы оно, например, начиналось с номера тикета в JIRA, запуск компиляции и даже интеграционных тестов, и автоматический аппрувал от билд системы, который не позволит замержить пулл реквест, пока билд не успешен.
А такая вещь, как Crucible или FishEye - это совсем не для этого.
maria_yudina Автор
Я перечислила примеры инструментов, которые могут использоваться для Code Review. И буквально первая ссылка в этом списке – ссылка на GitHub, где написано про Pull Requests: "Pull requests are fundamental to how teams review and improve code on GitHub."
Для всех остальных упомянутых в статье инструментов также на главных страницах по ссылкам явно написано, что они используются в том числе для Code Review. Про Crucible опять же буквально написано в официальной документации "Crucible allows you to request, perform and manage code reviews."
То, что можно настроить CI/CD, это прекрасно, но это никак не относится ни к теме статьи, ни к моим комментариям, ни к тому как эта статья задумывалась. И то, что указанные инструменты можно еще как-то использовать, помимо непосредственно Code Review – тоже. Я нигде не пишу, что это их единственное назначение и/или что это все инструменты для ревью, которые есть. Не было цели описать инструменты и все кейсы их использования.
Если для вас в статье нет ничего нового, а указанные инструменты вы используете иначе – это ок. Я не вижу тут причины для споров. Просто, возможно, вы не целевая аудитория этой статьи.