Классический процесс ревью кода страшен до безобразия и даже некоторого восторга. Разберём, почему это так, и что с этим делать.
В программировании мы любим задумываться над тем, как поизящнее написать программу, как выбрать методологию поэлегантнее, как бы удачнее применить паттерн. На этом наши возможности не останавливаются, и мы учим наши программы рефлексировать. То есть анализировать самих себя во время выполнения или же достраиваться во время компиляции. Мы учим нашу инфраструктуру автоматически восстанавливаться после сбоев или голосовать.
Когда же дело доходит до того, чтобы подумать, как работаем мы сами, то наступает ступор. С одной стороны, здоровому разработчику над этим думать не хочется, так как здоровый разработчик любит программировать. С другой стороны, доверять проектирование процесса разработки кому-то стороннему, например, менеджеру, тоже совершенно нежелательно. Что он вообще понимает, этот менеджер?! Он хоть программировать умеет?!
Можно было бы надеяться, что эта история уже каким-то чудесным образом разрешилась, и работаем мы уже хорошо, но нет. Мы только в начале пути. Нам пора начинать задумываться над тем, как мы работаем.
С этой точки зрения мой любимый процесс, в котором просто поразительное количество проблем, — ревью кода. Это тот случай, когда дела обстоят так плохо, что это даже хорошо. В этой статье мы рассмотрим традиционный процесс ревью с разных сторон и разберём пути устранения его несовершенства.
В русскоязычной литературе ревью иногда называется «обзором кода». Как по мне, это название не совсем удачное, и лучше использовать слово «разбор». Ведь мы не просто код собрались на код посмотреть, а основательно в нём разобраться. В совсем классических источниках вообще используется словосочетание «инспекция кода».
Например, такое употребление можно встретить в книге «Факты и заблуждения профессионального программирования»:
Тщательные инспекции позволяют устранить до 90% ошибок из программного продукта до того, как будет запущен первый эталонный тест.
Не стоит безоговорочно верить этим данным, так как в нашей индустрии нет хорошо выстроенной системы выдвижения и проверки гипотез. Гораздо более совершенная система есть в медицине. Но пускай 90% всё равно будут для нас ориентиром. Из книги неясно, о какого рода инспекциях идёт речь. Их, как мы увидим далее, не одна разновидность.
Откуда вообще пошла эта история с инспекциями кода? Дядюшка Боб Мартин любезно предоставил мне ссылку на то, что можно считать первым документальным свидетельством существования инспекций кода. Майкл Фэган (Michael Fagan) задокументировал информацию о них в своей статье 1976-го года Design and code inspections to reduce errors in program development.
В статье описано три вида инспекций:
Инспекция решения (design inspection);
Инспекция кода перед юнит-тестированием;
Инспекция кода после юнит-тестирования.
Предполагается, что после каждого этапа происходит исправление обнаруженных дефектов. Ещё я подозреваю, что в те годы юнит-тестирование было дорогостоящим занятием, и поэтому вокруг этого события расположены целых две инспекции.
Из всего набора инспекций в наши дни широко практикуется только один тип — ревью кода. Почему же мы не инспектируем наши решения?
Некоторые предположения об отсутствии разнообразия в видах ревью
Давайте снова обратимся к книге «Факты и заблуждения профессионального программирования». Вот что сообщает нам Роберт Гласс из 2002-го года:
Так почему же инспекции (называемые также экспертными проверками, хотя борцы за чистоту языка различают эти два термина) не прославляются так же, как меньшие «прорывы», например инструменты CASE и разные методологии? Тому есть четыре причины:
1. На инспекциях зарабатывают не многие ведущие производители.
2. В инспекциях нет ничего нового (и, следовательно, пригодного к продвижению на рынок).
3. ...
Крайне забавно, что причину в доминировании лишь одного вида ревью сегодня я вижу в прямо противоположном процитированному. Сегодня у нас есть рынок и представленные на нём инструменты. Это известные площадки для хостинга кода: GitHub, GitLab, Bitbucket. Более того, инструменты для ревью кода даже обычно не продаются отдельно, они сразу идут в комплекте с основным продуктом названных сервисов. С учётом популярности pull request'ов вообще сложно себе представить хостинг без инструмента для ревью. Да и кому же не хочется исправить одним махом 90% дефектов?!
А вот где же CASE, UML и прочие инструменты для моделирования решения ещё до кодирования? Их просто некуда добавить. Если действовать по аналогии с кодом, то где-то должна храниться диаграмма, трансформирующаяся затем в программу. Все изменения этой диаграммы должны проверяться внимательным взглядом через соответствующий инструмент. Известные мне инструменты вроде diagrams.net, LucidChart, Miro, Mural и многие другие под это не заточены.
Какие пороки существуют у ревью кода?
В этой части статьи мы рассмотрим ревью кода с разных сторон. Каждая из них продемонстрирует нам свои проблемы.
Процесс ревью с точки зрения BPMN
BPMN — это нотация и модель бизнес-процессов. То есть такой инструмент, с помощью которого можно описать бизнес-процесс. Среди средств описания присутствуют действия, события, логические операторы, сообщения и другие. Если вы временами описываете алгоритмы с помощью блок-схем, я рекомендую BPMN. Это гораздо более выразительный инструмент.
На нижеследующей диаграмме изображён процесс ревью с лишь одним проверяющим. Даже такой процесс таит в себе множество ужасов, чего уж тут говорить о процессе, когда проверяющих несколько. Для построения диаграммы я использовал специальный инструмент, BPMN Sketch Miner, создающий изображения из текста. Если вы также как и я терпеть не можете средства рисования диаграмм как раз-таки за то, что почти всё время занимает рисования, рекомендую присмотреться. Множество возвращающихся конвертов в левой нижней части — это ошибка отрисовки.
Всё начинается с создания PR’а. Следующий шаг для автора кода — оповестить ревьюера. В нём выражается любой из возможных способов оповещения проверяющего, будь то устная фраза или сообщение от бота. Далее начинается ожидание, и это самый интересный для нас этап. Продолжительность такого ожидания неизвестна и зависит от загруженности ревьюера. Затем происходит долгожданный разбор исходного кода. Есть шанс, что проверяющий одобрит код сходу, но может случиться и обратное — появятся несколько комментариев с просьбой исправить ошибки.
К этому моменту автор кода может быть уже давненько погружён в другую задачку. Теперь мы ждём его, и здесь снова продолжительность этого ожидания неизвестна. Возвращение к предыдущему коду потребует восстановления контекста. Затем нужно будет как-то истолковать для себя комментарии и отреагировать на них. Настало время оповестить ревьюера.
Не кажется ли вам, что недавно мы уже были в таком положении? Да, были. Мы только что прошли первую итерацию потенциально бесконечного цикла. Да-да, бесконечного. Всегда есть шанс, что ревью отправится на ещё одну доработку. Или же, что мы потеряем к нему всяческий интерес, и оно зависнет.
Хотим ли мы встраивать в наш процесс разработки потенциально бесконечный цикл? Обычно определённость предпочтительнее.
Процесс ревью с точки зрения видения решений
Иногда в наших командах код проверяют старшие разработчики или даже архитекторы. Им хочется, чтобы кодовая база была консистентной, чтобы использовались одни подходы и избегались другие. У них есть видение. У автора кода имеется своё видение. В большинстве случаев автор даже не знает о том, что именно хотят наиболее уважаемые товарищи. Или же забывает, не стоит отметать и этого. Возможно, именно в этот раз просто не получилось сделать, так как обычно делается. Ограничение использования каких-то подходов не приходит просто так и требует вспомогательной среды, постоянных напоминаний, обучения, а такое встречается редко.
Давайте посмотрим на следующее изображение.
На графике видно различие в восприятии автора и ревьюера. После первой итерации ревью начинается схождение, но нам ещё идти и идти.
Представьте себе, что вы наняли бригаду строителей, попросили их построить дом и больше никак с ними не общались до завершения строительства. Как думаете, насколько результат будет совпадать с вашим ожиданием? С большой долей вероятности расхождение будет значительным. Вот и здесь случается что-то похожее. Ведь если у разработчика нет определённого фреймворка, в рамках которого он принимает решения, то они могут быть совершенно неожиданными.
Процесс ревью с точки зрения межличностного взаимодействия
Картинка говорит сама за себя. Попросите ли вы своего коллегу полностью переделать разработанное, если он потратил на него много часов работы? Представьте себе конец спринта, задачку, которая вызывала трение на стендапе. Скорее всего, вы поможете своему товарищу влить задачку как можно скорее. С другой стороны, бывают и такие ситуации, когда опытный разработчик проверяет код новичка. В таком случае вариант «начинаем всё заново» вполне может случиться. Но своевременен ли такой вариант? Ведь автор кода свернул на кривую дорожку уже достаточно давно и уже долго продвигался по ней, принимая одно решение за другим.
Процесс ревью с точки зрения бережливой разработки
Бережливое производство ещё многому может научить нашу индустрию. Хотя, стоит отметить, что у нас уже есть «Бережливая разработка программного обеспечения» авторства Мэри Поппендик и Toма Поппендика. Она не особенно популярна, но для нашего анализа будет полезна. В этой книге описываются семь видов потерь, имеющих место в разработке ПО. Это не исчерпывающий список всех возможных несчастий. Просто в бережливом производстве есть традиция изобретать для разных сфер деятельности свои семь типов потерь. Оригинальные потери для Toyota изобрёл в своё время Тайити Оно, отец-основатель производственной системы «Тойоты». Именно эта производственная система вне Toyota стала Lean, то есть бережливым производством.
Семь типов потерь для индустрии разработки ПО следующие:
Частично сделанная работа,
Избыточные функции,
Повторное обучение,
Передачи,
Задержки,
Переключения между задачами,
Дефекты.
Классический процесс ревью кода собрал 6 типов потерь из 7-ми!🎉
Частично сделанная работа. Задачка в ревью большую часть времени проводит в ожидании. При анализе КПД потока создания ценности я обычно считаю эту стадию не временем, когда совершается полезное действие, а очередью. Ещё я часто наблюдаю интересный психологический эффект. Разработчик создаёт PR и считает, что его работа на этом закончена. Ревью кода — это крайне безответственное состояние задачи. По этой причине часто необходимо вмешательство руководства для стимулирования дальнейшего продвижения.
Повторное обучение. Этот этап виден даже на диаграмме процесса. Восстановление контекста задачи и её решения — это повторное обучение.
Передачи. Сложно отнести это к потерям в данном конкретном случае, так как передача от одного человека к другому — ключевая механика ревью.
Задержки. В ревью кода есть целых две задержки, закрученные в волнующий потенциально бесконечный цикл.
Переключение между задачками. Люди отвлекаются от своих задач для того, чтобы провести ревью или отреагировать на него.
Дефекты. Ревью кода подходит для обнаружения косметических дефектов, но не серьёзных проблем. Уже упомянутое выше справедливое нежелание «топить» товарищей на ревью приводит к тому, что в проекте начинают накапливаться значительные изъяны.
Мы рассмотрели процесс ревью с четырёх сторон. Что же теперь можно предпринять для его исправления?
Перепроектируем ревью кода
Структура этой части совпадает с предыдущей с той лишь разницей, что рассматривать мы будем исправленный процесс.
Исправляем структуру процесса
В обновлённой версии процесса мы видим автора кода, ожидающего завершения ревью и сессию парной разработки на случай, если есть что исправлять.
Также здесь присутствуют следующие значительные изменения:
Потенциально бесконечного цикла больше нет.
Осталось лишь одно время ожидания. С ним мы тоже разберёмся.
Автору кода не надо восстанавливать контекст и истолковывать комментарии. Они сейчас служат в качестве напоминаний для ревьюера.
Какие предварительные условия нужны для того, чтобы новый процесс заработал? В команде нужна новая роль дежурного по ревью. Она подразумевает работу над традиционно низкоприоритетными задачами, например, техническим долгом или неважными багами. Как только появляется ревью кода, дежурный тут же бросает свою задачку и подключается к прибывшему PR’у.
Если вы когда-то интересовались, зачем в системах требуется недозагрузка, так вот за этим. Если все в команде заняты высокоприоритетными делами и не имеют ни секунды свободного времени, то общее время разработки увеличится.
Исправляем различие в видении решения
Что мы обычно обсуждаем во время ревью кода? Решения, принятые во время его написания. Что-то было решено 2 часа назад, что-то неделю назад. Одно наше решение стоит на другом, между ними существует зависимость.
Насколько вам как автору кода понравится, если во время ревью сомнению будет подвергнуто одно из ваших первых решений? То самое, на котором стоит ещё пара сотен других. Думаю, вы можете и огорчиться из-за такой перспективы.
Лучше всего обсуждать проект решения поставленной задачи в момент его появления. Для того, чтобы такое обсуждение случалось, давайте добавим дополнительное ревью: ревью решения. Временами, когда проблема сложна, полезно провести некоторое время над планом и обсудить его с опытными коллегами.
Существует следующая полководческая мудрость:
Ни один план сражения не выдерживает контакта с противником.
Если мы перефразируем его в язык систем, то звучать будет примерно так: «Системе определённо будет необходимо изменить своё поведение, когда начнёт поступать обратная связь». В случае с кодированием обратной связью будут трудности, с которыми мы сталкиваемся при попытке приложить нашу доработку к существующему программному продукту.
С началом работ некоторые планы потребуется пересмотреть и поменять. Тут-то мы снова разойдёмся в видении с человеком, с которым проговаривали предполагаемый путь.
Адам Торнхилл (Adam Thornhill) в своей потрясающей книге Software Design X-Rays предложил следующий метод:
Именно поэтому я рекомендую вам делать первое ревью кода гораздо раньше. Вместо того, чтобы ждать завершения разработки, начните презентовать и обсуждать код на одной трети его готовности. Обращайте меньше внимания на детали, фокусируйтесь на общей структуре, зависимостях, соответствию предметной области. Понятно, что «треть» — это субъективное понятие, воспринимайте его как точку, когда базовая структура уже закодирована, проблема хорошо понятна и написан стартовый набор тестов. На этом этапе переделка всей структуры — это всё ещё адекватная возможность. Устранение потенциальных проблем сейчас гораздо дешевле, чем впоследствии.
В моей команде мы используем выражение «ревью скелета». Оно красноречивее описывает суть данного ревью.
Ревью решения избавляет нас и от межличностного трения: потребность просить коллегу всё переделать теперь вряд ли возникнет.
Смотрим на новый процесс с точки зрения бережливой разработки
Предложенный процесс устраняет или в значительной степени снижает обнаруженные в предыдущей версии процесса потери:
Частично сделанная работа. Автор кода имеет значительно меньше времени переключиться на другую задачку во время ожидания, да и можно явно это запретить. Ценная для пользователя задача не зависает на неопределённое время в ревью. Частично сделанная задача ревьюера — это цена полученной гибкости.
Повторное обучение. Этот негативный эффект пропадает, так как время ревью становится небольшим. Автор кода не успевает забыть.
Передачи. Остаются, хотя их число уменьшается из-за изъятия потенциально бесконечного цикла.
Задержки. Задержки для участников ревью значительно снижаются.
Переключение между задачками. Переключение более не разрешается автору кода и ограничено для ревьюера.
Дефекты. Исправление дефектов в предлагаемом процессе становится, по крайней мере, дешевле. Также у нас появляется возможность исправлять дефекты проектирования и не тащить их в проект на чувстве жалости к уже проделанной работе.
В завершение
Мы изучили процесс разбора кода для одного автора и одного ревьюера. Если бы число ревьюеров стало больше, то и количество проблем увеличилось бы. Да и изображения потребовалось бы рисовать в трёхмерном пространстве.
К сожалению, не все баги всплывают на поверхность, если вы добавляете людей на поздних этапах посмотреть на ваши решения. Скорее, вы просто затянете вливание вашего PR’а.
В попытках внедрить предложенный процесс я столкнулся с тем, что менеджеров ужасала сама мысль о существовании разработчиков, не занятых максимально приоритетными делами, тех самых дежурных ревьюеров. Хорошо, что наши менеджеры не руководят пожарными.
По поводу этой проблемы я переведу цитату из книги Actionable Agile Metrics for Predictability от Даниэля Ваканти (Daniel Vacanti):
Есть много стратегий, которые использует FedEx, но, вероятно, самая важная заключается в том, что в любой момент времени у FedEx есть пустые самолеты в воздухе. Да, я сказал пустые самолеты. Таким образом, если локация будет перегружена или если какие-то посылки не улетят из-за того, что регулярный рейс был заполнен, то пустой самолет перенаправляется («точно вовремя», нужно сказать) в проблемное место. В любое время у FedEx есть «запас прочности в воздухе»!
Если вы менеджер, подумайте над этим в следующий раз, когда будете планировать загрузить всех задачками по максимуму.
Довольны ли мы результатом? Да, дела обстоят гораздо приятнее, чем ранее.
Возможно ли работать ещё лучше? Конечно!
В качестве ещё более амбициозной цели можно поставить себе полный отход от ревью кода. Но не такой, когда мы его отменяем одним днём, и у нас всё валится, а такой, когда в ревью кода больше нет никакого смысла. Когда необходимый уровень качества кода достигается ещё в процессе разработки. Когда качество встроено в разработку. Сделать это можно через разработку вспомогательного фреймворка для принятия решений, чтобы среда разработки подсказывала разработчику принятые в данной команде или компании подходы к проектированию и кодированию. Я с таким фреймворком никогда не сталкивался, но линтеры, дающие подсказки прямо в IDE — это шаг в нужном направлении.
Спасибо за чтение!
