Python conf in Kharkiv, Nov 16 with Intel, Elastic engineering leaders. Prices go up 21.10

«Метод наводчика» при работе с пул реквестами

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

Ложная слепота

Помните тест, в котором нужно было сосчитать количество передач баскетбольного мяча? Если вы не видели, то посмотрите, прежде чем читать дальше.

В 2004 году был сформулирован принцип «ложной слепоты», и этот термин объединяет в себе несколько разнообразных феноменов нашего восприятия. Обычно выделяют два вида ложной слепоты: слепоту невнимания и слепоту к изменениям. Слепота невнимания (перцептивная слепота) подразумевает неготовность нашего мозга воспринимать изменения, которые мы не ожидаем увидеть. Как гориллу в ролике, которую уже окрестили «невидимой гориллой» и даже сайт отдельный для неё сделали. Еще причиной называют рассеянность, которая вызванная необходимостью полностью сконцентрировать внимание. Очевидно, что на себе вы этот феномен можете почувствовать, только если о нем не знаете. И действительно, при повторном эксперименте с другими футболистами и совершенно другой макакой вы ее обязательно заметите, так как ожидаете ее увидеть.

Слепоту к изменениям тоже можно заметить гораздо проще, есть довольно много роликов в интернете на эту тему. Даже National Geographic снял передачу, посвященную этому феномену. В обычных условиях изменение наблюдаемой картины мгновенно обращает на себя внимание. Однако мы можем не замечать изменения, если они совпадают с коротким прерыванием наблюдаемой картинки. Это может быть кратковременное моргание экрана, монтажный стык видео или короткая помеха. В экспериментах по слепоте к изменениям обнаружилось, что почти 70% испытуемых не замечали подмены главного действующего лица, а изменения менее существенных деталей оставались проигнорированными почти в 100% случаев.

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

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

Подходы к код-ревью

Есть два разных подхода. Конечно, их больше, но речь о нормальных подходах, а не превращающих ревью в бюрократию с профанацией.

Первый — это открыть пул реквест и промотать его сверху вниз (или в каком-то другом разумном порядке), тщательно его прочитав, что-то выполнив в уме, прочитав тесты и убедившись, что они проверяют код. Потом взять в левую руку Фаулера, в правую — Макконнела и указать на возможные косяки, плохие имена, «тут выделите класс», «заинлайните метод», «тут же N+1 у вас», «хватит мокать тесты», «это не REST», «вы забыли авторизацию», «не надо писать собственный метод, он в стандартной библиотеке уже есть» и всякое такое. Ну, или сказать «молодец». В общем, побыть сильно продвинутым линтером и анализатором кода.

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

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

Метод наводчика

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

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

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

В дополнение отмечу, что новичкам лучше обучаться на практике, и очень желательно, чтобы практика была своей, а не чужой. Еще учиться лучше всего на ошибках, правда ведь? Метод наводчика предполагает, что наводчиком может быть и молодой разработчик, а опытный будет исполнителем. В этом случае за молодым будет окончательное решение, но это не значит, что опытный должен выключить мозги и тюкать по клавишам, нет. У опытного будет очень сложная задача давать советы, аккуратно возражать и помогать таким образом, чтобы решение действительно было принято молодым наводчиком, а не опытным бойцом. Но с точки зрения опытного, решение должно быть принято правильное, само собой. В методе наводчика это немаловажный бонус, который получается как бы сам собой. Джуны учатся быстрее, правильнее и на своих действиях, а не на чужих. Да и забористые сеньоры тоже учатся правильно обучать молодняк.

Выводы

Резюмируя, вот проблемы, которые решаются методом наводчика:

  1. Ревьюверам нет дела до пул реквестов соседей по парте, пока этот пул реквест к ним не относится.
  2. Отвлекаться от текущей задачи не хочется вообще, поэтому ревью пул реквестов становится обузой.
  3. Ревьювер не может быть слишком строгим, потому как отношения к нему, как к человеку, могут стать хуже.

Проблема сводится к тому, чтобы максимально вовлечь ревьювера в работу этого пул реквеста. И под словом «вовлечь» подразумевается:

  1. Дать ревьюверу необходимое чувство ответственности за пул реквест.
  2. Показать исполнителю, что ревьювер — это не формальность или бюрократия, а напарник, который поможет избежать ошибок.
  3. Наладить правильный, конструктивный диалог между этими двумя.
LinkedIn

28 комментариев

Подписаться на комментарииОтписаться от комментариев Комментарии могут оставлять только пользователи с подтвержденными аккаунтами.

Использование Сонара и прочих тулзов статик анализа сокращает в разы время на код ревью. Нет варнингов — пропускаем. Если это не твой код с которым тебе предстоит работать, вообще пох что он там делает.

Не утонуть в коде и ревью помогает качественная разбивка на небольшие и максимально независимые подзадачи. Если задача сложная/не бьется, саме время подключать парное программирование.
В обсуждении и разбивке должна участвовать вся команда, чтобы не терять контекст.

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

Ревьюверам нет дела до пул реквестов соседей по парте, пока этот пул реквест к ним не относится.

ИМХО, это — нормальная ситуация. Ибо точно точно так же линейным разработчикам в Вашем методе людям «нет дела» до чужих задач, пока они выполняют свои (если это не лид и выше).

Отвлекаться от текущей задачи не хочется вообще, поэтому ревью пул реквестов становится обузой.

ОК, берём те же условия:
1. У человека уже есть задача, над которой он работает.
2. а) ему дают code reivew
2. б) ему дают быть «командиром» или «исполнителем»
При этом варианту 2. б) он обрадуется более, чем обычному ревью? Очень сомнительно. Потому что уровень вовлечения иной (выше), а в данном случае выше — значит хуже и ведёт к бОльшему недовольству, т.к. нужно сильнее углубляться в контекст и отрываться от текущей задачи. По сути вместо одной задачи на имплементацию становится 2 если не полноценных, то близко к тому — вот уж повод для радости...)
Учитывая данный нюанс, не указан очень существенный минус такого подхода — он дороже обычного ревью. Чтобы люди нормально это воспринимали, им нужно банально больше времени.

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

Если seniority level ревьюера выше, то ему более чем достаточно указать на фикс и аргумент, почему так лучше в обычном ревью. Джуниоры в этом случае аналогично учатся на своих ошибках (и так, ИМХО, даже эффективнее) — каким они образом учатся «на чужих», если сами всё делали? Если кто-то делает ревью а-ля «сделай в точности вот так» без аргументации — это проблема конкретного ревьюера, не ревью как такового, т.е. не является поводом вводить новые методы и т.д.

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

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

Конечно, справедливо заметить, что обсуждение стратегии должно предшествовать изменениям в коде

Проблема решается очень просто, и незачем усложнять и удорожать процесс.

В итоге, что остаётся? Фактически, качество кода senior-level специалиста. Который без труда пофиксит другой senior-level специалист, если там что-то будет. А если ничего (или почти ничего) не будет, так и слава Богу, что там уровень статистической погрешности.

Поэтому, ИМХО, процесс больше напоминает борьбу с ветряными мельницами, чем что-то действительно полезное...

Идея понятна, но есть одна проблема: вырождение управления. Если командир скажет что-то в духе «ну тут слева присобачь фигню нужной формы», не спускаясь до деталей? Формально всё правильно — но на самом деле никакой разницы от «обычного порошка» уже не будет.

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

В «высоком» opensource типа linux kernel это реализуется подробными сообщениями коммитов и «cover letters» для полной цепочки. В заказной разработке может быть другой метод, лишь бы максимально сохранялась история (те же тикеты).

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

У задачи должен назначаться не один исполнитель, а два.

Зарплату тоже на двоих делить? Хотя, 99% командиру, а 1% исполнителю — вполне нормально выходит.

Мсье изобрел парное программирование.

Мсье изобрел парное программирование.

Ніт.
При парном программировании решения принимают оба, нет того «кто делает» и того «кто командует». При ПП гибкость выше, ибо в любой момент можно изменить как тактические решения, так и стратегические. Меньше риском в плане отношений в команде, ибо нет того что стоит выше (того кто принимает решение). При ПП, тот кто не пишет код в данный момент, все равно обучается.

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

Прям вживую себе представил, стоят два артиллериста на огневой, рассуждают тактически и стратегически(!) и как жаахнут — бо гаубицЫЩА такого калибра, что один выстрел все решает. Ну хотя в «Урфине Джюсе и его деревянных солдатах» так и было — пушка Чарли Блэка решила исход генерального сражения (а следовательно и всей кампании) одним выстрелом.

Там пушка же горящим мусором стреляла? Наверное, самое лучшее сравнение со среднестатистическим качеством кода.

Ну, а по делу, ниже предложили «метод штурмана», что подходит куда лучше, чем наводчик и гаубицы.

Как-то это выглядит что вы сами придумали себе проблем и пытаетесь их решить.

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

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

Те проблемы что я прочитал в статье, уже давно решены:
— Парное программирование. Довольно похоже на то что вы описали, но при этом применимо для людей с равными «тайтлами». Так же имеет 100500 преимуществ если сравнивать с код ревью.

— Если уж не хотим парное программирование, то на ревью надо:
1) доверится тому кто выполныл задачу (даже если это джун);
2) попытаться ответить на вопрос «готов ли я взятся за поддержку этого кода?»
2.1) Если ответ да, то не важно что там писал Фаулер.
2.2) Если ответ негативный, то
2.2.1) попытаться сформулировать список проблем которы мешают лично вам
2.2.2) Обсудить их с исполнителем
2.2.3) Если не договорились пойти к «медиатору», задача которого выбрать 1 из 2 мнений. Решение медиатора обсудается во время медиации, но принимается обеими сторонами после.
При этом надо понимать, что если мы идем по пути «код ревь», то все члены команды должны учится на как можно раньших этапах выносить на обсудения те решения, в которых они не уверены.
Тут основная задача — это минимизация вероятности наступления «длинного пути».

1) доверится тому кто выполныл задачу (даже если это джун);

В описанном случае довериться нужно тому, кто будет проверять. Даже если это джун.

2.2.2) Обсудить их с исполнителем

И обсудить это нужно будет с тем, кто будет проверять.

2.2.3) Если не договорились пойти к «медиатору», задача которого выбрать 1 из 2 мнений.

Ну да, и медиатора еще нет. Каждый раз медиатором будет выступать тот, кто проверяет. И да, решение медиатора обсуждается во время медиации, но принимается обеими сторонами после.

Все. Кроме этих трех исправлений, вы описали почти то, что должно быть.

В описанном случае довериться нужно тому, кто будет проверять. Даже если это джун.

Стратегические решения принимает человек в квалификаций ниже исполнителя, ок.
Проблема у вас в слова «проверять»: ревьювер не проверяет, ревьювер просматривает код. Потому что в случае с проверкой джун не может ревьювить синьора, например.

И обсудить это нужно будет с тем, кто будет проверять.

А если кто-то еще будет не согласен с решением? Над кодом вполне могут работать более 2 человек.

Каждый раз медиатором будет выступать тот, кто проверяет.

Одна из сторон конфликта будет медиатором, гениально :)

---

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

В описанном случае довериться нужно тому, кто будет проверять. Даже если это джун.

що у випадку, якщо виконавець не згідний з рішенням, робити чз силу?

Классная идея, только терминология, на мой взгляд не совсем удачная. Я бы вместо «наводчик» использовал термин «ведущий» (facilitator). Хотя, это конечно же дело вкуса.

Над названием думал долго, наводчик показался более-менее удачным. А ведущий как ведущий телепрограммы? «Метод Якубовича»? Предлагайте название

Русский или украинский термин тут, на мой взгляд, вообще подобрать сложно. Нет общепринятой терминологии. Лучше всего подходит (как мне кажется) английский термин facilitator. www.urbandictionary.com/...​fine.php?term=facilitator Переводиться может в зависимости от контекста как «ведущий» или как «посредник». Идея (опять же, это мой взгляд на вещи, не обязательно я прав) в том, что фасилитатор (я пока так буду называть) не обязательно более опытный и не обязательно автор идеи. Но его ответственность в том, чтобы разобрать идею по запчастям, рассмотреть все возможные варианты архитектуры и выбрать лучший (с помощью исполнителя конечно), а потом помочь убедиться, что реализован именно выбранный вариант, причём реализован правильно и оптимально. «To facilitate» можно перевести как «содействовать», «облегчать», «продвигать». Термин «наводчик» вроде содержит нужный контекст, но он слишком уж, на мой вкус, военный. Если «посредник» и «ведущий» не нравится, тогда может быть «штурман». Мне кажется, «штурман» более точно отражает смысл роли.

«Штурман» очень даже нравится. Пожалуй, этот метод должен бы называться «методом штурмана». Так и назову его в англоязычном переводе, спасибо.

Есть один интересный нюанс.
В схема с 2мя исполнителями получается скорее ’дизайн ревью’. Ревью неких решений, архитектурных и не только. То есть из пост фактум ревью пулл реквестов мы так плавно перешли к ревью дизайна Перед началом исполнения.
Это действительно полезно. Вот только собственно сам пулл реквест теперь ревьювить не очень надо. Потому что если что-то пойдет не так, то принимающий опять будет ревьювить дизайн. И потом снова имплементация. И так по кругу.

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

Так это абсолютно нормальная ситуация — такой себе микро-agile на одну задачу.

Поинт в том, что по сути нужно не пост фактум ревью сделанного кода, а ревью решения/дизайна Перед началом имплементации.
И да, это будет микро agile.

а ревью решения/дизайна Перед началом имплементации.

Да, я рядом более развёрнуто описал то же самое.

И да, это будет микро agile.

Ничего страшного, оно вполне рекурсивно :)

Нужно и то, и то. Просто автор темы сделал акцент на вторую часть, хотя первая обычно даже важнее.

воу, интересная идея. можно это забабахать на английском где-то на medium.com? принял бы посильное участие в расшаривании.

В процессе. Как статья переведется, напишу сюда ссылку.

можно это забабахать на английском где-то на medium.com?

А это так обязательно использовать ресурсы за paywallʼом, а тем более рекомендовать их? Мало других площадок?

не обязательно
рекомендуйте другое

Да тысячи их. Главное — потом ссылку кинуть на reddit :)

Спасибо за статью

Подписаться на комментарии