Прообраз code review появился в 60-х годах прошлого столетия, когда программы писали на перфокартах. Главной проблемой тогда было преобразование программного кода в машинный — компиляция. Это сложный процесс, чувствительный к ошибкам и структуре написанного кода. Если в процессе генерации всплывала одна незначительная ошибка, приходилось начинать процесс заново: набирать, проверять и занимать очередь доступа к системе, которая могла длиться месяцами из-за большого количества желающих воспользоваться компьютером. Из-за высокой цены ошибки программисты досконально проверяли перфокарты друг за другом.
Сегодня ошибки в информационных системах по-прежнему стоят недешево, хоть исправлять их попроще. Проверка кода одним программистом за другим получила широкое распространение и сегодня известна как практика code review.
Набор преимуществ
Еще 20 лет назад мало кто знал о code review, 5 лет назад инженеры-разработчики не представляли как без него обойтись, а сегодня ищут альтернативные методы. В своей работе в 90% случаях мы прибегаем к code review, потому что плюсы подхода очевидны:
Экономия времени и бюджета
Много ошибок в бизнес-логике, архитектуре и алгоритмах можно отловить как раз на этапе code review. Если баг будет обнаружен в момент ручного тестирования, то тестировщику нужно пойти к инженеру и указать на ошибку. Инженер в это время занят уже другим контекстом и чтобы внести исправления, ему надо оставить текущую задачу и вернуться к прежней. На этапе code review на корректировку уходит меньше времени. Когда же ошибки всплывают на этапе продакшена, это может привести к финансовым потерям.
Универсальный код
Все инженеры пишут по-разному, у каждого свое понимание чистоты кода. И то, что напишет один, не всегда может правильно прочитать второй. Проблема решается за счет внедрения правил написания кода в команде в начале работы. Но на практике визуальные составляющие кода отлавливаются и исправляются именно на code review.
Снижение bus-фактора
Термин «bus-фактор» (фактор автобуса) появился из вопроса: «Сколько членов команды должен сбить автобус, чтобы провалить проект?». Если в кодовой базе есть блок, которым владеет только один инженер, то bus-фактор равен единице. С инженером может что-то случиться (заболеет, уволится, незапланированный отпуск) и тогда другим будет проблематично разобрать блок с нуля. Чем тут помогает code review: если инженер проверяет код, который написал исполнитель, то в процессе вникает в структуру, что в будущем поможет внести правки в отсутствии первого инженера.
Неформальное обучение
В процессе проведения code review возникает синергия: разбираясь с готовой работой, люди видят разные подходы, у них появляются новые идеи, их можно использовать в дальнейшем.
Обратная сторона медали
Безусловно, споры о целесообразности применения code review ведутся не просто так, у практики есть существенные недостатки:
Организация процесса
По опыту, на организацию проверки кода уходит от 20 до 50% от времени разработки: инженеру требуется поменять контекст, вникнуть в суть задачи, провести ревью, оставить комментарии, потратить время на урегулирование споров и все это время прогресс по основной задаче проверяющего стоит на месте.
Это можно решить, просто регламентируя время. Например, мы в команде разработки взяли за правило, что задача по code review ставится в Jira, тогда она должна быть выполнена в течение одного рабочего дня. Если вопрос срочный, задачу можно кинуть исполнителю уже лично, даже просто в сообщениях, тогда результат можно получить совсем быстро.
Ментальные сложности или потеря мотивации
Очень реальная ситуация: исполнитель старался, делал блок кода, а после получил от коллеги 20 комментариев из серии: «Нужно все переписать». В этот момент снижается самооценка, вместе с ней интерес к работе, а иногда и отношения с коллегами. Тут как минимум стоит отдельное внимание в команде уделить тактичным формулировкам в комментариях к ошибкам, выработать общие правила.
Сложно найти ответственных
При проведении code review сложно назначить ответственного за результат. Например: Вася написал блок кода, Петя провел code review, QA-инженер Витя проверил, не нашел замечаний, а на продакшене всплыл баг. Кто в этой цепочке несет за него ответственность? Вася, Петя, Витя, а может тимлид Николай? Или все вместе? Правильного ответа нет. Если назначить ответственным исполнителя, то ревьюер может сделать работу тяп-ляп — цель code review теряется. Если отвечает ревьюер, то исполнитель может писать неаккуратно. И так по кругу.
Чем можно заменить code review
Меняем «code» на «design»
Наиболее прогрессивная практика сегодня, на мой взгляд, — design review. Исполнитель получает задачу и перед тем как приступить к выполнению, продумывает работу и описывает техническую реализацию: какие функции планирует прописать, как они будут связаны друг с другом, какие будут интерфейсы, и прочее. Тимлид или технический координатор проверяют этот текст, дают советы по улучшению, возвращают исполнителю и тот приступает к задаче.
Метод design review пока широкого распространения не получил. Во-первых, потому что многие идеи появляются лишь в процессе работы над задачей, заранее спланировать их сложно. Кроме того, есть мнение, что подготовительная работа не связана с качеством кода. На мой взгляд, это не так — созданные документы дисциплинируют писать четко по инструкции, что минимизирует ошибки. Плюс, когда инженер двигается по плану, ему проще вернуться к задаче. Однако у метода есть ряд сложностей внедрения: все новое обычно вызывает дискомфорт, страх полностью заменить code review и допустить ошибки, многие команды отказываются писать какие-то документы. С подготовкой описания связана проблема идеального объема — формирование мыслей в сжатой форме — это навык, которому нужно учиться, а у инженеров иногда получаются подробные и абстрактные рассуждения.
Парное программирование
Практика парного программирования распространена больше, чем design review, но как полноценную замену code review её рассматривают редко. Суть заключается в том, что два разработчика одновременно делают одну задачу (рядом или во время созвона) и в процессе проверяют друг друга.
Плюсы:
баги ловятся быстрее — две пары глаз лучше одной;
созданный код понятен минимум двоим и высока вероятность, что и другие смогут разобраться в его структуре;
bus-фактор равен двум;
обмен знаниями в парной работе.
А главный минус — эффективность. Есть исследования, которые подтверждают, что, когда два человека делают одну задачу, скорость выполнения увеличивается на 40-50%. Но с другой стороны, если два человека будут делать каждый свою задачу, то времени затрачивается меньше, чем при парном выполнении сначала одной задачи, потом другой. К тому же не всех разработчиков можно посадить рядом.
Метод единичной ответственности инженера
Недавно узнал о новом подходе — человек, который проводит code review, не просто смотрит код, а скачивает его, запускает на машине и проверяет работу задачи. В этом случае ответственность лежит полностью на ревьюере, QA-инженер не нужен вовсе. Подобный процесс пока подходит только для смелых ценителей, так как у него много ограничений.
Во-первых, компания тратит в 1,5-2 раза больше денег на ревью и тестирование, так как инженер-разработчик попросту стоит дороже QA-инженера. Во-вторых, есть ментальные ловушки: тот кто, провел code review, знает, какие куски кода были изменены, а какие нет, и иногда ревьюер убежден — то, что не менялось, не может сломаться. Но проекты большие и охватить их полностью сложно. Могут быть ситуации, когда ревьюер подумал, что раз часть кода не менялась, то ошибок не будет и пропускает ее, а изменения эту часть все-таки затронули и в итоге всё ломается. В случае, когда на проекте есть QA-инженер, который ничего не знает про изменение кода, подобной ловушки нет — он проверяет всё.
Подведем итоги
В своей работе я пока не нашел полноценной замены code review. Иногда совмещаем практики: design review проводим при сложной задаче, проверяем в том ли направлении мыслим, а парное программирование используем при выявлении сложного бага. Но это лишь 10 случаев из 100.
Когда в команду приходит новый человек, при выполнении первой задачи он может сделать много ошибок, так как еще не погружен в контекст. За счет code review адаптация происходит быстрее. В design review велика вероятность что-то упустить. В парном программировании теряем эффективность. Так что альтернативы для погружения в контекст без отвлечения других людей на сегодняшний день, думаю, пока нет.
Ещё по теме:
Доклад Филиппа Дельгядо: Одна голова хорошо, а две — лучше (YouTube)
Code review — долго, плохо, дорого -- sharovatov.github.io
Максим Быстров
CTO SaaS-платформы TYMY
Комментарии (25)
Troonsply
09.10.2023 10:55Code Review заменить сложно!
Я тоже искала альтернативы, чтобы сэкономить время, но ни одна "альтернатива" такого итога не предвещает = (
Firsto
09.10.2023 10:55+2не всех разработчиков можно посадить рядом
С шарингом экрана при видеозвонке вполне себе норм получается.
bibiw_one
09.10.2023 10:55Code With Me тоже неплохо работает, если оба разраба используют IDE JetBrains
mbystrov_23 Автор
09.10.2023 10:55+1Это скорее было про уровень инженеров + их софт скиллы, а не про их географическую расположенность, не дописал мысль:) Если посадить рядом стажера и супер мега опытного человека, то для первого будет очень полезно конечно, а для второго это потеря времени в разрезе времени выполнения задачи. Конечно тут уже возникают другие преимущества в виде менторинга, но это уже отдельная задача, которая целью Code Review не является
ioncorpse
09.10.2023 10:55+1DesignReview - если сложная фитча вполне возможен в твоей ветке, нет?
И CodeReview на PR.
И можно парное программирование, если пишем для банка допустим, где цена ошибки заоблочная.
Не понимаю, что не так то?mbystrov_23 Автор
09.10.2023 10:55Все так, собственно, мы используем в разных кейсах все эти практики. Целью статьи было показать плюсы минусы CoreReview и предоставить некоторые альтернативы также с плюсами минусами :)
dididididi
09.10.2023 10:55+1Про codeReview там принципиальная ошибка. Человек делающий codeReview не проверяет архитектуру, и не вдумывается в код и не знает бизнес-логики.
У меня была 10 работ и только три из них, с code-review. Отгадайте, где был самый плохой код?)
Goodzonchik
09.10.2023 10:55+2Кажется разумным автоматизировать все, что можно автоматизировать: линтеры/форматтеры кода и прочие плюшки, которые делают код красивым. Чтобы не тратить на это время при ревью.
Иметь набор договоренностей и стайлгайды, которым следует команда. Но они могут пересматриваться
Делать какую-то спецификацию дизайн разумно, если делается что-то новое. Можно выделить отдельным этапом во флоу разработки, и еще на этапе анализа понять как делать.
dididididi
09.10.2023 10:55Вы точно мне ответили? Мне ваще всё равно на всё эти стайлы, из минусов только, если каждый по своему форматов в коммиь куча всякой отформатмрованного кода попадает.
Но вон какая по дефолту настройка стоит, ту и юзают обычно.
Спецификация дизайн - это что вообще?
Goodzonchik
09.10.2023 10:55Ответ был не Вам, а общим комментарием к статье)
Понятно, что там где нет code-review все будет очень плохо, сам работал без него однажды
Использовать дефолтные настройки не очень правильно, потому что у разработчиков могут быть разные IDE с разными дефолтными настройками. Разумно иметь единый источник правил, чтобы у всех разрабов был единый формат кода (был случай, когда поменял одну строку, но переформатировался весь файл).
"Спецификация дизайн", опечатался немного (спецификация или дизайн-ревью), но подразумевал спецификацию решения, или дизайн ревью решения. Какая-то активность, результатом которой будет описание технического решения, которое устроит остальных разработчиков, системных аналитиков или архитекторов (смотря какая команда)
dididididi
09.10.2023 10:55У меня обратный опыт и убеждение, что код ревью И юнит тесты вводят на в ноль закаканных проектах, где каждая строчка может всё свалить и требуются тройные проверки. И это та жизненная необходимость,. Если код простоя и качественный, то и юнит тесты ни код ревью.
Но даже я был бы рад, увидеть какие нить независимые исследования, где показана корреляция между наличием тестов,код ревью и качеством кода.
В качестве дурацкого примера могу привести, то что в Норвегии даже Аналога нашего ОМОНа не существует, но это не значит, что там преступность распоясалась.
wolfandman
09.10.2023 10:55+1Мы вдвоём с коллегой разработали проект за одним компьютером в несколько месяцев. Очень классно. Главное, чтобы опыт был неновичковый. Идеи проговариваются, критикуются, совершенствуются очень быстро.
IVNSTN
09.10.2023 10:55+1на практике визуальные составляющие кода отлавливаются и исправляются именно на code review
Это не самый удачный вариант, нужно сдвигать контроль оформления влево. Для большинства популярных языков есть и линтеры, и форматтеры, настройки (соглашения) как правило легко шарятся в виде закоммиченного в папку проекта файла. Разработчику нужно выдать экстеншн или инсталлятор того же инструмента, контроль переложить на CI, который бы не пропускал несоответствия.
задача по code review ставится в Jira
Бывает и такое, что обеденный перерыв ставят и в JIRA, и в календарь Outlook. Но до этого должен был случиться неслабый такой поворот не туда. Если это что-то решает, то почему бы и нет, но пахнет, как говорится, не очень хорошо.
Метод единоличной ответственности инженера
Такое есть, в рассуждениях про QualityGate как правило рассматривается как один из интипаттернов. Назначенный крайним быстро превратится в хранителя единственно правильного мнения и хорошо от этой ситуации не будет никому.
В парном программировании теряем эффективность
Суть рассуждений понятна, но формулировки кажутся не самыми удачными. Есть задачи, над которыми вдвоем сидеть странно и неуместно. Есть задачи, решение которых (далее вольно цитирую вас же) при парном подходе происходит быстрее, решение оказывается лучше, надежнее, эффективнее. И в рассуждениях, видимо, про финансовые затраты имеет смысл учитывать, что один бы делал дольше, сделал бы хуже, на ревью все равно бы двое тратили время, причем второй вникал бы с нуля, потом бы с ненулевой вероятностью все они повторили упражнение, т.к. рефактор и переосмысление непременно случились бы. Рассуждения об эффективности уместны в любом случае, но однозначный вывод возможен только если удастся куда-нибудь какую-нибудь линейку приложить и фактически увидеть, что где-то чего-то больше, а в другой ситуации - меньше.
За счет code review адаптация происходит быстрее
Не смог разгадать этот паззл. Толкать на прод не глядя от только что пришедшего в коллектив сотрудника и никак его не онбордить - это предполагается как альтернатива? Возможно просто не совсем точно сформулировано, но пахнет странным. Как если бы в нового сотрудника молча кидались задачами и только на ревью сообщали "неправильно сделал". Адаптация - она же сама по себе должна существовать, как процесс, тот самый "онбординг". Там же и про местные подходы, архитектуру, инструментарий тоже. А ревью как был самим собой и играл какую-то роль во взаимодействии с остальными разработчиками, тем же элементом процесса и для того же и остался.
mbystrov_23 Автор
09.10.2023 10:55Это не самый удачный вариант, нужно сдвигать контроль оформления влево
К сожалению, не все можно автоматизировать в виде линтеров, поэтому до сих пор пишутся стайл гайды. Но вы правы, нужно автоматизировать все, что можно автоматизировать
Бывает и такое, что обеденный перерыв ставят и в JIRA, и в календарь Outlook. Но до этого должен был случиться неслабый такой поворот не туда. Если это что-то решает, то почему бы и нет, но пахнет, как говорится, не очень хорошо.
У нас используется обычная канбан доска, есть статус "в процессе" за ним следует статус "Code Review" Когда требуется отдать задачу в ревью, инженер двигает ее в этот статус и ставит нужного инженера на ревью. Всем удобно, все довольны. Объясните поподробнее пожалуйста, почему это не очень хорошо пахнет?
Суть рассуждений понятна, но формулировки кажутся не самыми удачными....
Со всем написанным полностью согласен, по формулировкам дело вкуса, в ваш я, к сожалению, не попал :)
Не смог разгадать этот паззл. Толкать на прод не глядя от только что пришедшего в коллектив сотрудника и никак его не онбордить - это предполагается как альтернатива?
Постараюсь развернуть мысль
Это было написано к тому, что у code review есть минусы, но при этом, если его убрать, возникает проблема с новыми инженерами, как вы и написали. Как альтернативу в этом случае я как раз ничего не предлагаю, тк считаю code review в данном случае максимально полезной практикой. А адаптация происходит быстрее за счет того, что ревью проводит не один человек, который онбордит новичка, а вся команда(в сумме нескольких задач конечно)
P.S. Благодарю за развернутый комментарийIVNSTN
09.10.2023 10:55Когда требуется отдать задачу в ревью, инженер двигает ее
вот здесь
задача по code review ставится в Jira
формулировка, похожая на создание новой задачи "делать ревью вон той задачи". Если существующая задача просто двигается в другой статус, то вопросов нет.
enkryptor
09.10.2023 10:55+2Мой опыт разработки (больше восьми лет в разных командах) показывает, что лучше всего работает т.н. peer review — когда код проверяет коллега твоего же уровня. В этом случае роли чётко разделены:
Автор кода целиком несёт ответственность за свой код. Однако он не может оценить читаемость — для него его код всегда понятен — поэтому привлекает коллегу для code review.
Ревьюер читает код и убеждается, что код понятен и ему тоже. Он заинтересован в этом, потому что в дальнейшем ему скорее всего придётся поддерживать данный код. Но саму реализацию он не проверяет, доверяя коллеге как эксперту.
Поддержка уровня читаемости кода — задача, с которой code review справляется, не добавляя лишних проблем.
Другой допустимый вариант: когда старший разработчик выступает ментором для джуна. В этом случае он проверяет работу джуна целиком, включая весь написанный им код. Но это уже не совсем code review, это скорее проверка навыков ученика, то есть преподавательская деятельность. Тут роли тоже ясны — ментор готов тратить своё время на обучение джуна, а джун хочет научиться. Ответственность за качество итогового кода ментор целиком и полностью берёт на себя.
Проблемы начинаются, когда на code review пытаются навесить другие функции, пытаясь таким образом удешевить разработку — сэкономить на сеньорах или тестировщиках. Часто это выглядит так:
Есть несколько миддлов, которые не дотягивают до уровня проекта, но были наняты за неимением лучшего (например, других не заинтересовали предлагаемые условия).
Есть один сеньор, который прекрасно ориентируется в проекте, но не успевает всё делать сам.
Руководство вешает на сеньора обязанность проверять за миддлами их работу, надеясь таким образом и разгрузить сеньора, и не упасть в качестве.
В итоге имеем описанный в статье букет: приходится переделывать уже написанный код, миддлы демотивируются, портятся отношения в команде, сеньор всё равно перегружен, не всегда ясно кто ответственен за ошибки в коде.
Исправить это со стороны процесса не получится, т.к. причины проблемы не организационные. Но есть способы облегчить ситуацию:
Сеньору целенаправленно выделять время для работы с миддлами. Принять, что это теперь его основная обязанность.
Миддлам обсуждать с сеньором реализацию до написания кода, а не после.
Дробить работу на задачи меньшего размера.
Сеньору прокачивать навыки ненасильственного общения.
Не играть в "бокс по переписке" в комментариях к ревью. Любой диалог длиннее двух реплик должен проходить в формате в очной встречи или созвона.
Автоматизировать линтерами всё, что можно. Настроить прекоммит-хук, чтобы неправильно отформатированный код в принципе не мог попасть в ветку. Не тратить время на проверку code style на ревью.
Писать юнит-тесты и настроить CI/CD на каждый пуш. Приступать к ревью только по факту успешного прохождения тестов.
Отдельный разговор про тестирование — ведь задачу могут вернуть на доработку, что повлечёт за собой новый цикл code review, и так по несколько раз. Но это уже совсем другая история.
mbystrov_23 Автор
09.10.2023 10:55А сталкивались ли в peer review с тем, что ревьюер на самом деле не понимает код, но боится/стесняется показаться глупым и поэтому делает апрув, не разобравшись в написанном?
enkryptor
09.10.2023 10:55Я не могу однозначно ответить «нет», так как не знаю, что происходит в голове у человека, но мне на такое не жаловались.
Цель code review — убедиться, что написанный код понятен другим участникам команды. Если код непонятен, а вопросы не задаются из страха перед коллегами — это тревожный симптом, который может являться частью большей проблемы.
dididididi
09.10.2023 10:55Что значит мидл не дотягивает до уровня проекта?)))
Если уровень проекта хороший, то там и джун разберется.
Так уж и пишите, ацкоей калище и даже мидлы не тащат) Приходится "сеньору" объяснять, что он там накропал.
Algrinn
О код ревью, если кратко, это самый лучший способ испортить отношения с командой. Если человек пишет полную дичь, я ему об этом сообщу. Если человек пишет дичь только наполовину, то я ему об этом сообщать не буду. Если тебе говорят сделать полный бред, то я об этом сообщу. А если наполовину, то нет. У тебя у самого проблемы с безопасностью начнутся, если тебя будут пытаться закритиковать до смерти. Особенно если учесть тот фактор, что начальство имеет право удалённо следить за рабочими мониторами. Ревью должны делать аудиторы.
Algrinn
Вот, простой пример из реальной жизни: человек случайно назвал переменную Snake Case-ом. Вроде first_name, а не firstName. Язык Java. Ревью, комментарий: "Смесь Camel Case и Snake Case в классе". Человек был на испытательном сроке. Человека после этого чуть не уволили. Из-за подобных вещей атмосфера становится токсичной. Ну да, у нас серпентарий, добро пожаловать.
ZetaTetra
Почему-бы не использовать анализатор кода?
Вон, на яву полно всяких есть (Не силён в яве):
https://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis
Проблема в том, когда начинают обращать внимание на стилистику кода, вместо анализа самого кода, то больше шансов что пропустят действительно ошибку.
Ибо проверить стилистику может и программа, а вот найти логическую ошибку - может только человек.
Algrinn
В некоторых фирмах анализатор используется, а в некоторых нет. По поводу логических ошибок, эффективно самому дебажить. Потому что ты читаешь код и сомневаешься в том, что оно вообще работает и что человек вообще проверял работоспособность этого кода. И большие сомнения, что сценарии поведения в документе были правильно написаны. Не все могут работать на legacy
ZetaTetra
Ну анализаторы зачастую забесплатно раздаются, так что его и бизнесу-то продавать не надо особенно. Это ведь не M$ SQL Server Enterprise или IBM Web Sphere за кучу денег.
Хоть просто себе его поставить и настроить для начала. Как Notepad++, ConEmu, 7-Zip, IrfanView и т.п.
Ну а человек просто по невнимательности что-то написал и не проверил. Или написал, проверил, а потом решил код причесать и не проверил. Человеческий фактор никуда не денется в любом случае. Да и анализатор это ведь не полноценный CI/CD, а просто дополнение к IDE или компилятору.
l4rover
Уволить из-за ошибки в названии переменной? Ну может и ну его нафиг в такой фирме работать действительно...
У меня на проде, уверен лежит парочка не так названных переменных, а одно время целый интерфейс и соответственно куча его наследников были неправильно названы из за ошибки русской/английской с/c.
А проект самый высоко нагруженный в зелёном банке, ничего, никто не умер, исправили как нашли.
Ну и со стороны стажёра расстраиваться из за того что в замечании нет слов "ничего страшного, пожалуйста не волнуйся, но вот тут небольшая ошибочка" это тоже странно, как в обществе то жить, если в любом автобусе есть шанс ушат помоев огребсти)