Классический процесс ревью кода страшен до безобразия и даже некоторого восторга. Разберём, почему это так, и что с этим делать.
В программировании мы любим задумываться над тем, как поизящнее написать программу, как выбрать методологию поэлегантнее, как бы удачнее применить паттерн. На этом наши возможности не останавливаются, и мы учим наши программы рефлексировать. То есть анализировать самих себя во время выполнения или же достраиваться во время компиляции. Мы учим нашу инфраструктуру автоматически восстанавливаться после сбоев или голосовать.
Когда же дело доходит до того, чтобы подумать, как работаем мы сами, то наступает ступор. С одной стороны, здоровому разработчику над этим думать не хочется, так как здоровый разработчик любит программировать. С другой стороны, доверять проектирование процесса разработки кому-то стороннему, например, менеджеру, тоже совершенно нежелательно. Что он вообще понимает, этот менеджер?! Он, хоть программировать умеет?!
Можно было бы надеяться, что эта история уже каким-то чудесным образом разрешилась, и работаем мы уже хорошо, но нет. Мы только в начале пути. Нам пора начинать задумываться над тем, как мы работаем.
С этой точки зрения мой любимый процесс, в котором просто поразительное количество проблем, — ревью кода. Это тот случай, когда дела обстоят так плохо, что это даже хорошо. В этой статье мы рассмотрим традиционный процесс ревью с разных сторон и разберём пути устранения его несовершенства.
В русскоязычной литературе ревью иногда называется «обзором кода». Как по мне, это название не совсем удачное, и лучше использовать слово «разбор». Ведь мы не просто код собрались на код посмотреть, а основательно в нём разобраться. В совсем классических источниках вообще используется словосочетание «инспекция кода».
Например, такое употребление можно встретить в книге «Факты и заблуждения профессионального программирования»:
Тщательные инспекции позволяют устранить до 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 — это шаг в нужном направлении.
Спасибо за чтение!
Комментарии (35)
AlexCzech01
19.12.2022 04:14+3В маленькой команде, где у всех относительно неплохие знания обо всём (4, максимум 5 человек), такая схема будет работать, но будет обходиться очень дорого, потому что "дежурный по ревью" - это будет 20-25% ресурса; если не предполагать, что в большинстве случаев никаких доработок и не потребуется и большинство времени человек будет не ревьюить, а заниматься какими-то своими задачами - но в этом случае и никакой страшно нарисованной проблемы задержек и бесконечных циклов не будет - проверяющий быстро посмотрит, скажет ок и вернется к своим делам. Ревью откладывается на потом именно в командах, где в большинстве случаев это трудный, долгий и мучительный процесс с большим количеством итераций.
А в большой команде, где ревью много, а знания о системе распределены неравномерно, процесс, построенный по описанию в статье, выродится в одну из двух сторон в зависимости от степени добросовестности ревьюера. Либо это будет чисто формальная процедура, которая никаких багов конечно не предотвратит, кроме самых явных и тупых - будет проверено соответствие кода формальным критериям, имен переменных соглашениям и т.п.: то, что мог бы сделать линтер и человек для этого не нужен. Либо ревьюер, сидя над кодом часами - сначала сам разбираясь в нём, потом разбираясь вместе с автором, как его исправить - сам станет узким место процесса, потому что пока он ковыряется в одном реквесте, там его уже ждут ещё 5, а он должен закончить этот, чтобы бесконечных циклов и вываливаний из потока не произошло
В общем, там где описанное выглядит хорошо - нет проблемы, которую надо решать, а там где есть проблема - там оно хорошо не выглядит
Mingun
19.12.2022 07:59+1Кроме того, в финальной диаграмме цикл просто из явной формы переведен в неявную, так как никак не детализировано, каким же образом будет происходить "парная доработка". А он будет описываться такой же диаграммой, только быть может без явного открытия PR.
novoselov
19.12.2022 10:11+5Так там и вторая задержка никуда не делась, просто автор ее решил не рисовать для "красоты". Кто сказал что программисту удобно делать парное ревью когда скажет ревьюер? Вот и добавьте время на "договориться когда созвониться"
turbobureaucrat Автор
19.12.2022 21:12Да, это время стоит добавить. Но стоит добавить его в пункт «Когда ревьюер освобождается от прочих дел», то есть в первую и единственную задержку. В том виде, в котором я сейчас внедряю процесс, предполагается, что ревью происходит синхронно. То есть и ревьюер, и автор кода должны быть доступны в течение ближайшего часа после открытия PR'а.
turbobureaucrat Автор
19.12.2022 21:14Парная разработка происходит уже после открытия PR'а. Ревьюер и автор кода созваниваются, производят доработки. Можно просто через Zoom, можно через более подходящие для этого средства. У VS Code и IntelliJ, например, есть плагины для удалённой работы.
Mingun
20.12.2022 17:44А если у них разногласия, тогда как? Какой алгоритм описывает их разрешение? Уж не тоже ли самый?
turbobureaucrat Автор
20.12.2022 18:21Если разногласия у автора кода и дежурного ревьюера? Примерно на 50 PR'ов у нас такое всплыло раз, и мы позвали архитектора разрешить спор.
turbobureaucrat Автор
19.12.2022 18:33Спасибо за содержательный комментарий. В своей команде я сейчас провожу эксперимент «ревью за час». Его задача состоит в том, чтобы обнаружить продолжительные активности, случающиеся в время ревью кода, и явно передвинуть их на этап разработки. Таким образом мы пытаемся уйти и от узости горлышка, и от того, что на ревью будут проскакивать серьёзные ошибки.
Команда большая, 20 человек.
turbobureaucrat Автор
19.12.2022 21:09И ещё один момент. Вы пишете о важном качестве — добросовестности. Это прекрасная характеристика, но, к сожалению, непостоянная. У кого-то добросовестность означает один результат, у кого-то другой, да и от типа задачи зависит. Вот эту самую добросовестность хочется ожидать не от конкретного человека, а встроить в сам процесс разработки, чтобы он выдавал добросовестный результат.
panzerfaust
19.12.2022 08:15+2У автора кода имеется своё видение. В большинстве случаев автор даже не
знает о том, что именно хотят наиболее уважаемые товарищиНу значит имеем косямбу то ли от товарищей, то ли от автора. Написать гайд по основным моментам - невеликий труд. Прочитать его и уточнить неясности - тоже.
Далее начинается ожидание, и это самый интересный для нас этап.
Продолжительность такого ожидания неизвестна и зависит от загруженности
ревьюера.Договоритесь об SLA на данную процедуру среди своих коллег. Например, МР не может висеть без внимания больше рабочего дня. Закрепите соглашение на общем собрании, а потом карайте нарушителей.
Ведь автор кода свернул на кривую дорожку уже достаточно давно и уже долго продвигался по ней, принимая одно решение за другим.
Судя по контексту, автор кода свернул не туда, когда решил запихнуть всю задачу в один МР размером с кита. Это решается декомпозицией задачи. У меня была такая проблема в одном стартапе, куда разом запустили трех джунов. После нескольких жестких разборов полетов и одного увольнения все чудом научились делать МРы с более скромными скоупами.
В общем я вижу в вашей истории проблему не с ревью, а с коммуникациями в команде и с общей культурой разработки.
turbobureaucrat Автор
19.12.2022 18:29Ну значит имеем косямбу то ли от товарищей, то ли от автора. Написать
гайд по основным моментам - невеликий труд. Прочитать его и уточнить
неясности - тоже.Частично соглашусь в этом моменте. Хочу заметить при этом, что ограничений обычно достаточно много, и гайд может и не смочь закрыть все моменты. Даже если он и будет где-то находиться и покрывать необходимые моменты, то в каких-то решениях о нём могут забыть.
Договоритесь об SLA на данную процедуру среди своих коллег. Например, МР
не может висеть без внимания больше рабочего дня. Закрепите соглашение
на общем собрании, а потом карайте нарушителей.Про кару нарушителей — весёлый момент.???? В подавляющем большинстве случаев этот момент зависит не от разработчиков, а от устройства процесса разработки, то есть системы. Поэтому карательные усилия я рекомендую направлять на его преобразование.
Судя по контексту, автор кода свернул не туда, когда решил запихнуть всю задачу в один МР размером с кита. Это решается декомпозицией задачи. У меня была такая проблема в одном стартапе, куда разом запустили трех джунов. После нескольких жестких разборов полетов и одного увольнения все чудом научились делать МРы с более скромными скоупами.
В общем я вижу в вашей истории проблему не с ревью, а с коммуникациями в команде и с общей культурой разработки.
Да, про декомпозицию задачи — хорошее замечание, тут я согласен. Подскажите, пожалуйста, о команде какого размера вы говорите? Думаю, что это может иметь значение.
rsashka
19.12.2022 08:27+1Ревью бывают разными, о чем вы и начали писать в самом начале статьи. Но потом все смешали в одну кучу, вот у вас и получился вывод, что "ревью ненужно".
На самом деле важно осознавать, что разные виды ревью требуют разных подходов и разных аудиторий. В ревью для 10 строчек кода действительно можно найти 10 ошибок.
А вот ревью на 5000 строк может пройти без проблем, но совершенно по другой причине. Все понимают, что такой объем кода быстро проанализировать невозможно, поэтому естественно "режим" ревью будет отличаться от "режима" для 10 строк.
Если в первом случае будет анализироваться каждая строка, то второй случай буде скорее ревью архитектурным на общую концепцию, а не на реализацию. А раз анализируется архитектура кода, то и мелкие косяки в реализации можно игнорировать (например, можно включить в большое ревью первые 10 строк кода и они с большой вероятностью пролетят без замечаний).
Вот кстати заметка, как можно организовать архитектурно ревью с помощью Хабра, который ума палата :-)
turbobureaucrat Автор
19.12.2022 18:21Да, хорошее замечание про различные объёмы PR'ов.
Для меня в работе важно иметь выровненный поток разработки, и поэтому для ревью на 5000 строк не ставится вопрос о целесообразности ревью. Ставится вопрос, каким образом нам так перепроектировать нашу деятельность, чтобы такие здоровые PR'ы не возникали.
Я не писал, что ревью не нужно, а то, что в качестве цели можно поставить себе отказ от него. Но для этого нужно будет серьёзно поработать.
За заметку спасибо!????
klimkinMD
19.12.2022 11:26К вопросу инспекции кода (текста): "борцы за чистоту языка" различают понятия "методология" и "методика"
turbobureaucrat Автор
19.12.2022 18:13Подскажите, пожалуйста, на какой момент в тексте обратить внимание? За обоими использованиями слова «методология» в тексте стоит именно товарищество ИП, ООП, ФП и их друзья.
WondeRu
19.12.2022 11:27+1Оставлю свой майндмеп по код ревью здесь
turbobureaucrat Автор
19.12.2022 18:10+1Лихой майндмап, спасибо! Расскажете, какая идея за ним стоит?
WondeRu
19.12.2022 23:41+1Делал для подготовки к интервью, потом сам использовал, когда интервьюировал лида)
alexhott
19.12.2022 11:30Проблема тут как мне кажется не в том.
Работал я в проекте где было с десяток команд. Каждый реквест требовал как минимум 3 ревью (архитектор, аналитик, разработчик) иногда больше. И к тому же сначала из своей ветки в общую dev, потом в релиз.
И никаких особых проблем с этим не было. Если надо быстро - то пишешь, звонишь, связываешься другим способом с людьми и просишь посмотреть. Есть несогласие или недопонимание - пишешь, звонишь, связываешься другим способом с людьми и обсуждаешь.
Ну то есть если люди адекватные с обеих сторон, то коммуникации решают все вопросы и быстро.
ЗЫ: я вижу в ревью еще сильную обучающую модель.turbobureaucrat Автор
19.12.2022 18:09Подскажите, пожалуйста, что имеется в виду в вашем комментарии под проблемами? Что стоит за словом «быстро»?
Сейчас я наиковарнейшим образом запустил у себя в команде эксперимент «ревью за час», всплывает всякое.????
Про обучение хорошо, что написали, задумаюсь над этим моментом.
murkin-kot
19.12.2022 13:19+2Общая проблема подобных текстов - концентрация на мелочах.
Да, автор проделал полезную работу, но полезна она в узких рамках одного из множества процессов разработки. Очевидно, что оптимизируя всю разработку, то есть комплекс процессов, можно получить во много раз больший результат. Но как раз замахиваться на всё целиком желающих нет. Точнее на это замахивается молодёжь, но им абсолютно разумно и быстро бьют по рукам, ибо они навертят такого, что потом точно всё встанет.
Главный стопор, как всегда - бизнес. Им надо быстро, потому всё оптимизировать никогда не дадут, ибо дорого. В результате поощряется кусочная оптимизация, частичная автоматизация, а в целом - страшное убожество, если смотреть глобально.
С другой стороны, если и мелочью не заниматься, то получится вообще печально - никакого развития, даже в виде крайнего убожества. Но тратить время на убожество тоже ничуть не радостная перспектива. Отсюда мораль - бизнес обязан находить в себе ресурсы для какого-то оптимального улучшения самого себя. Проблема здесь следующая - бизнес платит деньги и ожидает, что за деньги ему сделают красиво. Но это, как большинству из нас отлично известно, явное заблуждение. Или отмазка, то есть самооправдание бизнеса и перекладывание вины с себя на работников. Вот такие отмазки и нужно вылечить. Чья это задача? Уж точно не программиста.
turbobureaucrat Автор
19.12.2022 18:06Ох, очень нравится мне ваш комментарий, с него и начну отвечать.????
Да, действительно эта статья концентрируется только на одной локальной оптимизации, которая в глобальном смысле может и не дать организации никакого преимущества. Но практикующему менеджеру она может быть полезна хотя бы с точки зрения понимания того, каким образом можно думать о процессе, как его дорабатывать.
К счастью, наличие статьи о некой локальной оптимизации не является свидетельством того, что я не осуществляю попытки производить и более глобальные оптимизации. Идут они с куда большим трением, но всё-таки идут. Например, процесс разработки интересных для пользователя эпиков ускорился в три раза просто потому, что мы стали тщательно собирать статистику. Не факт, что ускорение вообще было, возможно мы просто обнаружили действительные очертания системы, но тем не менее.
Однако из этой статистики видно, что КПД процесса разработки эпиков и после тройного ускорения в лучшем случае сейчас составляет 30%, и нам есть куда двигаться.
К слову, мне очень странно, что бизнес в этом не заинтересован. Ускорить разработку в 9 раз, как по мне, так очень бизнес-ориентированно.
murkin-kot
19.12.2022 20:41+1очень странно, что бизнес в этом не заинтересован
Если это крупняк, то всё вполне закономерно. А если мелочь, то вы просто не достучались до нужного человека. Хотя и во втором случае могут быть нюансы вроде зиц-председателя или конторы-прокладки.
То есть поймите свою среду обитания, да обрящете.
GbrtR
19.12.2022 19:04+2К сожалению код ревью зачастую используется как способ «коллективной безответсвенности», снять ответсвенность — типа я послал на ревью более знающим товарищам, если и они не поймали, то я уж тем более бессилен был. Особенно это касается логики реализации.
Плюс если кто-то хорошо делает ревью, то будут на этом ездить и присылать плохой код — всё равно ревьюер поймает и тогда поправлю, мне меньше думать. Также как и на хорошем тестере ездят, присылая сырые версии. Понятно что им так легче, но поэтому быть хорошим ревьюером не самый хороший вариант.
Ещё одна из проблем — в отсутсвии масштабирования у код ревью, т.е. ты проревьюил чей-то код, но работа которая была проделана, фактически не переиспользуется. Следующий раз когда похожая ошибка будет сделана, предыдущий результат ревью не поможет её избежать как на этапе написания, так и на этапе следующего ревью.
Будет замечательно когда появятся инструменты которые интегрируют результаты ревью с IDE и анализаторами. Для PVS-Studio может быть хорошая идея ( Andrey2008 ), они уже имеют достаточно большой опыт в анализе кода, интеграциях с процессами — так что сверху добавить функциональность для ревью вполне возможно.turbobureaucrat Автор
19.12.2022 21:24Прекрасно написали. Вот потому я работаю на значительное сокращение времени процесса ревью, ведь часто открытие PR'а ощущается как «дело сделано».
Есть такой замечательный общий принцип — встраивание качества в процесс производства/разработки. Мне он кажется очень перспективным для нашей индустрии.
А что за инструменты?
AirLight
21.12.2022 23:23Этап между премкой задачи и ее кодированием, который в статье называется "видение решения", также имеет название "Solution Design", так же как и документ, получаемый в результате этого этапа. Ранее, я как тимлид просил писать такой документ только в случаях, когда сложность задачи выше среднего.
Однако, времена меняются. Сейчас, с появлением Github Copilot такой документ можно будет использовать как подсказчик. Поэтому, думаю, сделать его обязательным элементом решения задачи.
И, сейчас еще появился ChatGPT, его наверное тоже можно будет использовать. Как раз как помощник в написании таких документов. Но, этот момент требует изучения.
BugM
Заменяем ревью кода на его одной трети готовности на ревью архитектуры. Хоть просто на доске порисовать можно. Это типовой подход, если решение задачи вызывает вопросы.
А дежурные по ревью не работают. Чтобы они работали у всех кто участвует в дежурстве должны быть примерно одинаковые знания по всей кодой базе. Это увы недостижимый идеал. И ревью опять подвиснет на том кто лучше знает вот этот кусок приложения и может сказать на ревью что-то разумное. По типовым простым кускам обычно вопросов и так нет. Оптимизировать их ревью нет смысла. round robin назначающий ревьюера для таких мест отлично справляется.
turbobureaucrat Автор
Ревью архитектуры, о которым вы говорите и ревью решения, упомянутое в статье — это разные ревью или речь об одном?
Да, хорошо отметили, по умолчанию такие ревью не работают. Но можно задать себе вопрос: «А правильно ли, что у меня для разных частей кодовой базы нужны разные ревьюеры?» И покопать в этом направлении. Архитектора помучать, например, пускай о консистентности подумает.????
BugM
Это ваше ревью трети готового кода. К этому моменту разработчик обычно уже понимает что он делает и если у него есть сомнения он готов рассказать что делать собирается и выслушать фидбек.
Неправильно но что поделать.
Готов исправить. Не подскажете где взять хотя бы десяток разработчиков которые не только сами AST умеют перекладывать, но и могут понять и оценить качество чужого кода перекладывающего AST? Если слова "дифференциальная база данных" у них будут находить какой-то отклик то вообще хорошо.
Таких у нас нет. И не надо. Код в продакшен пишут все технари. Не пишут уже только большие руководители разработки.
Ваш архитектор точно архитектуру перекладываля AST поймет и сможет сделать хотя бы не хуже? А дифференциальную базу спроектирует или исправит проблемы в существующей? Там есть некоторые вопросы к импотентности изменений. Просто их точно не исправить.
turbobureaucrat Автор
Я надеюсь, речь всё-таки об идемпотентности изменений?????
Про всё остальное единственное, что приходит в голову — образование на рабочем месте. Тоже процесс, тоже можно заниматься его непрерывным совершенствованием.
А критерии качества кода нужно явно формулировать. Например, через задание требуемых качественных характеристик проектируемой системе. Это как раз вотчина архитектора.
BugM
Ой :)
Люди имеют свойство уходить и приходить. 4 года вроде нормальный медианный срок работы? Чему-то серьезно учить в рабочее время просто нет смысла, оно не окупится никогда.
Архитектор такими общими словами говорит? И зачем он нужен тогда? Делай хорошо, не делай плохо, вот стайлгайд и линтер я и сам вещать могу часами.
Есть конкретный перекладыватель AST, у него есть архитектура. Довольно старая, но хорошо работает. Объем кода там приличный. Слова функтор, изоморфное преобразование, монада встречаются в комментариях заметно больше одного раза. Есть конкретные задачи. Из попроще визитор добавить какой. Из посложнее оптимизировать AST и выхлоп. Там точно можно, я знаю. Но это совсем неочевидно. Переписать, но более гибко понятно вообще лучше наверно тоже можно. Многолетние наслоения дают о себе знать.
Или дифференциальная база данных. Написана на довольно странных технологиях. Тоже с приличным таким объемом кода. У нее есть архитектурные проблемы и та же задача оптимизации. Идемпотентность это самая часто стреляющая проблема. На самом деле еще есть проблемы всякие.
Ваш архитектор решит эти задачи? Я понимаю что полностью их решить нельзя, но он хотя бы сделает так что станет заметно лучше?
turbobureaucrat Автор
Да, действительно приходят и уходят. Но тут тоже можно разбираться. Уходят же не просто так, а из-за каких-то моментов: карьерного потолка, нежелания компании работать над техническим совершенство своего программного обеспечения и другого.
По поводу архитектора ПО. Я не настоящий сварщик и хорошо бы, если бы вам ответили действительно опытные в этом вопросе люди. Но одной из задач архитектора является составление набора качественных требований к системе с конкретными значениями. Это могут быть требования к производительности, аптайму и ко многим другим характеристикам. По идее, архитекор должен быть настолько влиятельным, чтобы иметь возможность выступать в качестве постановщика задач для команды. А там от постановки задач выделяется время на то, чтобы команда, да и сам архитектор переосмыслили бы весь набор функторов, изоморфных преобразований и прочего, чтобы, например, вдвое увеличить производительность.
BugM
Спасибо что подтвердили бесполезность архитектора.
Вещать общие слова что работать надо хорошо, плохо не надо, зубы чистить надо, падать не надо, а если упали то надо это делать с контролируемым и минимальным радиусом поражения, в душ ходить надо каждый день я все еще могу сам. Непонятно только зачем.
Проблема не в этом, а в то как это все сделать вот в таких реальных условиях. Непадающий и нетормозящий круд сделать несложно, проблемы они со сложным кодом. И проблема с людьми кто умеет с ним работать и тем более его проектировать.
visirok
Культура архитектуры как-то ушла, не развившись. Она живёт кое-где в кровавом энтерпрейзе, а на уровне стартапов архитекторов или нет или это болтуны и жулики.
Но в прошлом веке и в начале нулевых годов культура была. Процветал UML. Хотя его мало кто понимал и часто применяли неправильно.
Но это было и при правильном использовании было эффективнее, чем сейчас.
Архитектура позволяет структурировать и специализировать код. И тогда его становится меньше. И code review начинает означать совсем другое.
rsashka
Вы путаете роль "архитектор" и конкретного человека.
Задачи для роли архитектор есть всегда. Просто иногда они решаются самим разработчиками, тем более, когда нет выделенного человека с ролью "архитектор".