Это вторая часть статьи о том, как правильно общаться и избежать ошибок в процессе код-ревью. Здесь мы поговорим о том, как довести ревью до конца и избежать неприятных конфликтов.
Основы изложены в первой части, так что рекомендую начать с неё. Но если не терпится, вот её краткое содержание: хороший рецензент не только ищет баги, но и обеспечивает добросовестную обратную связь, чтобы помочь коллеге повысить свой уровень.
Моё худшее код-ревью
Худшее код-ревью в моей жизни было для бывшей коллеги, назовём её Мэллори. Она начала работать в компании за несколько лет до меня, но только недавно перешла в мой отдел.
Ревью
Когда Мэллори прислала для ревью первый список изменений, код был довольно сырой. Она никогда раньше не писала на Python и никогда не разрабатывала систему поверх неуклюжей легаси-системы, которую мне приходилось поддерживать.
Я добросовестно задокументировал все проблемы, которые сумел найти, всего 59 штук. Если верить прочитанной литературе по код-ревью, я проделал отличную работу: нашёл ТАК много ошибок. Мне действительно было чем гордиться.
Через несколько дней Мэллори прислала новый список изменений и ответ на мои заметки. Она исправила самые простые ошибки: опечатки, названия переменных и т.д. Но отказалась решать проблемы высокого уровня, такие как неопределённое поведение кода для искажённых входных данных или то, что структуры потока управления в одной из её функций находились на шестом уровне вложенности. Вместо этого она печально пояснила, что исправление этих проблем не стоит времени разработки.
Рассерженный и раздосадованный, я выслал новые замечания. Мой тон был профессиональным, но переходил в область пассивно-агрессивного: «Можешь объяснить, зачем нам неопределённое поведение для искажённых входных данных?» Как несложно догадаться, Мэллори проявляла всё большее упрямство.
Цикл ожесточения
Был вторник, прошла уже неделя. Я с Мэллори по-прежнему обменивались письмами насчёт того же код-ревью. Я отправил ей свои последние замечания вечером предыдущего дня. Я специально дождался, пока она уйдёт с работы, потому что не хотел находиться с ней в одной комнате, когда она получит письмо.
Всё утро я чувствовал физическую тяжесть в желудке, потому что боялся следующего раунда ревью. Когда вернулся с обеда, то увидел, что Мэллори нет на рабочем месте, но от неё пришли новые изменения. Я догадался, что она тоже не хочет присутствовать в комнате, когда я их получу.
Сердце всё сильнее стучало в груди по мере увеличения моего возмущения с каждым её ответом. Я немедленно начал барабанить по клавиатуре опровержения, указывая на то, что она не приняла никакие из моих предлагаемых изменений, и в то же время не предоставила никаких оправданий, чтобы можно было одобрить её код.
Этот ритуал повторялся каждый день в течение трёх недель. Код практически не изменился.
Вмешательство
К счастью, наш самый опытный коллега Боб разорвал этот круг. Он вернулся из длительного отпуска и обнаружил, что мы ожесточённо швыряем друг другу заметки по код-ревью. Боб немедленно оценил ситуацию как тупиковую. Он попросил перенести ревью на него, и мы оба согласились.
Боб начал своё ревью с того, что попросил Мэллори составить новые списки изменений, выделив две маленькие библиотеки, о которых мы вообще не спорили, каждая на 30-50 строк. Когда Мэллори сделала это, Боб немедленно одобрил изменения.
Потом он вернулся к основному списку изменений, который сократился примерно до 200 строк кода. Он сделал несколько небольших замечаний, которые Мэллори исправила. Затем одобрил список изменений.
Всё ревью Боба завершилось за два дня.
Коммуникация имеет значение
Вы могли уже догадаться, что конфликт на самом деле возник не из-за кода. Там действительно имелись проблемы, но их явно мог решить коллега, который обладал навыками эффективной коммуникации.
Это был неприятный опыт, но я рад вспоминать о нём. Он заставил меня пересмотреть свой подход к код-ревью и определить направления для улучшения.
Ниже я поделюсь некоторыми техниками, которые помогут вам избежать таких неприятных ситуаций. Позже я вернусь к Мэллори и объясню, почему мой изначальный подход имел обратный эффект и почему подход Боба оказался абсолютно блестящим.
Техники
9. Стремитесь повысить уровень качества кода только на одну-две ступени
10. Ограничьте фидбек по повторяющимся примерам
11. Уважайте область ревью
12. Ищите возможность разбить большие ревью
13. Искренне хвалите
14. Утверждайте ревью, когда остались тривиальные правки
15. Заранее избегайте тупиковых ситуаций
Стремитесь повысить уровень качества кода только на одну-две ступени
Хотя в теории у вашего коллеги может быть желание исправить абсолютно все недочёты в коде, но его терпение не безгранично. Если вы станете тянуть с одобрением раунд за раундом, у него быстро появится раздражение, потому что у вас возникают всё новые и новые блестящие идеи, как улучшить код.
Я про себя ставлю коду оценку, как в школе, от A до F [от шестёрки до кола в американской системе образования — прим. пер]. Если получаю список изменений, который соответствует оценке D (3), то пытаюсь поднять автора до оценки C или B-. Не идеально, но достаточно хорошо.
В теории возможно повысить уровень качества с D до A+, но это может потребовать до восьми раундов ревью. К концу такой работы автор вас возненавидит и никогда больше не захочет присылать вам код.
Вы можете подумать: «Если я одобрю код уровня C, не приведёт ли это к тому, что вся кодовая база будет уровня C?» К счастью, такого не произойдёт. Я пришёл к выводу, что когда помогаю коллеге подняться с уровня D до C, следующий список изменений от него уже начинается с уровня C. Через несколько месяцев он начинает присылать код, который начинается с B, и переходит на уровень A к последним код-ревью.
Оценка F предназначена для кода, который либо функционально некорректен, либо настолько запутан, что вы не можете быть уверены в его корректности. Единственная причина задержать одобрение — это если код остаётся на уровне F после нескольких раундов ревью. См. главу о тупиках ниже.
Ограничьте фидбек по повторяющимся примерам
Если вы заметили, что у автора несколько раз повторяются однотипные ошибки, не нужно помечать каждый случай. Не стоит тратить время на написание одного и того же 25 раз, и автор тоже не захочет читать 25 повторяющихся замечаний.
Нормально будет назвать два или три отдельных случая ошибок этого типа. Для всего остального просто попросите автора исправить все ошибки этого типа, а не перечисляйте каждый конкретный случай.
Уважайте область ревью
Существует пример неправильных действий, когда рецензент обнаруживает что-то рядом с кодом из списка изменений — и просит автора исправить. Когда тот удовлетворяет просьбу, рецензент обычно находит, что код стал лучше, но он теперь непоследователен, так что нужно произвести ещё несколько незначительных изменений. А потом ещё несколько. Это продолжается и продолжается, пока конкретный лаконичный список изменений не разрастётся, вобрав в себя кучу посторонних вещей.
Если у ваших дверей появится голодный мышонок, вы захотите дать ему печенье. И если дадите, он попросит стакан молока. Он захочет посмотреть в зеркало, чтобы проверить, что у него не остались усы от молока, а затем попросит ножницы, чтобы постричься…
— Лора Джоффе Нумеров, «Если дать мышонку печенье»
Практическое правило: если изменение не из области ревью, то оно не подлежит разбору.
Вот пример:
Даже если вы всю ночь не можете заснуть и мучаетесь из-за магического числа и нелепого названия переменной в коде, это не подлежит разбору. Даже если автор — тот же самый человек, который написал соседние строчки кода, это изменение по-прежнему не подлежит разбору. Если это вопиющий случай, зарегистрируйте баг или предложите собственное исправление, но не заставляйте автора разбираться с этим изменением в процессе код-ревью.
Исключением будет только тот случай, если изменение относится к изменяемому коду, хотя и не входит в него, например:
В этом случае обратите внимание, что автору нужно переименовать функцию с ValidateAndSerialize на просто Serialize. Это не влияет на сигнатуру функции, но всё равно та становится некорректной с таким названием.
Я слегка нарушаю это правило, если в коде немного исправлений и я замечаю какую-то простую правку, пусть и не подлежащую обязательному разбору. В таких случаях я делаю примечание, что автор может проигнорировать замечание, если хочет.
Ищите возможность разбить большие ревью
Если вы получаете список изменений из более чем ~400 строк кода, попросите автора разбить его на более мелкие части. Чем больше строк выходит за пределы этого лимита, тем труднее подготовить ответ. Лично я отказываюсь рассматривать списки изменений более чем на 1000 строк.
Автор может ворчать по поводу разбиения списка изменений, потому что это нудная задача. Облегчите его бремя, предложив логичные границы разделения. Самый простой случай — когда список изменений затрагивает несколько отдельных файлов. Здесь список изменений можно просто разделить между файлами. В более трудных случаях определите функции или классы на самом низком уровне абстракции. Попросите автора перенести их в отдельный список изменений, а после его одобрения вернитесь к остальному коду.
Если код низкого качества, решительно запросите разделение списка изменений. Сложность анализа плохого кода экспоненциально растёт с размером. Вам гораздо проще проводить аудит пары небрежных списков изменений по 300 строк, чем одной большой мерзости на 600 строк.
Искренне хвалите
Большинство рецензентов фокусируются на ошибках в коде, но код-ревью — это великолепная возможность поощрить правильное поведение.
Например, вы рассматриваете код от автора, у которого проблемы с написанием документации — и вдруг натыкаетесь на чёткий, лаконичный комментарий к функции. Скажите автору о его успехе. Он будет быстрее прогрессировать, если вы будете сообщать о правильных действиях, а не просто ждать того случая, когда он облажается — чтобы сообщить ему об этом.
Не нужно иметь какую-то конкретную цель, чтобы похвалить человека. Каждый раз, когда я вижу в списке изменений что-нибудь, что меня радует, я говорю автору об этом:
- «Не знал об это API. Действительно полезная вещь!»
- «Элегантное решение. Никогда о таком не думал»
- Разбить эту функцию было отличной идеей. Так намного проще»
Если автор — начинающий разработчик или недавно присоединился к команде, то может нервничать или занять оборонительную позицию во время ревью. Искренние комплименты частично снимают это напряжение — вы демонстрируете, что являетесь коллегой, который готов помочь, а не враждебным охранником кода с синдромом вахтёра.
Утверждайте ревью, когда остались тривиальные правки
У некоторых рецензентов есть неправильная установка, что они должны тянуть с одобрением ревью до тех пор, пока не увидят исправления каждой ошибки. Это добавляет несколько ненужных раундов ревью, которые впустую тратят время и автора, и рецензента.
Одобряйте код, если верно любое из следующих утверждений:
- У вас больше нет замечаний.
- У вас остались только тривиальные замечания.
- Например, переименование переменной, исправление опечатки.
- Оставшиеся изменения необязательны к исправлению.
- Явно укажите их как необязательные, чтобы у коллеги не возникло впечатление, что от них зависит ваше одобрение.
Я видел, как рецензенты тянут с одобрением, потому что автор не поставил точку в конце комментария. Пожалуйста, не нужно так делать. Так вы указываете автору, что он неспособен поставить простой знак пунктуации без вашего надзора.
Существует некоторая опасность в том, чтобы одобрять код, когда остались нерешённые вопросы. По моей оценке, примерно в 5% случаев автор или неправильно интерпретирует замечания последнего раунда, или полностью упустит их. Чтобы избежать таких ситуаций, я просто проверяю правки, внесённые автором после одобрения. В редких случаях недопонимания я или сообщаю автору, или создаю собственный список изменения с исправлением. Небольшое количество работы в 5% случаев лучше, чем ненужные усилия и задержка в 95% остальных случаев.
Заранее избегайте тупиковых ситуаций
Самый худший результат код-ревью — это возникновение тупиковой ситуации: вы отказываетесь ставить подпись без дополнительных изменений, а автор отказывается их делать.
Вот некоторые признаки того, что вы зашли в тупик:
- Тон обсуждения становится напряжённым или враждебным.
- Объём важных замечаний на каждом раунде не уменьшается.
- Вы сталкиваетесь с противодействием необычно большому количеству своих замечаний.
Поговорите с человеком
Поговорите с человеком в видеочате. Из-за текстового общения легко забыть, что с той стороны живой человек. Слишком легко представить своего коллегу как некое порождение упрямства или некомпетентности. Живая беседа разрушает это неверное впечатление и для вас, и для автора.
Предложите дизайн-ревью
Сварливое код-ревью может указывать на то, что какие-то проблемы не предусмотрели заранее в процессе разработки. Может вы спорите о вещах, которые стоило обсуждать во время дизайн-ревью? Оно вообще было?
Если причина несогласия лежит в высокоуровневом выборе архитектуры проекта, то решать проблему должен более широкий круг людей, а не просто два человека, которые оказались втянуты в это обсуждение кода. Поговорите с автором о том, чтобы расширить обсуждение на всех остальных участников группы разработки в виде дизайн-ревью.
Уступка или эскалация
Чем дольше вы с коллегой топчетесь в тупике, тем больше вреда наносите своим отношениям. Если альтернативы вас не устраивают, то остаётся только два варианта: уступить или обострить.
Оцените, во что выльется одобрение изменений. Вы не можете создавать качественные программы, если постоянно одобряете низкокачественный код, но вы также не сможете добиться высокого качества, если настолько ожесточённо воюете с коллегой, что больше не можете вместе работать. Насколько велик вред, если вы в реальности одобрите список изменений? Этот код способен повредить критически важные данные? Или это фоновый процесс, где в самом худшем случае грозит сбой задачи и необходимость отладки? Если второй вариант более точно описывает ситуацию, то подумайте о том, чтобы уступить и сохранить возможность дальнейшего сотрудничества с коллегой в нормальных условиях.
Если уступить не вариант, поговорите с автором насчёт обсуждения с менеджером группы или техническим руководителем. Предложите назначить другого рецензента. Если эскалация конфликта вам невыгодна, согласитесь с решением и двигайтесь дальше. Продолжение спора приведёт к возникновению плохой ситуации, где вы выставите себя в непрофессиональном свете.
Восстановление после тупика
Сумбурные аргументы в процессе обсуждения не столько относятся к коду, сколько к отношениям между людьми. Если вы попали в тупик или близки к нему, такая ситуация может повториться, пока не будет решён основополагающий конфликт.
- Обсудите ситуацию с менеджером.
- Если конфликт возник в вашей команде, менеджер должен знать о нём. Может быть, с этим автором просто сложно работать. Возможно, вы влияете на ситуацию неким образом, какой сами не понимаете. Хороший менеджер поможет вам обоим решить эти проблемы.
- Отдохните друг от друга.
- Если возможно, постарайтесь не проводить код-ревью друг у друга несколько недель, пока страсти не утихнут.
- Изучите способы разрешения конфликтов.
- Мне показалась полезной книга «Важные разговоры». Её советы могут показаться очевидными, но есть огромная ценность в анализе своего поведения в конфликтной ситуации со стороны, когда вы не находитесь в пылу спора.
Моё худшее код-ревью: работа над ошибками
Помните код-ревью с Мэллори? Почему моё стало трёхнедельным мучением с пассивно-агрессивной жижей, а Боб пронёсся ветерком за пару дней?
В чём я ошибся
Это было первое ревью для Мэллори в нашей группе. Я не подумал, что она может чувствовать предвзятое отношение или занять оборонительную позицию. Мне следовало начать только с высокоуровневых пометок, чтобы не хоронить её под грудой замечаний.
Мне следовало больше постараться, чтобы показать: моя задача не препятствовать её работе, а скорее помочь. Я бы мог предоставить примеры кода или отметить положительные моменты в её списке изменений.
Я позволил своему эго повлиять на ревью. Я потратил целый год, восстанавливая эту старую систему до работоспособного состояния. Внезапно появляется новый человек, который с ней возится, и она не хочет всерьёз принимать мои замечания? Я воспринял это как оскорбление, но такое отношение стало контрпродуктивным. Мне следовало сохранить объективное отношение, которое я пытаюсь принести во все свои ревью.
В конце концов, я слишком долго позволил сохраняться тупиковой ситуации. После нескольких раундов следовало понять, что дальнейший прогресс отсутствует. Я должен был радикально изменить ситуацию, например, встретиться лично для разрешения более глубокого конфликта или перенести обсуждение на уровень менеджера.
Что Боб сделал правильно
То, что Боб первым делом разбил ревью на части, оказалось очень эффективным. Вспомните, что ревью застопорилось на три долгие недели страданий. И тут внезапно он одобрил два фрагмента кода. Это придало позитивных эмоций и Мэллори, и Бобу, потому что дело сдвинулось с мёртвой точки, наметился прогресс. В остальном коде по-прежнему остались проблемы, но список изменений уменьшился и стал проще в работе.
Боб не пытался довести код до совершенства. Вероятно, он нашёл те же проблемы, о которых кричал я, но он учёл, что Мэллори недавно начала работать в нашей команде. Благодаря проявленной в данный момент гибкости он поможет Мэллори улучшить качество кода в долгосрочной перспективе.
Вывод
После публикации первой части статьи некоторые читатели высказали критику относительно стиля общения, который я рекомендовал. Некоторые назвали его покровительственным. Другие беспокоились, что он не слишком прямой и возникает риск недопонимания.
Такие отзывы разумны и их можно было ожидать. Один считает, что краткие комментарии выглядят резкими и грубыми. Другой может назвать их лаконичными и эффективными.
Осуществляя аудит кода, вы принимаете много решений: на чём сконцентрироваться, как оформить отзыв, когда одобрять результат. Не обязательно принимать те варианты, которые я предлагаю. Просто имейте в виду, что есть варианты.
Никто не даст вам рецепт идеального ревью. Наиболее эффективные техники в каждом конкретном случае зависят от личности автора, ваших взаимоотношений и корпоративной культуры. Оттачивайте свой подход, критически размышляя о результатах своего ревью. Когда возникает напряжённость, возьмите паузу и оцените происходящее. Уделяйте внимание качеству своих замечаний и предложений. Если вы не можете довести код до своих стандартов качества, подумайте, какие помехи есть в процессе ревью — и как их устранить.
Удачи, и пусть ваши код-ревью будут человеческими.
Дополнительная литература
Д-р Карл Вигерс — единственный из встреченных мной авторов, кто уделил должное внимание социальным факторам в код-ревью. Он хорошо подытожил свои тезисы в статье «Очеловечивание экспертиз». Написанная в 2002 году, она сохраняет свою актуальность, демонстрируя долгосрочную ценность эффективного общения.
Автор: m1rko
