Сегодня очень многие в разработке используют ревью кода. Практика полезная, нужная. Даже если вы не делаете ревью, вы наверняка знаете, что это такое.
На рынке есть куча инструментов для ревью кода с готовыми сценариями использования, рекомендациями и правилами. GitHub, Phabricator, FishEye/ Crucible, GitLab, Bitbucket, Upsource — список можно долго продолжать. Мы в Badoo тоже в своё время с ними работали: в своей предыдущей статье я рассказывал нашу историю ревью кода и о том, как мы пришли к изобретению собственного «велосипеда» — решения Codeisok.
Информации предостаточно, можно нагуглить кучу статей про ревью кода с реальными примерами, практиками, подходами, рассказывающих о том, как хорошо, как плохо, как нужно делать, а как — не нужно, что стоит учитывать, а что — нет, и т. д. В общем, тема «обсосана до косточек».
Именно поэтому другую часть айсберга можно и не заметить.
Я надеюсь, что вы серьёзно отнесётесь к тем вещам, которые я опишу в этой статье, в некоторых случаях специально утрируя. Ревью, как и любой другой процесс, требует внимательного и обдуманного внедрения, тогда как подход «Сделаем как у всех, лишь бы было» может не только не сработать, но даже навредить.
После прочтения этого вступления у вас может возникнуть вопрос: а что сложного в ревью кода? Дело-то плёвое. Тем более что большинство инструментов, перечисленных выше, сразу и флоу ревью предлагают (из коробки: поставил, закоммитил/ запушил — и вперёд!).
Но практика показывает, что всё не так очевидно, как кажется на первый взгляд. В том числе из-за мнимой простоты процесса. Когда всё налажено и работает, есть риск, что менеджер успокоится и перестанет погружаться в процесс, передав его разработчикам. А они или будут следовать процессу, или займутся чем-то другим в ущерб решению проблем, которые процесс ревью призван решать. Менеджер может и не догадываться, что что-то идёт не так, потому что не даёт установок или не навязывает правила. В то время как для бизнеса очень важно решать задачи как можно быстрее, не тратя время на незапланированные активности.
Когда-то давным-давно, когда я работал в другой компании, мне в память глубоко запала беседа о ревью кода с одним из ведущих разработчиков. Это был p1lot. Возможно, кто-то из читателей знаком с ним (p1lot, привет :)). Сейчас он работает с нами в Badoo, и это здорово.
Так вот, PILOT тогда сказал: «Самое сложное для меня в процессе ревью — пойти на компромисс и пропустить готовую и корректно работающую задачу дальше, даже если я знаю другой способ её решения и даже если мой способ мне нравится больше». На мой взгляд, эта мысль является ключевой. И основной посыл всей моей статьи так или иначе вертится вокруг этого постулата.
Зачем нужен процесс ревью кода?
У вас в компании есть ревью? Правильно ли вы его делаете? У меня есть тест, который, возможно, заставит вас усомниться в этом.
Нужно ответить всего на три вопроса:
- Сколько времени занимает ревью кода для средней (сферической в вакууме) задачи?
- Как вы минимизируете время ревью?
- Как вы определяете, что ревью конкретной задачи сделано правильно?
Если у вас нет внятных ответов на некоторые (или на все) вопросы, если вы сомневаетесь в своих ответах, то осмелюсь предположить, что что-то идёт не так.
Если вы не знаете, сколько времени потребуется на ревью, и не минимизируете его постоянно, то как вы осуществляете планирование? Возможна ли ситуация, когда исполнитель, который делал ревью четыре часа, не пил чай половину этого времени? Можно ли быть уверенным в том, что от задачи к задаче время на чаепитие не увеличивается? А, может, ревьюер вообще не смотрит код, а просто нажимает «Code is OK»?
И, даже если ревью делается добросовестно, как добиться того, чтобы с ростом кодовой базы и компании время на реализацию задач не увеличивалось? Ведь руководство, как правило, ожидает, что процессы будут ускоряться, а не замедляться.
Если же ответ на третий вопрос сразу не приходит в голову, то есть смысл задаться ещё одним. Знаете ли вы, зачем вам на самом деле нужен этот процесс? Потому что «так принято у всех больших ребят»? Может быть, он вам и не нужен вовсе?
Если вы спросите своих лидов и разработчиков о том, что представляет из себя «правильное» ревью кода, вы очень удивитесь тому, насколько разные вещи услышите. Ответы могут варьироваться в зависимости от личного опыта, убеждений, собственной уверенности в правильности или неправильности вещей и даже от используемого технологического стека и языка разработки.
Помните про готовые инструменты для ревью кода, предлагающие свои подходы и флоу? Мне, например, интересно, как бы ответили на вопрос разработчики этих инструментов. Будут ли коррелировать их ответы о «правильности» ревью с ответами ваших сотрудников? Не уверен. Иногда я грустно наблюдаю, как кто-то внедряет у себя инструменты ревью, ни на минуту не сомневаясь, что они необходимы. То ли люди делают это «для галочки», то ли чтобы отчитаться, что «ревью кода внедрили, всё хорошо». И в итоге забывают про это.
Не хочу быть Кэпом, но тем не менее. Общаясь с сотрудниками, обращайте внимание на ответы вроде «Потому что так заведено» или «Это же best practice, все так делают». Вы и сами отлично знаете, что не нужно бездумно внедрять какой-то процесс, не понимая, зачем он нужен. Любой процесс должен быть оправдан и внедрён (если необходимо) с поправкой на нужды бизнеса и проекта, а также на проблемы, которые действительно существуют и которые действительно хочется решить. Карго-культу в компании с хорошей инженерной культурой не место.
Соответственно, менеджеру важно донести до людей то «правильное» ревью кода, которое необходимо именно в его проекте. А перед этим, разумеется, стоит критерии «правильности» сформулировать для себя, выбрать подходящие инструменты и установить уместные правила. А там уже и до контроля недалеко.
Плохое ревью кода
Так какой он, тот самый правильный процесс? Давайте разбираться. Ниже будет много моих личных рассуждений, и тем, кому не терпится узнать, к чему я всё это пишу, предлагаю сразу перейти к части «Хорошее ревью кода».
Тем же, кто остался, говорю спасибо и тем не менее предлагаю определить правильность ревью самостоятельно, исходя из нужд вашего проекта, тщательно всё обдумав. А я лишь попробую вам в этом помочь.
Чтобы выделить для себя важные и необходимые вещи в любом процессе, бывает полезно для начала отсечь очевидно ненужные, которые вредят инженерной культуре. Так и поступим.
Обмен знаниями
На вопрос «Зачем нужен процесс ревью кода?» я очень много раз слышал ответ "Для обмена знаниями". Действительно, полезная и нужная вещь. И многие согласны с тем, что важно иметь в команде процесс для обмена знаниями и экспертизой. Но уверены ли вы в том, что ревью кода подходит для этого?
Я не раз спрашивал людей, что они имеют в виду под «обменом знаниями». И в ответ слышал разное.
Во-первых, это демонстрация новичкам (в команде или затронутом компоненте) принятых правил и практик: вот так у нас принято делать, а так мы не делаем, потому-то и потому-то.
Во-вторых, это свежий взгляд новичка (опять же в команде либо затронутом компоненте) на то, как делаются обычные вещи. Вдруг они делаются неэффективно, и новый человек поможет это обнаружить?
В-третьих, это просто ознакомление с некоторой частью кода. Ведь если в будущем ревьюер столкнётся с необходимостью изменять эту часть кода, ему будет намного легче.
Давайте пройдёмся по всем пунктам и попробуем оценить, насколько они уместны в процессе ревью.
Code review как инструмент обучения новичков
В данном случае речь идёт в основном о таком сценарии: новичку даётся задание, он его выполняет, а потом опытный член команды (владелец компонента) указывает на ошибки, которые он допустил. Процесс важный и нужный, не спорю — новичкам нужно как-то показывать принятые в команде правила.
Но давайте начистоту: не поздновато ли? Не будет ли более правильным рассказать новичку об этих правилах до допуска его к коду? Ведь цена ошибки очень высока — редко новички сразу работают так, как вам нужно. А значит, задача будет возвращаться и возвращаться на доработку. А значит, продукт станет доступен пользователю позже, чем мог бы.
Получается, в процесс, продолжительность которого трудно оценить, мы добавляем ещё один, временные затраты на который ещё менее предсказуемы. Таким образом мы увеличиваем время, необходимое на доставку задачи пользователю, на ещё более неизвестную величину.
Разумеется, иногда обучение новичков через рабочие процессы имеет право на существование. Но порой мы не задумываемся о недостатках подходов, которые вошли в привычку. А допустить ошибку тут не просто легко, а очень легко.
Например, если в компании нет отлаженного процесса обучения и коучинга, менеджер вынужден «бросить в воду» новичка. У последнего при этом не остаётся выбора: нужно либо «плыть», либо уходить из компании. Кто-то реально учится на таких ситуациях, а кто-то не выдерживает. Думаю, многие сталкивались с подобным на протяжении своего профессионального пути. Мой же посыл в том, что драгоценнейшее время может тратиться неоптимально из-за такого явления. Равно как и процесс адаптации нового сотрудника в команде может выполняться неоптимально. Просто потому, что ни у кого руки не дошли организовать эффективный процесс внедрения новых сотрудников в команду, и менеджер подумал, что новичок всему научится на этапе ревью кода. А на самом деле это два разных процесса, очень нужных и важных.
Конечно, даже при условиях, что правила изначально объяснены новичку и что в компании существуют другие процессы обучения, процесс адаптации новичка нужно как-то контролировать. Ревью — это один из процессов, который может помочь. Но контроль и обучение — это разные вещи, не так ли? Тем более что в контроле нуждаются все члены команды, а не только новички. Ведь все мы можем делать ошибки, забывать о договорённостях и так или иначе влияем на ту часть системы, которой владеет вся команда (в данном случае на код).
Какой можно сделать вывод? Описанный выше процесс направлен на достижение иной цели: не на обучение, а на контроль. И этот самый контроль необходим не только новичкам, а команде в целом.
Всё это легко формулируется в такой тезис: "Ревью кода необходимо, чтобы контролировать соблюдение договорённостей и общих правил всеми членами команды". А обучение новичков в этом случае будет не чем иным, как искусственной подменой цели, которая только запутает людей и направит процесс в другое русло.
Но даже с этим выводом, который, казалось бы, сформулирован правильно, можно наломать дров. Ревью кода — это этап, который наступает тогда, когда код уже написан. И может случиться так, что код, написанный без соблюдения общих правил, подгонять под общее лекало будет очень дорого. Наша задача состоит в том, чтобы сообщить исполнителю о том, что что-то пошло не так, как можно раньше. И поэтому я утверждаю, что иногда ревью кода — не самый подходящий процесс для контроля соблюдения общих правил. Во многих случаях проверки на соответствие правилам можно и нужно перенести на более ранние этапы.
Например, можно проверять форматирование кода в хуках системы контроля версий (равно как и использование задепрекейченных классов, договорённости о расположении файлов в соответствующих папках, использование внешних зависимостей и сторонних библиотек и др.). Таким образом минимизируется время, необходимое на ревью кода.
Продолжая разговор об обучении новичков в процессе ревью кода, проведём аналогию. Допустим, вы производите какой-то товар, например, торты. Допустим, вы получили заказ на торт для свадьбы VIP-персоны. Доверите ли вы «ревью» готового торта новичку? Готовы ли вы к тому, что новый кондитер возьмёт на себя ответственность за позор или успех всей пекарни на этой свадьбе? А может, вы хотите, чтобы он на этом этапе познакомился с правилами, принятыми в команде (как выпекать коржи, готовить крем и настаивать клюкву на коньяке)? Очевидно, что и процесс знакомства новичка с правилами, и контроль с его стороны — довольно странные вещи на данном этапе: торт-то уже испечён. Так почему же с фичами и кодом мы можем поступать по-другому? Или фичи, которые мы запускаем, не несут за собой позор или успех нашей команды в глазах пользователей и заказчиков?
Вы можете возразить, сказав, что задачи «для важных персон» мы готовим не каждый день и что новичку вполне можно доверить не очень срочную или не очень важную задачу. И будете правы.
Но во-первых, чему новичок научится на пустяковых задачах, в которых мало изменений кода (и эти изменения не в критичных местах и для бизнеса наверняка не столь важны, раз задача несрочная)? Как он может научиться печь торты, если всё время взбивает сливки? Переход от простого к сложному, конечно, нужен. Но делать это необходимо, тщательно контролируя процесс. Новичков нужно учить, а не бросать учиться самостоятельно.
А во-вторых, компании даже такой, казалось бы, полезный и правильный подход может навредить. Понять это очень просто, используя метод доведения до абсурда, чтобы сделать вывод максимально простым и понятным.
Представим, что мы открыто заявляем, что задачи, которые нам даёт продакт-менеджер, нужны для обучения новичков. Почему? А так же, как с ревью кода: мы ведь поручаем некоторые простые и несрочные задачи новичкам, чтобы они научились работать так, как у нас принято.
К чему это может привести в итоге? Ретивый исполнитель, который добросовестно делает своё дело и считает, что всё, что он делает, должно быть сделано максимально хорошо, примется за процесс обучения. Он может поставить задачу сделать несколько реализаций вместо одной, чтобы потом показать обучаемому недостатки и преимущества разных подходов. Он может читать лекции, сравнивая подходы и best practices, применяемые в компании и за ее пределами. Он может много чего ещё предпринять, чтобы обучить новичка. В результате время, требуемое на реализацию задачи, будет всё увеличиваться, ведь вместо разработки мы фокусируемся на обучении.
В случае ревью кода такой ретивый сотрудник инициирует длинную дискуссию в комментариях к ревью о пользе и вреде тех или иных реализаций, постит куски кода со Stack Overflow, показывая, что в других головах бывают другие идеи, даёт ссылки на статьи и книги, которые обучаемый должен прочитать, чтобы понять, что его реализация неидеальна.
Это нормальный процесс обучения, который может существовать, но который должен делаться правильно. И далеко не всегда стоит интегрировать его в процесс решения задачи бизнеса. Потому что это два разных процесса. Пусть новичок делает разные составляющие торта, пусть сделает несколько вариантов, пусть что-то испортит и выбросит. Пусть кто-то более опытный показывает ему, как нужно действовать и пересказывает старинные рецепты. Но учиться «на боевом конвейере», который производит ваш товар для клиентов — не лучшая идея. И если уж вы решились на такое, то «водите за ручку» новичка, не позволяя ему создавать помехи производственному процессу в ходе своего обучения.
Если ваша компания — это не институт, не школа, не техникум и не какое-либо другое образовательное учреждение, то обучение — не ваша прямая обязанность. Бизнес нанял вас для решения других задач и достижениях других результатов.
К слову, именно поэтому мы не добавляем в Codeisok функционал для загрузки картинок, оформления текста, подсветки кода в комментариях, хотя запросов на подобные фичи поступало и продолжает поступать немало. Мы пропагандируем идею того, что ревью кода — это не личный блог, не мессенджер и не сервис для обучения. Инструмент нужен для другого. Ведь для указания на недостатки кода обычного текстового комментария более чем достаточно.
Code review как свежий взгляд на код
Идём дальше. Следующий сценарий «обмена опытом» такой: мы в команде что-то делаем, соблюдая внутренние договорённости, и у нас замылился глаз, а тут пришёл новый человек (из другой компании или из другого компонента) — и, возможно, он увидит недостатки в нашей организации работы.
Идея хорошая, звучит разумно. Действительно, а вдруг мы делаем что-то не так?
Но опять вернёмся к сроку жизни задачи и наступлению этапа ревью кода. Не поздновато ли? Торт уже испечён, коржи смазаны кремом, розочки приклеены. Цена ошибки очень высока. И тут мы узнаём, что в другой пекарне в коржи добавляют соду, гашенную лимонным соком, для пышности. И что? Что делать? Переделывать?
Очевидно нет, так как: 1) мы всегда делали без соды, и результат был неплохим; 2) возможно, покупатели берут торты именно у нас, а не в другой пекарне, потому что им не нравится привкус соды; 3) торт уже готов, свадьба сегодня вечером, а новый торт испечь мы не успеем.
Тогда зачем это всё? Делиться опытом надо. Свежий взгляд нужен. Но, как мне кажется, на другом этапе. Например, перед тем как печь коржи, можно уточнить у новичка: «А как вы делали это раньше? А почему этот способ лучше?» И, наверное, стоит испечь пару тортов по-старому и по-новому, чтобы дать нашим постоянным клиентам попробовать и узнать их мнение.
Бездумное внедрение best practices, увиденных в статьях или докладах, может нанести вред компании и продукту. «Соду добавляют в коржи все крупные игроки: „Бугл“, „Трейсбук“, „Райл.ру“, „Юмдекс“. Все так делают». И что? Из-за этого Петя должен немедленно взяться за переделку задачи, которая уже готова?
Я уверен, что нет. Если изначально, перед решением задачи, вы не договаривались, что «в коржах будет сода», то очень недальновидно требовать её добавления на этапе ревью кода. У вашего бизнеса нет цели «иметь соду в коржах». Если ваши торты и так неплохо продаются, то совершенно неважно, есть в них сода или нет.
Нужно понимать, что обмен опытом — это другой процесс, никак не завязанный на ревью кода. Он может происходить раньше, позже или параллельно с ним, но он — другой. Можно устраивать взаимные мастер-классы с конкурентами. Некоторые пытаются выкрасть суперсекретный рецепт и суперсовременную методику взбивания крема перфоратором. Кто-то старается подсмотреть чужие идеи и усовершенствовать с их помощью свои процессы. Но странно делать это на работающем конвейере, на том этапе, когда ваш товар уже практически готов, а цена переделки очень высока.
Ведь мы говорим об этапе ревью. Ревью кода, который уже написан и готов к переходу на следующий этап. Этот код ждёт нажатия ревьюером кнопки «Code is OK». И ваши заказчики совсем не готовы к тому, что вы будете препарировать испечённый торт с новым поваром, чтобы показать ему, как вы печёте торты и выслушать его критические замечания.
Code review как знакомство с частью кода
Пожалуй, это выглядит как самая разумная и понятная часть, с которой многие согласны. Сценарий тут подразумевается такой: один программист написал код задачи, остальные на него посмотрели, изучили — и теперь знают, как с ним работать. Таким образом мы уменьшаем риск возникновения bus factor: если разработчик уйдёт, другие смогут успешно работать с его кодом. А также мы готовы к тому, что в следующий раз этот код может «потрогать» кто-то другой, и в этом случае он уже будет знать, как с ним работать.
Давайте подумаем, действительно ли это работает так, как задумано, и действительно ли помогает людям обмениваться компетенциями и знаниями о конкретных участках кода.
Посмотрим на ситуацию глазами того, кто написал код. В каждой ли задаче он применяет подходы и практики, которые обязательно должны усвоить все участники команды?
Допустим, он изобрёл какой-то крутой алгоритм сортировки (может, давно уже изобрёл и постоянно использует, а может быть, только что). Нужно ли, чтобы об этом знали все члены команды? Конечно, да. Нужно, чтобы люди узнали, что крутой программист Вася создал нереально быстрый и эффективный алгоритм сортировки, который будет полезен всем.
Однако, на мой взгляд, таких крутых вещей, которые непременно должны интегрироваться в общие правила и стандарты, не так уж много. И используются они далеко не в каждой задаче. Так стоит ли каждый раз останавливать весь конвейер, отрывать всех от работы и показывать им код всех задач Васи?
Очевидно, что во многих компаниях вся команда узнать о ноу-хау Васи на этапе ревью не может. Ведь ревью делает не вся команда, а какое-то количество ревьюеров. Если у вас это делают все, рано радоваться: с ростом команды это будет всё более и более неудобным, а в конечном итоге и невозможным. Этот процесс не масштабируется, и важно помнить, что временные ресурсы, которые можно тратить на ревью кода, ограниченны.
Теперь посмотрим на ситуацию со стороны других разработчиков, которые могли бы сделать ревью кода Васи и оценить его труд. Сидят ли они на своих местах без дела и ждут ли, пока Вася им покажет что-нибудь крутое в коде? Наверняка нет — они работают над своими задачами. У всех есть свои дела, и разработчик не всегда готов всё бросить и моментально переключиться на обсуждение достижений коллег, какими бы ошеломляющими они ни были.
И, наверное, разумно было бы, если бы Вася сообщил о своём изобретении всем как-то иначе. Сделал презентацию, например. Или просто написал письмо с куском кода и объяснениями, почему это круто, почему его алгоритм реально классный и почему он теперь должен стать стандартом для всех. Остальные его прочитают, каждый в своё время, зададут вопросы, согласятся или не согласятся с решением. И только после этого можно принимать или не принимать новый стандарт. Возможно, у кого-то окажется предложение лучше: более оптимальное, дешёвое, быстрее реализуемое.
Чувствуете? То, что я описываю, это не процесс ревью кода — это процесс создания и одобрения общих практик и подходов. Совершенно иной процесс.
И на этом этапе снова могут всплыть best practices от коллег и «В моей прошлой компании это делали по-другому» или «Я читал, что это делают иначе». И что? Best practices в общем случае, для сферической компании в вакууме, могут быть подходящими. Означает ли это, что все они будут полезны в вашей компании? Как минимум не всегда. И уж точно это не значит, что кто-то должен немедленно переписать свой код, на написание которого уже потрачено время.
Соответствие любым абстрактным или внешним best practices, гайдлайнам, модным тенденциям и т. д. — каким-то правилам, которые оторваны от компании/ контекста — ради самого соответствия никому не нужно. «Статические методы считаются плохим тоном», «Такое теперь надо выносить в трейты», «Синглтон — плохая практика, лучше использовать фабрику и DI». Кому надо? Кем считается?
Порой можно услышать здравые аргументы, но справедливые для абстрактных проектов и задач, а не для реальных. Даже полезные и нужные вещи обсуждать и рекомендовать на этапе, когда код уже написан, — поздно, потому что внедрение обойдётся неоправданно дорого.
Кроме того, в коде Васи может быть не новый крутой алгоритм или ноу-хау, а «костыль», грязный хак, который ему пришлось использовать по каким-то причинам. Действительно ли об этом должны знать все в команде? Если грязный хак оправдан и по-другому сделать было нельзя, то, наверное, да. Но даже в этом случае не стоит отвлекать всю команду, останавливая конвейер. Сомневаюсь, что и самому Васе эта идея понравилась бы.
Во-первых, мало кто готов «прямо сейчас» отвлечься и оценить «грязность» хака, ведь все заняты своими задачами. А во-вторых, никто ничего не запомнит. Да и люди в команде меняются, и, если кто-то в будущем наткнётся на этот хак, у человека так или иначе возникнет вопрос: почему сделано именно так? И оптимальным решением будет добавление комментария прямо в код. Комментария, объясняющего причину, которая вынудила прибегнуть к такому хаку (вспоминаем правила хорошего тона при комментировании кода: мы описываем в комментариях не что мы делаем в коде, а почему мы делаем именно так).
А если можно избежать использования этого грязного хака, то очевидно, что задачу стоит переоткрыть и заставить Васю переделать. Это прямое назначение ревью кода, и понятно, что к ознакомлению всей команды с кодом оно отношения не имеет (зачем всем знать о коде, который всё равно будет переписан?).
Третий случай — это когда в задаче ничего супернового и грязных хаков нет. Она базируется на всех принципах и правилах, принятых в команде, оформлена в соответствии с общими стандартами и т. д. Так зачем же тогда на неё отвлекаться всей команде? Ведь никакого bus factor нет, и, если кому-то другому придётся работать с этим участком кода в будущем, он без труда в нём разберётся.
Ну и, наконец, если мне всё ещё не удалось вас убедить, то у меня вопрос: как понять, что обмен знаниями в процессе ревью произошёл?
— Петя, ты прочёл код, который написал Вася?
— Прочёл.
— Всё понял?
— Всё понял.
Вот и весь процесс верификации. А где уверенность в том, что Петя всё понял правильно? Где уверенность в том, что, когда Петя в будущем будет изменять этот участок кода, он не наломает дров? Как измерить время, необходимое для погружения в задачу для разработчиков, один из которых код ранее не видел, а другой — пару раз делал ревью этого кода?
Более того, где уверенность в том, что в следующий раз работать с этим участком кода будет именно Петя, который делал ревью, а не Эдуард, который сейчас делает ревью задачи Арсения с совсем другим компонентом?
Таким образом, я утверждаю, что ознакомление с кодом чужих компонентов в процессе ревью кода работает далеко не так, как ожидается. Это только замедляет этап ревью кода и, как следствие, тормозит доставку фичи пользователю.
Если необходимо делиться знаниями между разными компонентами, то лучше подойти к этой задаче как к отдельной и самостоятельной. Следовать общим правилам для всей компании, использовать одинаковый технологический стек, делиться знаниями на внутренних митапах и семинарах, писать общую документацию (опять же по принятым стандартам и правилам), передавать задачи по соответствующим компонентам на реализацию коллегам из других компонентов, соблюдая все правила по адаптации новичков (рассказать о том, как принято в компании, что вы делаете, а что — нет, помочь составить хороший технический план и т. д.). Такое погружение в компонент гораздо полезнее для всех участников, нежели просто просмотр кода.
Ревью кода — это процесс верификации, контроля, процесс, который должен проходить максимально быстро. И примешивать к нему другие процессы (обучение новичков, обмен знаниями, демонстрация крутизны экспертизы и т. д.) просто неуместно. Это как минимум неоправданно дорого и — главное — не работает на данном этапе.
Code review как документация
Ещё один вариант ответа на вопрос «Зачем нужно ревью кода?» был про документирование. Тут может быть не совсем очевидно, что имеется в виду, поэтому поясню.
Сценарий подразумевается такой: разработчик и ревьюер кода о чём-то договорились в комментариях к ревью, и в будущем, если у кого-то возникнет вопрос о том, почему в данном месте сделано именно так, информацию об этом можно будет найти в ревью. То есть по git blame найти коммит, по коммиту найти тикет, по тикету найти ревью, в ревью найти обсуждение, изучить его и понять, почему сделано именно так.
Получается, что разработчик, у которого и так много источников информации — стандарты и соглашения, техническое задание, тикет в багтрекере, сам код (с комментариями или без) — должен лезть ещё в один. И читать весь тред, пытаясь уловить нить беседы и понять причины, побудившие сделать именно так. Причём мнения сторон в этом треде могут измениться несколько раз. Звучит странно, не правда ли?
Но вопросы действительно могут возникнуть. Что делать в этом случае?
Очевидно, что, если вопрос возник на этапе ревью, на него следует ответить. Но также нужно понимать, что если у ревьюера возник вопрос, то такой же вопрос может возникнуть и у людей, которые будут работать с кодом позже. Это может быть знаком того, что код написан в обход каких-то соглашений (и требуется пояснение, почему). В этом случае стоит оставить пояснение прямо в коде, например, в виде комментария со ссылкой на тикет — тогда любой человек, глядя на код, будет понимать, почему сделано именно так.
Если причин для игнорирования соглашений нет, то очевидно, что задачу необходимо вернуть на доработку и привести к требуемому виду. Стоит ли хранить для этого историю переписки и заставлять разработчиков в будущем всю эту историю перечитывать? Думаю, это лишнее.
Кроме того, может быть так, что код написан недостаточно понятно для ревьюера, хоть и соответствует правилам. В этом случае его следует переписать, чтобы для ревьера и будущих разработчиков он стал понятен. Опять же, нужно ли это хранить в истории для будущих поколений? Я в этом не уверен.
Даже если возникла ситуация, когда комментарии нужны и будущим поколениям стоит о них знать, в этом случае лучше оставлять их в коде или тикете — в тех источниках информации, которые у разработчика уже есть. Зачем вводить новый (тем более со всей историей переписки)?
В тикете можно написать «Решили сделать так-то по таким-то причинам» как итог всех обсуждений. Так и читать понадобится меньше, и много времени у интересующегося разработчика не отнимет. Ну и информацию при необходимости разработчик сможет получить.
Вывод напрашивается сам собой: использовать ревью кода как источник документации не стоит. Этот процесс нужен для другого.
Хорошее ревью кода
Список ошибочных вещей и процессов, которые люди имеют склонность добавлять в ревью кода, можно долго продолжать. И даже если бы я поставил перед собой цель собрать полный список, я бы не смог сделать его исчерпывающим. Ведь сколько людей, столько и мнений.
Я думаю, что в данном случае нужно иметь в виду тот факт, что смешивать несколько процессов в один — не всегда правильно. Особенно если речь идёт о процессах, продолжительность которых сложно планировать и контролировать.
Правильный процесс ревью кода — это процесс контроля. Контроля того, что задача выполнена в полном объёме. Контроля того, что соблюдены общие правила и договорённости. Контроля того, что решение не избыточно и что его легко поддерживать и развивать в будущем.
Следовательно, я бы выделил следующие моменты, которые обязательно должны быть проконтролированы на этапе ревью:
Архитектура и дизайн решения
Насколько предлагаемое решение соответствует задаче? Насколько оно сложно для понимания, поддержки, расширения? Насколько получившаяся архитектура требовательна к ресурсам и какова сложность реализованных алгоритмов? Можно ли было сделать проще, понятнее, дешевле? Сделано ли что-то лишнее, что не относится к задаче? Используем ли мы внешние зависимости и библиотеки? Насколько это оправданно? И так далее.
Не забываем, что это этап контроля. Соответственно, архитектура решения должна быть оговорена заранее. На данном этапе мы должны удостовериться, что договорённости соблюдены.
Также стоит иметь в виду, что это один из самых сложных, ответственных и трудоёмких моментов в ревью кода. Человек, выступающий в роли ревьюера, должен быть готов идти на компромиссы и помнить о том, что идеальных решений не бывает. И если он видит в задаче решение, которое по тем или иным причинам считает неидеальным, то он должен в первую очередь примерить его на нужды бизнеса, а не на свой перфекционизм.
Ревьюер может предложить свою реализацию, которую он считает более правильной с точки зрения best practices, паттернов и других вещей. Но тут очень важно понимать, что решение, которое он видит в коде, лучше его собственного уже одним лишь тем, что оно готово. Оно уже написано. Разработчик потратил своё время на реализацию, интеграцию этого куска кода с другими элементами, тестирование и написание автотестов и так далее. И исправлять код следует только в том случае, если решение заведомо неправильное.
Соблюдение правил и соглашений, принятых в команде
Все мы разные. У каждого свой взгляд на вещи, и зачастую можно обнаружить, что даже в слаженном и давно сработавшемся коллективе взгляды могут быть диаметрально противоположные. Кто-то считает, что табуляции лучше пробелов, а кто-то уверен в обратном.
Чтобы это не сказывалось на ежедневных задачах, чтобы любой участник команды мог как можно быстрее вникнуть в код и задачу другого, устанавливают командные соглашения. Начиная со стандартов оформления кода, структуры папок и файлов проекта и заканчивая формализацией API-методов и запретами на использование тех или иных конструкций языка. Таким образом обеспечивается и поддерживаемость кода, и уменьшение риска возникновения bus factor, и много что ещё.
Тут опять же важно помнить о том, что на этапе ревью мы контролируем соблюдение общих договорённостей. Учить на этом этапе стандартам кодирования уже поздно и может быть неоправданно дорого.
Корректность решения
Имеет ли решение какие-то недостатки? Опечатки в коде, магические числа и другие неопределённые константы. Магические строки и другие неожиданные данные, которые программист не учёл при решении задачи. Проверки на null и другие места, где потенциально можно «выстрелить себе в ногу». Проверка решения с точки зрения информационной безопасности и так далее.
Это очень важная часть ревью кода, и выполняться она должна очень тщательно и аккуратно. Сколько программистов у вас в команде знает, что это необходимо делать на этапе ревью? Сколько программистов это реально делает?
Тестируемость кода и наличие юнит-тестов
Данный этап проверок также очень важен, ведь он направлен на улучшение той самой пресловутой поддерживаемости кода.
Многие понимают, что означает термин code maintainability, но далеко не все могут объяснить, что это такое. А раз так, то как тогда ставить задачу команде на поддержание этой самой maintainability? Поэтому я считаю, что разработчик, подумавший о том, как его код будет тестироваться, а уж тем более тестирующий собственный код и написавший автотест для автоматизации тестирования, естественным образом будет стремиться к тому, чтобы его код удовлетворял большинству критериев code maintainability. Ведь без этого написать автотест практически невозможно.
Общей рекомендацией к любому изменению кода также может быть правильная декомпозиция задач. Чем меньше объём кода, проходящего через конвейер всех процессов от идеи до продакшена, тем легче этот код проверять на соответствия, тестировать, объединять с кодом других задач и выкладывать. Ревью кода — не исключение. Задачу с большим количеством изменённых строк во многих файлах очень тяжело читать и понимать (и удерживать в голове зависимости). Риск и цена исправления недочётов могут быть очень высокими.
Заключение
Вот, собственно, и всё. Эти четыре момента — это именно те самые вещи, которые необходимо проверять на этапе ревью. Их легко формализовать, легко донести до исполнителей, а значит, несложно и сформулировать критерии верификации и прогнозировать продолжительность. Автоматизация и другие способы минимизировать время ревью становятся более чем возможными.
И напоследок дам ещё несколько советов.
Важно помнить, что любой возврат кода на доработку не лучшим образом сказывается на качестве реализации. Даже если все действуют из лучших побуждений. В случае ревью кода это работает так: разработчик правит код, но тестирует его уже не так тщательно, как в первой итерации (зачем? Ведь он уже всё проверил, а изменил совсем чуть-чуть). Опыт показывает, что большинство таких ситуаций оборачивается багами именно в тех местах, где были правки по результатам ревью кода. Так работает человеческая психология.
В начале статьи я предлагал вам ответить на три вопроса про необходимость и правильность ревью. Теперь у вас есть алгоритм, который поможет найти ответы. Зная шаги контроля и учитывая размер задачи, вполне можно планировать время, необходимое на ревью кода, каждый раз стремясь его всё больше и больше сократить.
Внедряя любой процесс, важно помнить о целях, которые мы перед собой ставим, и о проблемах, которые хотим решить. Тогда будет гораздо проще отделять зёрна от плевел и формулировать измеряемые критерии оценки эффективности. А формулировать их нужно для себя самого и для своей команды, которая тоже остро нуждается в решении возникших проблем, но понимать их может по-своему.
Есть и ещё один важный момент: те процессы, правила и ценности, которые справедливы для одной компании, могут быть не столь полезными и работающими в другой. То, что я описал как примеры правильного ревью, работает в нашей компании. Мы пропагандируем быструю разработку, частые релизы и делаем ревью для каждой задачи. Подумайте о том, насколько это применимо к вашему проекту, и о том, что ревью — это один из тех процессов, которые пускать на самотёк нельзя.
Но решение, конечно, остаётся за вами.
Автор: uyga
