Проверка кода (code review) — отличный инструмент для повышения качества кода, но он не учитывает один факт: отправляют и просматривают код люди, а они устают, теряют сосредоточенность, ленятся, да и просто испытывают эмоции в самые неожиданные моменты.

Поэтому хочу представить свое видение хороших и плохих практик код ревью с учётом человеческих особенностей.

Люди ленивы

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

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

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

Плохая практика: несколько спринтов пилить функциональность, а потом залить её одним большим pull request’ом.

Хорошая практика: делать изменения небольшими кусочками и создавать pull request’ы по мере выполнения задач. Даже если их будет смотреть один человек, ему будет проще при дозированности информации.

Перекладывание ответственности

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

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

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

• все знают, что происходит в проекте;

• сотрудник понимает, что если он допустил ошибку, то виноват только он;

• младшие разработчики могут учиться, читая код старших разработчиков;

• тимлид не выгорает;

• в случае ухода тимлида команда сможет продолжить доставлять функциональность.

Плохая практика: прогонять весь код через одного человека.

Хорошая практика: делать перекрестную проверку, когда все члены команды смотрят код друг друга.

Сложно меняют контекст

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

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

Помоги своему коллеге выстроить контекст. Для этого в code review есть описание. Туда надо поместить:

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

• Как эта задача решалась и почему было выбрано именно это решение.

• Всю документацию, которая получилась при работе над задачей.

Это поможет твоему коллеге восстановить контекст и глубже оценить написанное тобой решение.

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

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

Люди не хотят глубоко погружаться в код

Если взять все замечания и отсортировать их по частоте появления, то получится вот такая последовательность:

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

Первые два типа замечаний, такие как опечатки или проблемы со стилистикой кода, легко обнаруживаются автоматически. Тебе необходимо договориться с командой, как вы будете оформлять свой код — сколько пробелов использовать в начале строки, оставлять ли висящую запятую и так далее, — и применять их для всех проектов команды. При этом отличным решением будет повесить проверку стилистики кода на pre-commit hook’е. Тогда на проверку будет попадать уже отформатированный код. Аналогично стоит поступить с орфографическими ошибками, добавив spell checker.

Плохая практика: писать в code review про опечатки.

Хорошая практика: использовать линтеры и spell checker.

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

Отличной практикой будет ведение чек-листов со списком того, на что нужно смотреть во время проверки кода. Например:

• поиск потенциальных проблем с безопасностью;

• проблемы с производительностью;

• оценка выбранного архитектурного решения;

• ....

Плохая практика: не искать в новом коде проблемы из конца списка.

Хорошая практика: использовать чек-листы и проверять по ним возможные проблемы.

Люди эмоциональны

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

Принимаем замечания на свой счет

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

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

Не связывать код с человеком, его написавшим

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

1. Эй, приятель! Ты ошибся в этом выражении. Но я все равно считаю, что ты умный! Вероятно, это просто опечатка.

2. Ты допустил ошибку в этом выражении, ты вообще знаком с синтаксисом языка? Кто вообще так пишет?

К счастью использования «ты» можно легко избежать, просто убрав объект из предложения: «Кажется, в этом выражении ошибка». Либо вообще составить предложение в виде вопроса: «Может, попробовать описать это при помощи конструкции switch/case?»

Оформление комментария в виде вопроса — хорошая практика, поскольку это оставляет человеку выбор; тогда как комментарий, оформленный в виде приказа (например, «Используй в этом месте switch/case»), заставляет разработчика занимать оборонительную позицию.

Плохая практика: использовать местоимение «ты» в комментарии к ошибке и писать комментарии в повелительном наклонении.

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

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

Плохая практика: считать, что человек, критикующий твой код, критикует тебя.

Хорошая практика: воспринимать критику как одну из точек для профессионального роста.

Конфликтность vs важность

Если замечания, которые возникают при проверке кода, разделить на четыре группы в зависимости от конфликтности и важности, то получится вот такая схема:

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

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

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

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

Плохая практика: холиварить во время код ревью.

Хорошая практика: приглашать модератора дискуссий в код ревью.

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

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

1. Молча согласиться и исправить.

2. Встретиться и лично обсудить вопрос, а после встречи занести решение в code review.

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

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

Такие встречи не должны быть длинными, договориться можно за 15-20 минут. После 30-40 минут конструктивное обсуждение заканчивается, а диалог перетекает в холивар. Если подобное происходит регулярно, то не стесняйтесь приглашать на такие встречи модератора, возможно даже из других команд или направлений.

Плохая практика: вести текстовые дискуссии.

Хорошая практика: обсуждать замечания лично.

Вывод

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

Эта статья основана на моих видео «Как ревьюить код? Не будь токсичным на код ревью» и «Код ревью как делать правильно? Хорошие и плохие практики (часть 2)». Спасибо за прочтение и приятного просмотра.