Главная » Хабрахабр » Code review: вы делаете это неправильно

Code review: вы делаете это неправильно

Сегодня очень многие в разработке используют ревью кода. Практика полезная, нужная. Даже если вы не делаете ревью, вы наверняка знаете, что это такое.

GitHub, Phabricator, FishEye/ Crucible, GitLab, Bitbucket, Upsource — список можно долго продолжать. На рынке есть куча инструментов для ревью кода с готовыми сценариями использования, рекомендациями и правилами. Мы в Badoo тоже в своё время с ними работали: в своей предыдущей статье  я рассказывал нашу историю ревью кода и о том, как мы пришли к изобретению собственного «велосипеда» — решения Codeisok.

д. Информации предостаточно, можно нагуглить кучу статей про ревью кода с реальными примерами, практиками, подходами, рассказывающих о том, как хорошо, как плохо, как нужно делать, а как — не нужно, что стоит учитывать, а что — нет, и т. В общем, тема «обсосана до косточек».

Именно поэтому другую часть айсберга можно и не заметить.

Ревью, как и любой другой процесс, требует внимательного и обдуманного внедрения, тогда как подход «Сделаем как у всех, лишь бы было» может не только не сработать, но даже навредить. Я надеюсь, что вы серьёзно отнесётесь к тем вещам, которые я опишу в этой статье, в некоторых случаях специально утрируя.

После прочтения этого вступления у вас может возникнуть вопрос: а что сложного в ревью кода? Дело-то плёвое. Тем более что большинство инструментов, перечисленных выше, сразу и флоу ревью предлагают (из коробки: поставил, закоммитил/ запушил — и вперёд!).

В том числе из-за мнимой простоты процесса. Но практика показывает, что всё не так очевидно, как кажется на первый взгляд. А они или будут следовать процессу, или займутся чем-то другим в ущерб решению проблем, которые процесс ревью призван решать. Когда всё налажено и работает, есть риск, что менеджер успокоится и перестанет погружаться в процесс, передав его разработчикам. В то время как для бизнеса очень важно решать задачи как можно быстрее, не тратя время на незапланированные активности. Менеджер может и не догадываться, что что-то идёт не так, потому что не даёт установок или не навязывает правила.

Это был p1lot. Когда-то давным-давно, когда я работал в другой компании, мне в память глубоко запала беседа о ревью кода с одним из ведущих разработчиков. Сейчас он работает с нами в Badoo, и это здорово. Возможно, кто-то из читателей знаком с ним (p1lot, привет :)).

На мой взгляд, эта мысль является ключевой. Так вот, PILOT тогда сказал: «Самое сложное для меня в процессе ревью — пойти на компромисс и пропустить готовую и корректно работающую задачу дальше, даже если я знаю другой способ её решения и даже если мой способ мне нравится больше». И основной посыл всей моей статьи так или иначе вертится вокруг этого постулата.

У вас в компании есть ревью? Правильно ли вы его делаете? У меня есть тест, который, возможно, заставит вас усомниться в этом.

Нужно ответить всего на три вопроса:

  1. Сколько времени занимает ревью кода для средней (сферической в вакууме) задачи?
  2. Как вы минимизируете время ревью?
  3. Как вы определяете, что ревью конкретной задачи сделано правильно?

Если у вас нет внятных ответов на некоторые (или на все) вопросы, если вы сомневаетесь в своих ответах, то осмелюсь предположить, что что-то идёт не так.

Возможна ли ситуация, когда исполнитель, который делал ревью четыре часа, не пил чай половину этого времени? Если вы не знаете, сколько времени потребуется на ревью, и не минимизируете его постоянно, то как вы осуществляете планирование? А, может, ревьюер вообще не смотрит код, а просто нажимает «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 и другие места, где потенциально можно «выстрелить себе в ногу». Проверка решения с точки зрения информационной безопасности и так далее.

Сколько программистов у вас в команде знает, что это необходимо делать на этапе ревью? Это очень важная часть ревью кода, и выполняться она должна очень тщательно и аккуратно. Сколько программистов это реально делает?

Тестируемость кода и наличие юнит-тестов

Данный этап проверок также очень важен, ведь он направлен на улучшение той самой пресловутой поддерживаемости кода.

А раз так, то как тогда ставить задачу команде на поддержание этой самой maintainability? Многие понимают, что означает термин code maintainability, но далеко не все могут объяснить, что это такое. Ведь без этого написать автотест практически невозможно. Поэтому я считаю, что разработчик, подумавший о том, как его код будет тестироваться, а уж тем более тестирующий собственный код и написавший автотест для автоматизации тестирования, естественным образом будет стремиться к тому, чтобы его код удовлетворял большинству критериев code maintainability.

Чем меньше объём кода, проходящего через конвейер всех процессов от идеи до продакшена, тем легче этот код проверять на соответствия, тестировать, объединять с кодом других задач и выкладывать. Общей рекомендацией к любому изменению кода также может быть правильная декомпозиция задач. Задачу с большим количеством изменённых строк во многих файлах очень тяжело читать и понимать (и удерживать в голове зависимости). Ревью кода — не исключение. Риск и цена исправления недочётов могут быть очень высокими.

Вот, собственно, и всё. Эти четыре момента — это именно те самые вещи, которые необходимо проверять на этапе ревью. Их легко формализовать, легко донести до исполнителей, а значит, несложно и сформулировать критерии верификации и прогнозировать продолжительность. Автоматизация и другие способы минимизировать время ревью становятся более чем возможными.

И напоследок дам ещё несколько советов.

Даже если все действуют из лучших побуждений. Важно помнить, что любой возврат кода на доработку не лучшим образом сказывается на качестве реализации. Ведь он уже всё проверил, а изменил совсем чуть-чуть). В случае ревью кода это работает так: разработчик правит код, но тестирует его уже не так тщательно, как в первой итерации (зачем? Так работает человеческая психология. Опыт показывает, что большинство таких ситуаций оборачивается багами именно в тех местах, где были правки по результатам ревью кода.

Теперь у вас есть алгоритм, который поможет найти ответы. В начале статьи я предлагал вам ответить на три вопроса про необходимость и правильность ревью. Зная шаги контроля и учитывая размер задачи, вполне можно планировать время, необходимое на ревью кода, каждый раз стремясь его всё больше и больше сократить.

Тогда будет гораздо проще отделять зёрна от плевел и формулировать измеряемые критерии оценки эффективности. Внедряя любой процесс, важно помнить о целях, которые мы перед собой ставим, и о проблемах, которые хотим решить. А формулировать их нужно для себя самого и для своей команды, которая тоже остро нуждается в решении возникших проблем, но понимать их может по-своему.

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

Но решение, конечно, остаётся за вами.


Оставить комментарий

Ваш email нигде не будет показан
Обязательные для заполнения поля помечены *

*

x

Ещё Hi-Tech Интересное!

Юнит тестирование скриптов баз данных

Принимая удобство в использовании юнит тестов на моем любимом С++, я попытался перенести свой опыт на TSQL, тем более что новый работодатель любит полезную инициативу на местах и раздает плюшки за оное. Просмотрел несколько известных фреймворкoв я пришел к выводу, ...

Теория счастья. Введение в мерфологию

Продолжаю знакомить читателей Хабра с главами из своей книжки «Теория счастья» с подзаголовком «Математические основы законов подлости». Это ещё не изданная научно-популярная книжка, очень неформально рассказывающая о том, как математика позволяет с новой степенью осознанности взглянуть на мир и жизнь ...