company_banner

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


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

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

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

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

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

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

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

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

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

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

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


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

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

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

    Заключение


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

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

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

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

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

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

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

    Badoo

    317,00

    Big Dating

    Поделиться публикацией
    Комментарии 84
      +8

      Я бы выделил две проблемы практики код-ревью:


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


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

      А все остальное решается инструментальными средствами: линтерами, анализаторами и т. д.

        0
        Спасибо за ваше мнение.
        +12
        Очень много букв про некую мнимую вещь. Есть правила изначальные и автоматизация всего что можно. Рефакторинг кода будет всегда, как бы вы не старались его избежать

        От Badoo хочется видеть более интересные статьи. В частности, когда планируется внедрение рекомендации на основе глубокого обучения (фото понравившийся людей) или прогресс тормозит коммерция? Вы должны тут быть одними из первых. Или у вас уже это есть?
          +3
          К сожалению, описанные в статье проблемы — не мнимые. Это личная практика. Как бы это странно или нереально ни выглядело.
          В моем случае масса проблем была как раз в том, чтобы установить «изначальные правила». В командах, которые уже думают что итак все знают и умеют. Отсюда и проблемы.

          По поводу машинного обучения — можем, умеем, практикуем. Обязательно напишем что-нибудь интересное. Спасибо что читаете наш блог.
            0
            мнимая вещь? если разработчикам не важна стабильность и поддерживаемость решения то да, можно сказать мнимая. Но если вы серьезно относитесь к своей работе — то нет. Линтеры вас не особо спасут. Например, бывало что люди изобретают свой vtable вместо использования виртуальных функций. Как такое отловит линтер?

            к слову, практически в любом более или менее успешном проекте код ревью очень затяжный процесс. Например, в сообществе разработчиков llvm: www.youtube.com/watch?v=jqAUxr-vDe0
              0
              Можно делать много всего, но через полгода это все будет уже совершенно другим. Наверняка такие крупные компании как Badoo это делают. У них есть на это ресурсы, и возможно это реально там нужно. Но для мелких коллективов и команд это не есть самое главное. Я лишь просто хотел бы что бы они написали интересные вещи по разработки. То, что могли бы посмотреть более мелкие команды и разработчики. А код ревью, какой у нас офис и как мы все замечательно проводим время это наверняка не самое интересное что они могут нам рассказать.
            +1

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

              +1
              Спасибо за ваше дополнение!
                0
                Не стоит путать новичка в проекте и новичка в программировании.
                  0
                  Мифическим зверем меня ещё никто не называл :)
                  +1
                  Тезис: ревью кода, написанного сотрудником, должен выполнять более опытный сотрудник. Верно ли это?

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

                  Если да, то получается, что чем опытнее сотрудник, тем больше на нём задач на ревью (миддл ревьюит за двумя жуниорами, синьор за тремя миддлами и избранные задачи за одним жуниором, лид за тремя синьорами, самые сложные задачи за двумя миддлами и двумя жуниорами...). Наиболее опытные сотрудники, которые могли бы решать задачи наиболее эффективно, будут заниматься ревью 8 часов в день 40 часов в неделю. А если самый крутой лид из любви к искусству всё-таки напишет несколько строк кода, то кто будет ревьюить их?
                    0
                    Тезис: ревью кода, написанного сотрудником, должен выполнять более опытный сотрудник. Верно ли это?


                    В моем случае было по-разному. Поэтому я и постарался описать ситуацию с разных сторон.

                    По моему скромному мнению, можно и так и эдак. Главное чтобы все понимали что делают и для чего это делается.
                      0
                      например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот код
                      Если джун правда разобрался и понял, как работает код лида — это прекрасно.
                        0
                        Да, но в процессах, когда задача считается не решённой, пока не завершён ревью — это будет означать, что задача решалась неделями (и клиент ждал неделями), хотя лид всё сделал за полдня.
                          0
                          Мы это решаем двумя способами:
                          1. Ограничиваем количество строк для П.Р.
                          2. Создаем WIP пул-реквесты, в которые докомичиваем. Всякие джуны могут их читать по мере того, как они появляются
                            0
                            А если фича — логически цельный кусок, выше количества макс. строк?
                              0
                              Обычно можно разбить её. Но всегда есть пункт 2
                          +13
                          Если джун не понял как работает код лида, то лид некомпетентный. Чем выше квалификация, тем менее загадочный код должен писать программист.
                            0
                            Главное, чтобы джун понял, как писать такой же код
                              +4
                              Про собственно код я согласен. Но, чтобы оценить код, надо понять постановку задачи. Для лида / синьора задача вполне может ставиться в бизнес-терминах, жуниору ещё предстоит научиться понимать все эти страшные слова (к написанию кода отношения не имеющие).
                                –1

                                "Загадочным" он может быть не только потому, что написан запутанно, но и потому, что джун не знаком с некоторыми идиомами. Свежий пример:


                                @computed get authorName() { this.article.author.name }

                                Что значит кеширует? А если имя поменялось? А если данные ещё не загружены с сервера? Как сделать асинхронный запрос? Почему нигде нет расстановки индикаторов загрузки? Как они вообще сами по себе появляются? А где обработка ошибок? Кто вообще рисует этот красный восклицательный знак? Да это криптокод какой-то!

                                  +1
                                  Откуда из этого кода (моделька на MobX, да?) вообще возьмутся идеи о кешировании, серверах, запросах, индикаторах, ошибках и восклицательных знаках? Можно не понимать, что значит декоратор @computed, но откуда такой поток идей от вида простого геттера?
                                    –2

                                    Ну как откуда. Заменяем this.article.author.name на "Alice" и вот уже никаких запросов, индикаторов загрузки, сообщений об ошибках и прочих "магических" вещах.

                                      +2
                                      Мы говорили про чтение кода, а не про исследование его путём наблюдения за сайд-эффектами при модификации.
                                        0
                                        Вот смотрит человек в код компонента, видит лишь пяток подобных геттеров, и недоумевает как же это всё работает без асик-эвэйтов, установки флага загрузки данных, навешивания обработчиков событий и прочих привычных вещей. «Да это криптокод какой-то!» — реальная цитата.
                                          0
                                          Так эвэйты, промисы или колбэки вместе с установкой флагов в коде присутствуют. И рендеринг по этим флагам тоже понятен и очевиден, если понятно как вообще он происходит. Единственное, что часто непонятно разработчику, столкнувшемуся с реактивным подходом в первый раз — это механизмы перерендеринга, как вызов обычного сеттера ил иного мутирующего метода, вызывает перерендеринг. И самая частая конкретная проблема: вызывает этот сеттер, а перерендеринга не проиходит. Причина обычно в том, что не знает нюансов мониторинга слежения за свойствами, например, что мониторится изменение значения ссылки на объект, а не изменение его свойств (мониторить можно и свойства, но вот в конкретном куске кода может быть неочевидно что конкретно мониторится).

                                          То есть «криптокод» — это не сам код, а механизм реакций на изменения состояния. Сам код как раз становится простым и понятым: установили флаг загрузки и начали запрос, по окончанию запроса записали данные и сбросили этот флаг. А в функции рендинга проверяем флаг — если установлен пишем «loading...», если не установлен, то выводим данные. Куда проще-то?
                                            0
                                            Так эвэйты, промисы или колбэки вместе с установкой флагов в коде присутствуют.

                                            Нет.


                                            То есть «криптокод» — это не сам код, а механизм реакций на изменения состояния.

                                            О чём я, собственно, и говорил — незнание идиом делает даже предельно простой код непонятным. Но это не проблема кода, это обычный пробел в знаниях, который надо заполнять.


                                            Куда проще-то?

                                            Без всего вот этого вот:


                                            установили флаг загрузки и начали запрос, по окончанию запроса записали данные и сбросили этот флаг. А в функции рендинга проверяем флаг — если установлен пишем «loading...»
                                              +1
                                              Если не присутствуют, то они кодом компонента не являются и для изучения работы компонента не нужны. MobX не предоставлять функциональности запросов к серверу, он лишь реактивное хранилище данных.

                                              > О чём я, собственно, и говорил — незнание идиом делает даже предельно простой код непонятным. Но это не проблема кода, это обычный пробел в знаниях, который надо заполнять.

                                              Да что в таком коде непонятного может быть? Вот файл с бизнес-логикой, вот файл с инфраструктурной логикой (запросы к серверу), вот файл с логикой отображения. Последние ссылаются на первый. Несколько аннотаций типа `@observable`, `@computed`, `@action` и `@observer` знающему человеку скажут о характере динамического взаимодействия, а незнающему не помешают разобраться что происходит в коде. Единственный непонятный момент для него останется, это как вызовы action или изменение `@observable` приводят к вызову `@observer` и всё. Объяснения «считай, что под капотом генерируются события изменения и обсервер на них подписывается» достаточно чтобы сделать общую картину работы компонента полной, если значения английских слов ни на какие мысли не навели. Но из полной картины можно человеку доверять довольно значительные изменения в коде, если они не должны изменять существующие реактивные связи.

                                              > Без всего вот этого вот:

                                              Это ваши слова. Я обычно не делаю флаг загрузки, а проверяю состояние промиса. А запрос делать надо по любому, есл взаимодействие с сервером требуется. Да, бывают, варианты, когда в коде или конфиге явно запроса не найти, когда даже урл формируется динамически по, например, имени класса. Но вот это как раз «криптокод».
                                                0
                                                Да что в таком коде непонятного может быть?

                                                Вы почему у меня всё это спрашиваете? :-) Я точно так же в недоумении. Ну, точнее, причина-то понятна. Человек не хочет разбираться в новых идиомах, поэтому ему проще обвинить код в "нечитаемости", что бы это ни значило.


                                                А запрос делать надо по любому, если взаимодействие с сервером требуется.

                                                Из компонента никаких запросов делать не надо — он сам сделается моделью по необходимости.


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

                                                И проверять руками ничего не надо.

                                                  0
                                                  Да не надо ему разбираться в этих идиомах если его задача, например, всегда выводить имя с заглавной буквы, как бы оно не хранилось. Или адаптировать парсинг тела ответа к новому формату серверного API. Разбираться надо только если ему нужно изменять, дополнять или удалять реактивные связи. Неочевидна для обычного программиста динамика взаимодействия классов, но не «статическая» логика получения данных или их отображения.

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

                                                  Что значит «не надо»? Не, ну можно сделать хелпер (собственно в экосистеме React+MobX уже не один есть), который будет в заисимости от стейта промиса выводить один из трёх компонентов. Но в коде (пускай и конфигоподобном) нужно указывать, что выводить на каждый стейт и проверка будет осуществляться.
                                                    0
                                                    Да не надо ничего указывать. «Обёртка» сама вставит по необходимости. Там же типовой код.
                                                      0
                                                      Какой типовой? Откуда она знает надо показать надпись loading..., загрузка, прогресс-бар или спиннер? И это простые варианты только для загрузки. Уж молчу про обработку ошибок хотя бы по HTTP-статус коду.
                                                        0
                                                        Откуда она знает надо показать надпись loading..., загрузка, прогресс-бар или спиннер?

                                                        А зачем вам неконсистентность в представлении индикаторов ожидания?


                                                        Уж молчу про обработку ошибок хотя бы по HTTP-статус коду.

                                                        Этим занимается http-адаптер. Зачем вьюшке знать про http-коды?

                                                          0
                                                          > А зачем вам неконсистентность в представлении индикаторов ожидания?

                                                          Он дизайнер, он так видит. И даже без неконсистенции очень хочется тот индикатор, который дизайнер нарисовал, а не какой-то системный дефолтный.

                                                          >Этим занимается http-адаптер. Зачем вьюшке знать про http-коды?

                                                          Вьюшке не надо, вьюшке надо знать какое по типу и содержанию сообщение выводить и, опционально, разные варианты продолжения работы показать. Решение какое сообщение показать будет принимать контроллер, получив данные из модели от http-адаптера по факту напрямую или примитивно смапленные типа 400 => 0, 404 => 1 и т. д., если уж мы говорим в терминах MVC. Ну или по типам исключений типа 400 ClientError 404 EntityNotFound и т. п…

                                                          Напомню, для меня компонент — это почти полноценное приложение, реализующее всю триаду MVC, а значит кроме вьюшки включает и модель (в которой будет http-адаптер), и контроллер, который будет принимать решение какие ошибки показывать и показывать ли их в ответ на http-ошибки (не важно, напрямую будет из модели они возвращаться, или простой маппинг будет на какие-то ошибки модели)
                                                            0
                                                            Он дизайнер, он так видит. И даже без неконсистенции очень хочется тот индикатор, который дизайнер нарисовал, а не какой-то системный дефолтный.

                                                            Так и воткните его по дефолту, незачем его в каждой вьюшке копипастить.


                                                            разные варианты продолжения работы показать

                                                            Можно пример?

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

                                                              Пример «Abort, Retry, Ignore?» — знакомо ?:)
                                                                0
                                                                Пример «Abort, Retry, Ignore?» — знакомо ?:)

                                                                Опять же незачем это писать в каждой вьюшке. Всё это должно переиспользоваться по умолчанию.

                                                                  0
                                                                  Ну хоть в одной же надо написать. Это не должно быть внутренней магией фреймворка или библиотеки. Особенно, когда единого варианта нет и быть не может.
                                                                0
                                                                Так и воткните его по дефолту, незачем его в каждой вьюшке копипастить.
                                                                А если в разных местах разные индикаторы?
                                                                  0
                                                                  Сейчас расскажет как переопределяется.
                                                                    0
                                                                    А если в разных местах разные индикаторы?

                                                                    А зачем вам неконсистентность в представлении индикаторов ожидания?

                                                                    :)

                                                                      0
                                                                      Вы словно никогда с дизайнерами не работали. Это слишком сложное слово для них.
                                      0
                                      код может быть непонятен читающему в силу нескольких факторов, и квалификация написавшего всего лишь один из них. Абстрактный джун может быть недостаточно знаком с используемыми ЯП/фреймворками/библиотеками/паттернами, терминологией и матчастью предметной области и т.д.
                                    +10
                                    например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот код

                                    В 90% случаев это значит, что лида надо увольнять или переводить в менеджеры (если есть задатки или принцип Питера давит). В 10% — что должен был ревьюить миддл.
                                    Если ведущий специалист пишет код, который никто не понимает, то куда же он ведет продукт? Мне в командах нужны лиды, которые могут сделать всякие ответственные штуки, например, самая ответственная — публичный API или общая схема взаимодействия блоков. Такой хороший код не сложен для понимания, а прост. Сложный код — не гордость опытного разработчика, а позор или признание какой-то слабости.
                                    Для большинства бизнес-приложений это так. Есть исключения: одноразовый код, иногда high-concurrency, всякие интринсики или асм-вставки, код для экзотических железяк, всякая криптография и суровая математика. Ну да, есть иногда куски для объяснения которых нужен опыт выше джуна. Каждый такой кусок — повод задуматься "а не фигню ли мы делаем".

                                      +3
                                      А я думал наоборот, чем опытнее разработчик, тем проще код.
                                        +1

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

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

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

                                      можно ли было баги, внедренные после доработки, обнаружить на второй итерации кодревью? может быть второе ревью было поверхностным? что если его усилить чек-листами, гайдлайнами, автоматизированными проверками
                                        +1
                                        Скорее всего, если в команде часты случаи появления багов из-за правок по результатам ревью (равно как и по результатам тестирования, как самим разработчиком, юнит-тестами и ручным, так и штатными QA) из-за психологических особенностей процессов «полирования» по сравнению с основной задачей, то и повторное ревью имеет те же особенности. И они именно психологические. Если можно усилить процесс повторного ревью автоматизированными проверками или ещё чем, то усиливать надо ревью вообще.

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

                                        Психологически правки не воспринимаются как отдельная задача, требующая столь же серьёзного отношения как и любые другие на всех этапах, как минимум: постановка задачи, планирование решения, собственно решение, тестирование, ревью и доставка. Часто некоторых этапов вообще нет или решение о необходимости такого этапа принимается ситуационно на основании субъективных метрик типа «много ли переписано строк после прошлого ревью/тестирования».

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

                                          0
                                          читая Вас, VolCh, я слышу вот что:
                                          — у нас бывает код плохого качества
                                          — ну это изза психологии, да и делать качественно оказывается дорого.
                                          — аа, хорошо, оставим всё как есть

                                          вы говорите, проблема психологического характера, а решения не предлагаете. то что вы делаете — это попытка объяснить происходящее и примириться с ним. вообще это один из способов решения проблем. пусть так.
                                          я же на это смотрю так: вы можете пожертовать качеством, чтобы сделать быстрее/дешевле, пойти не полному процессу производства софта, а срезать углы. бородатая тактика управления содержанием проекта.
                                          может быть те дефекты, которые появляются изза доработок, не влияют на бизнес, бизнес модель их демпфирует без последствий для продаж и клиентов. ну так и вообще всё замечательно в этом случае — не нужны тяжеловесные процессы, можно больше сфокусироваться на фичах и меньше на тест-активностях.
                                            0
                                            Я именно что объясняю свое видение причин появления подобных проблем, но не предалагаю ни мириться, ни бороться. Хотя и предлагаю один из способов борьбы, если считаете, что бороться надо и у вас проблемы именного того характера, которые этот способ устраняет. Мало кто со стороны сможет оценить целесообразность борьбы и, тем более, эффективность конкретного способа в конкретных условиях.

                                        +3
                                        Но давайте начистоту: не поздновато ли? Не будет ли более правильным рассказать новичку об этих правилах до допуска его к коду? Ведь цена ошибки очень высока — редко новички сразу работают так, как вам нужно. А значит, задача будет возвращаться и возвращаться на доработку. А значит, продукт станет доступен пользователю позже, чем мог бы.


                                        Это как waterfall против agile.

                                        I'd say that the ultimate goals of code review are:
                                        * decrease number of bugs since two pair of eyes usually better that one pair of eyes.
                                        * improve code quality since 2 opinions usually better than one opinion.

                                        Everything else are secondary things.
                                          +4
                                          Квинтэссенция, лучше не скажешь. В статье на мой взгляд излишне много дартаньянства и местечковой заточки. И команды, и проекты в разных компаниях бывают очень разные. И код ревью в них служит разным целям. И на часто повторяющийся вопрос «а не поздновато ли» значительная часть таких команд с чистой совестью ответит — нет.
                                            0
                                            Скорее «лучше поздно чем никогда» :)
                                              –3
                                              В статье на мой взгляд излишне много дартаньянства

                                              Вы так говорите, как будто это что-то плохое :)

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

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

                                            На что только не идете в Badoo, чтобы оправдать свой велосипед) Воды налили!
                                            Просто в процессе ревью не надо быть ботаном и помнить о цели проекта и качестве выполнения рассматриваемой задачи.

                                              0
                                              Просто в процессе ревью не надо быть ботаном и помнить о цели проекта и качестве выполнения рассматриваемой задачи.

                                              Золотые слова. Как этого добиваться на постоянной основе, не поделитесь?
                                                +2
                                                Кадры решают все. Я думаю вы прекрасно это понимаете и собственно ради того и ведете блог на хабре чтобы иметь возможность выбрать правильных людей к себе в команду.
                                                  –2
                                                  Явно не бюрократией и зарегулированными процессами. Команда должна решать сообща, при учёте мнения своих наиболее опытных коллег. Люди и их взаимодействие важнее чем процессы и инструменты, говорится в Аджайл манифесте неспроста ;-)
                                                0
                                                В целом ревью работают хорошо у нас в команде. Есть и ограничение по времени и разделение на «вопросы» и «требования по изменению». Есть правила по оформлению кода, и стандартные практики. У всех в команде есть право вето, часть может давать добро на «принятие» кода.
                                                Смотря на новичков и их прогресс с точки зрения написания более понятного кода, я могу сказать, что ревью в самом деле работают на улучшение качества кода и на обучение сотрудников.
                                                Единственная настоящая сложность это пограничные случаи, когда проверяющие блокируют изменения по незначительным с точки зрения изменяющего поводам. Т.е. когда эмоции начинают брать верх. Такие моменты наносят ощутимый урон команде с точки зрения духа товарищества, который, на мой взгляд, должен присутствовать. А вы что думаете по этому поводу?
                                                  0
                                                  А сколько у вас таких пограничных случаев и как часто они возникают? Если есть урон «духу товарищества», то как вы это исправляете?
                                                  Поделитесь, очень интересно.
                                                    0
                                                    Возникают нечасто — реже, чем раз в месяц. Собственно поэтому каждый раз разбираемся по факту. Бывает, что просто обсуждаем, признаем то или иное мнение правильным, вносим, при необходимости, изменения в процедуры и/или стандарты, и закрываем проблему. Бывает, что пытаемся найти компромис. Пока неразрешимых проблем не было.
                                                    С точки зрения «товарищества», то помимо собственно работы проводим социальные мероприятия, всякие встречи вне работы, во время работы на обеде, и тому подобное.
                                                    0
                                                    Единственная настоящая сложность это пограничные случаи, когда проверяющие блокируют изменения по незначительным с точки зрения изменяющего поводам

                                                    Скажу как человек, у которого есть такой ревьюер. Все время кажется, что для него это своеобразный спорт — найти то, до чего можно докопаться и не аппрувить ревью до тех пор, пока не будет так, как он хочет. Это очень сильно демотивирует. Начиная даже интересную задачу, всегда помню что скоро открывать реквест и он снова будет меня нервировать. Для примера, последний случай: для материализации коллекции из одного элемента (это 99% работы, самый максимум не более 10) он заставил меня поменять .ToList() на .ToArray() (это C#). Причина была: сэкономить память. Фактически экономии там несколько байт и у нас не хай-лоад.
                                                      0
                                                      К сожалению, слишком часто ревью кода из инструмента помощи автору кода сделать хорошо превращается в инструмент реализации комплекса вахтёра.
                                                        –2
                                                        С другой стороны, а чем-то был обусловлен выбор именно .ToList()? Я, например, если не вижу обоснований использованного варианта реализации, но даже без глубокого погружения в задачу вижу другие альтернативы, более экономные по ресурсам, то прошу обосновать. Если обоснование вида «да какая разница по списку итерироваться или по массиву?», то настаиваю на более экономном варианте, если его читаемость не ниже.
                                                          +1
                                                          Однообразием. И здравомыслием. Экономить пару байт на этой операции нет никакого смысла, рядом тяжелые апи-вызовы бегают туда-сюда. Как раз для вас в начале статьи цитата: «Самое сложное для меня в процессе ревью — пойти на компромисс и пропустить готовую и корректно работающую задачу дальше, даже если я знаю другой способ её решения и даже если мой способ мне нравится больше».
                                                            0
                                                            Однообразие — важный и объективный момент. Это реальное обоснование, если в «146%» случаев используются списки, а не массивы. Здравомыслие — субъективно.
                                                              –1
                                                              Ох, не понимаю я этого стремления ввернуть ToList. Скажем, в древние времена, когда ToArray пользовался древним ArrayBuffer'ом и вызывал аллокации чуть ли не на каждый элемент, в отличие от List<>, который это делал логарифм раз, я еще понимаю. Но в нашем современном мире ToArray делает то же самое, что ToList, только действительно тратит меньше памяти и вдобавок ограничивает в действиях с коллекцией — я бы тоже на ревью спросил «а почему тут используется лист? ты собираешься добавлять туда еще что-то или убирать?» Потому что как правило эти ToList'ы просто нужны для неотложенного итерирования.

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

                                                              Опять же, зависит от обстоятельств: может быть это метод типа
                                                              public IReadonlyList<MyClass> Filter(IQueryable<MyClass> someData)
                                                              {
                                                                  return someData.Where(x => x.IsValid).ToList();
                                                              }

                                                              тогда по сути все равно — что ToList, что ToArray.
                                                        +3
                                                        Ревью кода настолько обобщенная вещь, что его единственная цель — это решать ту задачу, ради которой оно было введено.
                                                        Если оно введено для галочки и отлично эту галочку закрывает — то так тому и быть.
                                                        Если введено для того, чтобы никто не мог реализовать свои индивидуальные порывы по улучшению продакшена, и ты теперь знаешь, что твои «улучшения» дальше твоего рабочего места не уйдут — то быть такому ревью.
                                                        Чтобы перекрыть кислород расторопным менеджерам, умело подбивающих разработчиков пропихнуть в рамках других задачи их личные хотелки — почему нет?
                                                        Если в команде два с половиной человека, и ревью — это отличная возможность для них обмениваться знаниями и опытом в подходах к решению поставленных перед ними задач — что в этом плохого?

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

                                                        Да, для чего бы ни использовался какой-угодно инструмент, ретроспективно всегда можно указать, что это не самое лучшее применение. Но если это что-то живое (а не зацементированный ритуал), то оно будет со временем эволюционировать вместе с задачами, которые необходимо будет решать. И пока есть осознание, что если мы этот инструмент уберем, то старая проблема вернется, — мы все делаем правильно.
                                                          0
                                                          В целом согласен, но итоговый вывод не факт. Если есть осознание, что ревью — это далеко не самый лучший инструмент для решения старой проблемы, а может и новой какой-то, которой не было, когда его вводили (и проект живой, и процесс ревью живой) или вводили не думая, что он решит и эту проблему, то, как минимум, стоит подумать о том, чтобы снять с ревью часть его задач, доверив их решение более подходящим инструментам типа линтеров, автоматически снимаемых метрик кода или тестов.

                                                          Грубо говоря, микроскопом и гвозди забивать, и возбудителей болезней искать в крови — это вряд ли правильно, за исключением каких-то особых случаев. При том не просто гвозди забивать было бы эффективней молотком или строительным пистолетом, а и качество поиска микробов страдает от того, что гвозди забивают.
                                                          +3
                                                          Большая просьба автору писать чуть покороче. Времени и так нет. Это не претензия, просто пожелание для будущих статей.
                                                            0
                                                            Доля правды есть, но слишком категорично утверждается, что ревью должно решать единственную задачу как практически последний рубеж обороны от разработчика. Провокация?

                                                            Ревью, как часть процесса разработки и доставки, может решать множество задач в рамках конкретной компании/проекта/продукта. И это не значит, что это неправильно. Может для каких-то задач есть более эффективные в теории инструменты, но, например, стартовые, постоянные и(или) переменные издержки в них неприемлемы здесь и сейчас.

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

                                                            Если вы, владельцы бизнес-процесса ревью и бизнес-процессов вообще в вашей компании, считают, что у ревью единственная бизнес-цель, которую оно эффективнее всего решает, а другие цели должны достигаться с помощью более эффективных для них инструментов, то, да, неправильно использовать ревью для других целей. Но если столь редкого единодушия нет, то неправильно говорить «вы делаете это неправильно». Навскидку, например, у компании может не быть возможности внедрять более эффективные инструменты обучения и коучинга (каждый раз когда думаю, что уже знаю, что такое коучинг, узнаю его новые значения) или внедрить автоматические инструменты проверки подавляющего большинства правил форматирования кода.

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

                                                              У меня и моих супервизоров всю жизнь в роли программиста ревью как-то был инструментом именно оценки решения задачи. Так как до попадания кода (я писал на js) в репу посредством гит хуков была введена цепочка eslint (в конфиге airbnb) → prettier → юнит тесты → commitlint, а при попытке открыть пул реквест в основную dev-ветку из feature-веток прогонялись е2е тесты в селениуме, то код, попадающий на ревью был уже сравнительно чист, отформатирован и нормально работал — и по моим ощущениям в основном проверялся на некое "качество исполнения" — отметались разнообразные велосипеды, грязные хаки, недокументированные / непонятные куски кода, и прочее подобное гадство (обычно с комментариями, но тут уже как повезёт — попадались и оригиналы с синдромом вахтёра, заворачивающие код с ужасающими пометками а-ля "ЧТО ЭТО ВООБЩЕ ТАКОЕ?" или просто "deny").


                                                              Не скажу что всё вышеописанное было вот прямо как-то очень круто или правильно, но удобно было явно. Нет, были конечно неприятные случая когда люди уставали смотреть на ошибки линтера и коммитили с --force, но потом народ перешёл на гитлаб и поставил pre-recieve-хук.


                                                              Как-то так.

                                                                0
                                                                Спасибо что делитесь вашим опытом!
                                                                0
                                                                Итого вам нужно быть уверенным что:
                                                                решение описанное в коде на ревью, оптимальное
                                                                полностью протестировано
                                                                новички с ним ознакомились
                                                                свежие идеи пошарены в команде
                                                                демо новой фичи
                                                                и т.д и т.п…
                                                                Не слишком ли много для ревью? Процесс итак занимает кучу времени, скучен и однообразен.

                                                                С другой стороны Pair-programming, mob programming помогают справиться с практически всеми вопросами которые люди пытаются взвалить на код ревью. Тут вам и нахождение оптимального решения, шаринг, обучение, тест покрытие… В итоге ревью превращается в обычную формальность которая может быть автоматизирована.
                                                                  +1
                                                                  Перечитайте мою статью еще раз, пожалуйста. Там немного не про то.
                                                                  0
                                                                  Не совсем понятно что за «Эти четыре момента — это именно те...» имеются ввиду в заключении. Выше дюжина пунктов, какие из них — те самые 4?
                                                                  Дополнительное перечисление их в заключении было бы полезно для понимания.
                                                                    0
                                                                    Эти 4 момента выделены в подзаголовки:
                                                                    1) Архитектура и дизайн решения
                                                                    2) Соблюдение правил и соглашений, принятых в команде
                                                                    3) Корректность решения
                                                                    4) Тестируемость кода и наличие юнит-тестов
                                                                    –1
                                                                    Не очень понял, о чем статья, сорри:), но при код-ревью я лично руководствуюсь следующими принципами:
                                                                    • реализуется ли полностью фича Y в данном код ревью X
                                                                    • нет ли ошибок? проверены ли граничные условия? есть ли юнит-тесты? покрывают ли они задачу?
                                                                    • не будет ли проседания по скорости, если одобрить X?
                                                                    • если ответы «да», «да», «да», то про некоторые места спрашиваю почему здесь поменялось то-то и то-то (хотя я сам понял почему поменялось или думаю, что понял)
                                                                    • если автор бодро и резво отвечает, значит он знает, что делает, и я апрувлю pull request

                                                                    Да, и крупных переделок у нас не бывает после code review, только косметика, потому что все детали реализации всех крупных изменений обсуждаются заранее, а мелочь и так люди обычно делают правильно.
                                                                      +6
                                                                      На самом деле надо признать, что в code review есть два класса задач, каждая из которых может решаться, а может и игнорироваться.

                                                                      1. Проверка качества кода. «А зачем ты тут так странно делаешь, когда есть strange.do_it()?» У тебя тут сортировка вручную написана. Вот этот код будет работать, но его совершенно невозможно понять. Раздели в этом месте код не несколько независимых функций. Переменная tmp3 недостаточно информативная. Etc. Фактически, это эквивалент редакторской правки для статьи или книги.

                                                                      2. Проверка на соответствие кода архитектуре проекта. Это может делаться только тем, кто эту архитектуру знает. Предпочительно — PTL, или самый старший из программистов. Он не спрашивает про сортировку пузырьком, он спрашивает «зачем ты тут используешь старый паттерн, который мы усиленно выпиливаем?», «не трогай объект Foo, т.к. он под сильным рефакторингом прямо сейчас, лучше используй вот это и это».

                                                                      Совершенно разные задачи, совершенно разные комментарии. Первое может делать любой более-менее освоившийся программист, второе — только человек глубоко в проекте.
                                                                        0
                                                                        Насчёт обучения не соглашусь. Когда это отдельный процесс, не понятно, чему обучать, можно недообучать, или можно залезть в такие дебри, которые в процессе работы и близко не нужны. При ревью же рано или поздно джун наткнется на всё, причем только на то, что надо. Тут-то ему и объяснят, как делать, как не делать.
                                                                        Наверное оптимально выстроить отдельный процесс, где разобрать самые частые кейсы за немного времени. Остальное оставить на ревью. Но этот процесс еще надо выстроить, если времени и желания этим заниматься нет, то ревью очень даже катит.
                                                                          +1
                                                                          Огромное спасибо за статью! Планируете ли перевод на английский?

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

                                                                        Самое читаемое