Антипаттерны Code Review

Введение

Code Review кажется отличной идеей, верно? Два разработчика, просматривающие один и тот же код, имеют в два раза больше шансов обнаружить проблемы. Это распространяет понимание того, как развивается проект. Рецензент может узнать полезные приемы, подробно читая код автора, или найти возможность научить автора полезному приему, который он еще не знал.

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

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

Антипаттерны

Смерть от тысячи изменений

Вы начинаете читать код. Как только вы видите недочеты, вы добавляете комментарий в Code Review, указывая на них. Затем вы прекращаете чтение.

Разработчик добросовестно исправляет ваше замечание и выкладывает исправленный патч.

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

И так далее. И так до тех пор, пока разработчик не потеряет надежду.

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

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

Записка о выкупе

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

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

Если вы хотите снова увидеть свой любимый код…

Двойная команда

Один патч. Два рецензента.

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

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

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

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

Игра в угадайку

Есть проблема и множество возможных способов ее решения. Разработчик выбрал один из них и представил патч.

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

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

Так что теперь вы можете завалить и его, причем таким же бесполезным способом!

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

Инверсия приоритетов

При первом просмотре кода выделите мелкие и простые недочеты. Имена переменных немного непонятны, в комментариях есть опечатки.

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

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

Опоздание с пересмотром дизайна

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

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

Но теперь вас попросили проанализировать незначительную, но важную вещь, например, простое исправление ошибки, которая мешает многим разработчикам. Это ваш шанс!

Потребуйте обосновать весь план работы на данный момент. Если разработчик не знает ответов, потому что работает над одной маленькой частью общей работы, пожмите плечами — это не ваша проблема, и вы не нажмете кнопку «Одобрить», пока не будете удовлетворены.

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

Уловка-22

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

С другой стороны, если есть много маленьких патчей, то некоторые из них сами по себе не имеют смысла! Так что настаивайте на том, чтобы их снова склеили.

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

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

Сальто-мортале

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

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

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

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

Если серьезно, друзья…

До сих пор эта статья была шуткой.

(Я надеюсь, что мне вообще не придется говорить об этом, но я уверен, что кто-нибудь поймет меня неправильно).

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

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

Авторитет

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

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

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

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

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

Ограждение Code Review

До сих пор я уделял особое внимание рецензированию кода. Рецензент кода и автор кода — коллеги; ни один из них не отвечает за другого; ни один из них не имеет окончательного права голоса над кодовой базой в целом. Вот почему полномочия, которые вы получаете в ходе рецензирования кода, временны: завтра, после того как этот патч вольется в базу, у вас их уже не будет. А послезавтра все может быть наоборот.

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

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

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

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

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

Но даже в контексте контроля за выполнением требований невежливо разворачивать «Игру в угадайку» без объяснения причин. Когда я так поступаю, я обычно пытаюсь объяснить, что разработчик упустил из виду сложную часть, и если я сам не знаю ответа, я так и скажу.

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

Отказ от ответственности

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

Так что эта статья не направлена на какого-то конкретного человека, который недавно рецензировал мой код!

Источник