Проверка кода (code review) — отличный инструмент для повышения качества кода, но он не учитывает один факт: отправляют и просматривают код люди, а они устают, теряют сосредоточенность, ленятся, да и просто испытывают эмоции в самые неожиданные моменты.
Поэтому хочу представить свое видение хороших и плохих практик код ревью с учётом человеческих особенностей.
Люди ленивы
Все люди ленивы. Под ленью я понимаю состояние экономии ресурсов, будь то физических или ментальных. Наши предки никогда не жили в изобилии ресурсов, поэтому организм научился их экономить, а лень — это один из эффективных инструментов.
Я заметил интересную особенность: если я пришлю на проверку небольшой кусок кода, то наверняка получу гораздо больше замечаний, чем если бы изменений было в разы больше. На первый взгляд, это нелогично, ведь чем больше кода, тем больше в нем проблем. Но если представить себе разработчика, который открывает страницу с изменениями и в ужасе видит 20 страниц нового кода, то становится понятно, почему он быстрее пытается его закрыть.
Открывая code review, разработчик рефлекторно оценивает количество работы, которую придется проделать, и с бо̒льшим рвением берется за маленькие изменения, поскольку на них потребуется меньше сил. Я считаю, что большие изменения почти никто не читает, разработчики просто стараются найти несколько дефектов, чтобы сделать видимость проверки.
Плохая практика: несколько спринтов пилить функциональность, а потом залить её одним большим pull request’ом.
Хорошая практика: делать изменения небольшими кусочками и создавать pull request’ы по мере выполнения задач. Даже если их будет смотреть один человек, ему будет проще при дозированности информации.
Перекладывание ответственности
В своей книге «5 пороков команды» Патрик Ленсиони явно показывает, что в команде начинаются проблемы, когда никто из сотрудников не хочет нести ответственность за результат.
Я достаточно часто встречаю ситуацию, когда весь код, который пишет команда, проходит через тимлида. В долгосрочной перспективе разработчики теряют ощущение, что они ответственны за создаваемую функциональность. Ответственность как бы делится между разработчиком и тимлидом. Разработчик начинает допускать мысль, что он может ошибиться, поскольку тимлид обязательно найдет проблему и исправит. Участники таких команд часто становятся безликими закрывателями тасок. А если тимлид уходит из компании, то команда оказывается недееспособной.
Вместо того, чтобы прогонять весь код через одного человека, стоит посмотреть в сторону практики перекрестной проверки, когда ревьюверы назначаются случайным образом. При таком подходе:
все знают, что происходит в проекте;
сотрудник понимает, что если он допустил ошибку, то виноват только он;
младшие разработчики могут учиться, читая код старших разработчиков;
тимлид не выгорает;
в случае ухода тимлида команда сможет продолжить доставлять функциональность.
Плохая практика: прогонять весь код через одного человека.
Хорошая практика: делать перекрестную проверку, когда все члены команды смотрят код друг друга.
Сложно меняют контекст
Наш мозг может обмануть сознание во многих ситуациях, и один из таких обманов — это многозадачность, точнее, иллюзия, что мы многозадачны. На самом деле это не так, мозг просто быстро меняет контекст. Но каждая смена контекста — это затраты энергии, а как мы уже выяснили, наш мозг ленив. В итоге проверяющему либо приходится тратить много сил на переключение контекста, либо он просто не погружается в код, ограничиваясь поверхностным просмотром.
Заканчивая работу над задачей, ты имеешь максимально полный контекст. Твоему коллеге же мало что понятно, и ему придется трать силы на восстановление этого контекста. К сожалению, без понимания проблемы, для решения которой была поставлена задача, невозможно оценить само решение. Поэтому для полноценного код ревью восполнять контекст всё-таки придется.
Помоги своему коллеге выстроить контекст. Для этого в code review есть описание. Туда надо поместить:
Причины, по которым эта задача вообще нужна. Чаще всего они описаны в тикете, поэтому можно добавить ссылку на него.
Как эта задача решалась и почему было выбрано именно это решение.
Всю документацию, которая получилась при работе над задачей.
Это поможет твоему коллеге восстановить контекст и глубже оценить написанное тобой решение.
Плохая практика: не делать описание, надеясь, что твой коллега в курсе, чем ты занимаешься.
Хорошая практика: делать подробное описание, которое включает в себя причины, по которым нужна эта задача, и описание решения с пояснением, почему задача решалась именно таким путем.
Люди не хотят глубоко погружаться в код
Если взять все замечания и отсортировать их по частоте появления, то получится вот такая последовательность:
Если посмотреть на эти замечания с точки зрения затрат сил на их обнаружение, то окажется, что проблемы, которые легко обнаружить, будут вверху списка, а проблемы, для поиска которых придется потрудиться, окажутся внизу. Это еще одно объяснение, почему люди не очень хотят глубоко погружаться в твой код.
Первые два типа замечаний, такие как опечатки или проблемы со стилистикой кода, легко обнаруживаются автоматически. Тебе необходимо договориться с командой, как вы будете оформлять свой код — сколько пробелов использовать в начале строки, оставлять ли висящую запятую и так далее, — и применять их для всех проектов команды. При этом отличным решением будет повесить проверку стилистики кода на pre-commit hook’е. Тогда на проверку будет попадать уже отформатированный код. Аналогично стоит поступить с орфографическими ошибками, добавив spell checker.
Плохая практика: писать в code review про опечатки.
Хорошая практика: использовать линтеры и spell checker.
Еще одна особенность этого списка в том, что последние три типа проблем можно обнаружить только на код ревью и в продакшене. Особенно это касается проблем с безопасностью. Даже если ваша компания периодически проводит аудит безопасности, люди, которые будут его проводить, начнут, скорее всего, с просмотра кода, и тем самым сделают своеобразное code review.
Отличной практикой будет ведение чек-листов со списком того, на что нужно смотреть во время проверки кода. Например:
поиск потенциальных проблем с безопасностью;
проблемы с производительностью;
оценка выбранного архитектурного решения;
....
Плохая практика: не искать в новом коде проблемы из конца списка.
Хорошая практика: использовать чек-листы и проверять по ним возможные проблемы.
Люди эмоциональны
Если бы по обе стороны код ревью сидели «эконы», однако в этом процессе участвуют обычные люди, которые склонны испытывать эмоции, зачастую заставляющие нас вести себя не очень разумно.
Принимаем замечания на свой счет
Самый яркий пример нерационального поведения — принимать замечания на свой счет. Эконы понимают, что замечания должны улучшить код проекта и в долгосрочной перспективе принести пользу всем.
К сожалению, на практике это работает иначе. Любой человек в долгосрочной перспективе пытается оградить себя от деятельности, которая связана с негативом, и разработчики не исключение. Программист, который постоянно получает негативную обратную связь от коллег, становится безынициативным, теряет интерес к написанию кода и, как следствие, перестает развиваться.
Не связывать код с человеком, его написавшим
При написании комментария к ошибке необходимо избегать местоимения «ты». Для примера, вот безобидный комментарий: «Ты допустил ошибку вот в этом выражении...». Но поскольку общение происходит в виде переписки, то читающий этот комментарий в зависимости от своего настроения, отношений с коллегами, уверенности в себе и еще кучи факторов может интерпретировать эту фразу двумя способами:
Эй, приятель! Ты ошибся в этом выражении. Но я все равно считаю, что ты умный! Вероятно, это просто опечатка.
Ты допустил ошибку в этом выражении, ты вообще знаком с синтаксисом языка? Кто вообще так пишет?
К счастью использования «ты» можно легко избежать, просто убрав объект из предложения: «Кажется, в этом выражении ошибка». Либо вообще составить предложение в виде вопроса: «Может, попробовать описать это при помощи конструкции switch/case?»
Оформление комментария в виде вопроса — хорошая практика, поскольку это оставляет человеку выбор; тогда как комментарий, оформленный в виде приказа (например, «Используй в этом месте switch/case»), заставляет разработчика занимать оборонительную позицию.
Плохая практика: использовать местоимение «ты» в комментарии к ошибке и писать комментарии в повелительном наклонении.
Хорошая практика: использовать обезличенные предложения. Составлять комментарии в виде вопроса, давая читающему возможность согласиться или поспорить с утверждением.
Программисту, отправляющему код на проверку, стоит отделять отношение к себе от кода. Если кто-то ругает твой код, то не стоит считать, что он ругает тебя. Попробуйте отнестись к этому как к возможной точке для профессионального развития.
Плохая практика: считать, что человек, критикующий твой код, критикует тебя.
Хорошая практика: воспринимать критику как одну из точек для профессионального роста.
Конфликтность vs важность
Если замечания, которые возникают при проверке кода, разделить на четыре группы в зависимости от конфликтности и важности, то получится вот такая схема:
В идеале, ошибки нужно заводить только по замечаниям из четвертой группы, поскольку они наиболее конструктивны, полезны для проекта и вызывают наименьший негатив.
Проблемы из третьей группы должны исправляться автоматически при помощи линтеров и spell checker`ов.
Проблемы из первой группы я бы вообще отнес к вкусовщине: это то, что мало влияет на работу проекта в целом, зато очень сильно раздражает.
Если в вашей команде при проверке часто возникают замечания из первой группы, то стоит ввести роль модератора — это человек, который не участвует в проверке, но следит за адекватностью дискуссий. Если обсуждение постепенно переходит в плоскость холивара, модератор должен остановить это.
Плохая практика: холиварить во время код ревью.
Хорошая практика: приглашать модератора дискуссий в код ревью.
Если у вас в команде часто возникают замечания из второй группы, которые, с одной стороны, важны для качества кода проекта, а с другой — могут стать причиной конфликтов, то стоит ограничить возможности текстовой переписки.
Если проверяющий заводит ошибку, то у написавшего код есть всего две возможности:
Молча согласиться и исправить.
Встретиться и лично обсудить вопрос, а после встречи занести решение в code review.
Дело в том, что во время личного обсуждения вероятность неправильной интерпретации фразы меньше.
Возможно, в начале встреч станет больше и вам будет о чем подискутировать с коллегами, но в итоге количество встреч придет в норму, а внутри команды выработается общее представление о подобных ситуациях.
Такие встречи не должны быть длинными, договориться можно за 15-20 минут. После 30-40 минут конструктивное обсуждение заканчивается, а диалог перетекает в холивар. Если подобное происходит регулярно, то не стесняйтесь приглашать на такие встречи модератора, возможно даже из других команд или направлений.
Плохая практика: вести текстовые дискуссии.
Хорошая практика: обсуждать замечания лично.
Вывод
Код ревью может подсвечивать проблемы в команде. Это могут быть конфликты в процессах или межличностных отношениях. Например, отправка большого количества кода говорит о проблемах с декомпозицией, а постоянные неконструктивные замечания от одного из разработчиков могут говорить о личных конфликтах внутри коллектива. Если помнить, что проверкой занимаются люди со своими особенностями и эмоциями, то она станет отличным инструментом для поддержания кодовой базы в чистоте.
Эта статья основана на моих видео «Как ревьюить код? Не будь токсичным на код ревью» и «Код ревью как делать правильно? Хорошие и плохие практики (часть 2)». Спасибо за прочтение и приятного просмотра.