Термин “code review” может означать разные действия, от простого чтения какого-то кода из-за спины разработчика до совещания на 20 человек, где вы разбираете код строчка за строчкой. Я здесь имею в виду формальную и письменную процедуру, но не отягощённую рядом совещаний для инспекции кода.

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

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

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

Ревью завершается, когда рецензент одобряет изменения. Обычно этому сопутствует отправка сообщения LGTM, сокращённой фразы “looks good to me”.

Почему это так трудно?

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

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

Филип Гринспан, сооснователь ArsDigita, из книги «Основатели за работой».

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

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

Техники

Отдайте компьютерам скучную часть работы

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

Очевидный пример — ошибки с пробелами в отступах. Сравните, сколько усилий требует выявление такой ошибки от человека, по сравнению с применением автоматического инструмента форматирования:

Правая часть пустая, потому что автор использовал редактор кода, который автоматически форматирует пробелы каждый раз при нажатии кнопки «Сохранить». Ну худой конец, когда автор отправляет свой код на проверку, система непрерывной интеграции сообщает о неправильных пробелах. Автор исправляет проблему ещё до того, как рецензент её заметил.

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

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

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

Нужно всем вместе поработать, чтобы внедрить автоматические проверки такого рода в рабочий процесс code review (например, хуки перед коммитами в Git или вебхуки в GitHub). Если процесс ревью требует от автора запускать такие проверки вручную, то вы лишаетесь большей части преимуществ. Автор неизбежно иногда будет забывать о проверке, из-за чего вам придётся возиться с с простыми ошибками, которые могла исправить автоматическая проверка.

Оформите аргументы по стилю в виде руководства по стилю

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

Хорошее руководство по стилю определяет не только внешние элементы оформления вроде соглашения о присвоении имён или правила отступов, но и как использовать функции данного языка программирования. Например, JavaScript и Perl набиты функциональностью — там есть много вариантов реализации одной и той же логики. Руководство по стилю определяет Один Правильный Способ программирования, так что вы больше не увидите, что одна половина вашей команды использует один набор функций языка, а другая половина — совершенно другой набор функций.

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

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

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

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

Я предпочитаю держать наше руководство в формате Markdown в системе контроля версия (например, на GitHub Pages). Так все изменения проходят через стандартную процедуру рецензирования — кто-то должен явно одобрить изменения, а каждый в команде может поднять любую проблему. Вики и Google Docs тоже подходят.

Вариант 3: Гибридный подход
Сочетая варианты 1 и 2, вы можете адаптировать существующее руководство и одновременно вести локальный документ для расширения или перезаписи базы. Хороший пример — руководство Chromium C++. Там в качестве базы взяли руководство по C++ от Google, но сделали собственные изменения и дополнения.

Начинайте ревью немедленно

Расценивайте code reviews как задачу с высоким приоритетом. Когда вы изучаете код или пишете отзыв, не торопитесь, но начинайте делать это немедленно — в идеале, в течение нескольких минут.

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

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

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

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

Начните с высокого уровня, и спускайтесь ниже

Чем больше заметок вы пишете в каждом конкретном раунде, тем больше риск, что автор будет чувствовать себя подавленным. Точный лимит зависит от разработчика, но опасная зона обычно начинается в районе 20-50 заметок на один раунд ревью.

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

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

Щедро используйте примеры кода

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

Хороший способ улучшить восприятие код ревью автором — найти возможность подарить ему подарок. А какие подарки любят получать все разработчики? Конечно, примеры кода.

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

Предположим, ваш автор не очень хорошо знаком с функцией списковых включений в Python. Он присылает на рецензию код с такими строчками:

urls = []
for path in paths:
  url = 'https://'
  url += domain
  url += path
  urls.append(url)

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

Автор будет гораздо счастливее получить такую заметку:

Предлагаю рассмотреть списковое включение такого рода:

 

urls = ['https://' + domain + path for path in paths]

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

Применяйте эту технику только для ясных, бесспорных улучшений. В вышеприведённом примере спискового включения немногие разработчики будут спорить с сокращением количества строк кода на 83%. И наоборот, если вы напишете пространный пример для демонстрации изменения, которое «лучше» на ваш личный вкус (например, изменения стиля), то такой пример кода создаст впечатление, что вы настырно проталкиваете свои предпочтения, а не проявляете щедрость.

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

Никогда не говорите «ты»

Это звучит немного странно, но послушайте: никогда не обращайтесь лично к автору в процессе код ревью.

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

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

Например, вот безобидный комментарий:

Ты допустил ошибку в слове «благополучно».

Автор может интерпретировать такое замечание двумя соврешенно разными способами:

• Интерпретация 1: Эй, приятель! Ты неправильно написал слово «благополучно». Но я всё равно считаю, что ты умный! Вероятно, это просто опечатка.
• Интерпретация 2: Ты допустил ошибку в слове «благополучно», придурок.

Сравните это с замечанием, в котором отсутствует обращение «ты»:

sucessfully -> successfully

Вторая заметка — это просто исправление, а не оценка автора.

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

Вариант 1: Замените «ты» на «мы»

Ты можешь дать этой переменной более наглядное имя, вроде seconds_remaining?

превращается в:

Мы можем дать этой переменной более наглядное имя, вроде seconds_remaining?

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

Вариант 2: Удалите из предложения субъект
Другой вариант избежать личного обращения — использовать сокращение, которое исключает из предложения субъект:

Надо подумать о переименовании переменной для более наглядного имени, вроде seconds_remaining.

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

Эта переменная должна быть переименована для более наглядного имени, вроде seconds_remaining.

Ещё один вариант — перефразировать предложение в виде вопроса, который начинается со слов «Что насчёт…» или «Как насчёт…»:

Что насчёт переименовать эту переменную для более наглядного имени, вроде seconds_remaining.

Оформляйте отзывы как запросы, а не команды

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

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

Сравните одно и то же замечание, оформленное двумя разными способами:

Людям нравится контролировать свою работу. Такой запрос даёт автору чувство самостоятельности.

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

Сравните, насколько воинственной кажется беседа в зависимости от оформления первоначального замечания:

Видите, насколько более цивилизованным стало общение, когда вы сконструировали воображаемый диалог в доказательство своего тезиса оформили замечание в виде запроса, а не команды?

Обосновывайте принципами, а не мнениями

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

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

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

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

Моё худшее код-ревью

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

Ревью

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

Я добросовестно задокументировал все проблемы, которые сумел найти, всего 59 штук. Если верить прочитанной литературе по код-ревью, я проделал отличную работу: нашёл ТАК много ошибок. Мне действительно было чем гордиться.

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

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

Цикл ожесточения

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

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

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

Этот ритуал повторялся каждый день в течение трёх недель. Код практически не изменился.

Вмешательство

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

Боб начал своё ревью с того, что попросил Мэллори составить новые списки изменений, выделив две маленькие библиотеки, о которых мы вообще не спорили, каждая на 30-50 строк. Когда Мэллори сделала это, Боб немедленно одобрил изменения.

Потом он вернулся к основному списку изменений, который сократился примерно до 200 строк кода. Он сделал несколько небольших замечаний, которые Мэллори исправила. Затем одобрил список изменений.

Всё ревью Боба завершилось за два дня.

Коммуникация имеет значение

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

Это был неприятный опыт, но я рад вспоминать о нём. Он заставил меня пересмотреть свой подход к код-ревью и определить направления для улучшения.

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

Техники

Стремитесь повысить уровень качества кода только на одну-две ступени

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

Я про себя ставлю коду оценку, как в школе, от 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 году, она сохраняет свою актуальность, демонстрируя долгосрочную ценность эффективного общения.

Источник