Comments 45
Заменяем ревью кода на его одной трети готовности на ревью архитектуры. Хоть просто на доске порисовать можно. Это типовой подход, если решение задачи вызывает вопросы.
А дежурные по ревью не работают. Чтобы они работали у всех кто участвует в дежурстве должны быть примерно одинаковые знания по всей кодой базе. Это увы недостижимый идеал. И ревью опять подвиснет на том кто лучше знает вот этот кусок приложения и может сказать на ревью что-то разумное. По типовым простым кускам обычно вопросов и так нет. Оптимизировать их ревью нет смысла. round robin назначающий ревьюера для таких мест отлично справляется.
Ревью архитектуры, о которым вы говорите и ревью решения, упомянутое в статье — это разные ревью или речь об одном?
Да, хорошо отметили, по умолчанию такие ревью не работают. Но можно задать себе вопрос: «А правильно ли, что у меня для разных частей кодовой базы нужны разные ревьюеры?» И покопать в этом направлении. Архитектора помучать, например, пускай о консистентности подумает.?
Ревью архитектуры, о которым вы говорите и ревью решения, упомянутое в статье — это разные ревью или речь об одном?
Это ваше ревью трети готового кода. К этому моменту разработчик обычно уже понимает что он делает и если у него есть сомнения он готов рассказать что делать собирается и выслушать фидбек.
А правильно ли, что у меня для разных частей кодовой базы нужны разные ревьюеры?
Неправильно но что поделать.
Готов исправить. Не подскажете где взять хотя бы десяток разработчиков которые не только сами AST умеют перекладывать, но и могут понять и оценить качество чужого кода перекладывающего AST? Если слова "дифференциальная база данных" у них будут находить какой-то отклик то вообще хорошо.
Архитектора помучать, например, пускай о консистентности подумает
Таких у нас нет. И не надо. Код в продакшен пишут все технари. Не пишут уже только большие руководители разработки.
Ваш архитектор точно архитектуру перекладываля AST поймет и сможет сделать хотя бы не хуже? А дифференциальную базу спроектирует или исправит проблемы в существующей? Там есть некоторые вопросы к импотентности изменений. Просто их точно не исправить.
Я надеюсь, речь всё-таки об идемпотентности изменений??
Про всё остальное единственное, что приходит в голову — образование на рабочем месте. Тоже процесс, тоже можно заниматься его непрерывным совершенствованием.
А критерии качества кода нужно явно формулировать. Например, через задание требуемых качественных характеристик проектируемой системе. Это как раз вотчина архитектора.
Я надеюсь, речь всё-таки об идемпотентности изменений??
Ой :)
Про всё остальное единственное, что приходит в голову — образование на рабочем месте. Тоже процесс, тоже можно заниматься его непрерывным совершенствованием.
Люди имеют свойство уходить и приходить. 4 года вроде нормальный медианный срок работы? Чему-то серьезно учить в рабочее время просто нет смысла, оно не окупится никогда.
А критерии качества кода нужно явно формулировать. Например, через задание требуемых качественных характеристик проектируемой системе. Это как раз вотчина архитектора.
Архитектор такими общими словами говорит? И зачем он нужен тогда? Делай хорошо, не делай плохо, вот стайлгайд и линтер я и сам вещать могу часами.
Есть конкретный перекладыватель AST, у него есть архитектура. Довольно старая, но хорошо работает. Объем кода там приличный. Слова функтор, изоморфное преобразование, монада встречаются в комментариях заметно больше одного раза. Есть конкретные задачи. Из попроще визитор добавить какой. Из посложнее оптимизировать AST и выхлоп. Там точно можно, я знаю. Но это совсем неочевидно. Переписать, но более гибко понятно вообще лучше наверно тоже можно. Многолетние наслоения дают о себе знать.
Или дифференциальная база данных. Написана на довольно странных технологиях. Тоже с приличным таким объемом кода. У нее есть архитектурные проблемы и та же задача оптимизации. Идемпотентность это самая часто стреляющая проблема. На самом деле еще есть проблемы всякие.
Ваш архитектор решит эти задачи? Я понимаю что полностью их решить нельзя, но он хотя бы сделает так что станет заметно лучше?
Да, действительно приходят и уходят. Но тут тоже можно разбираться. Уходят же не просто так, а из-за каких-то моментов: карьерного потолка, нежелания компании работать над техническим совершенство своего программного обеспечения и другого.
По поводу архитектора ПО. Я не настоящий сварщик и хорошо бы, если бы вам ответили действительно опытные в этом вопросе люди. Но одной из задач архитектора является составление набора качественных требований к системе с конкретными значениями. Это могут быть требования к производительности, аптайму и ко многим другим характеристикам. По идее, архитекор должен быть настолько влиятельным, чтобы иметь возможность выступать в качестве постановщика задач для команды. А там от постановки задач выделяется время на то, чтобы команда, да и сам архитектор переосмыслили бы весь набор функторов, изоморфных преобразований и прочего, чтобы, например, вдвое увеличить производительность.
Спасибо что подтвердили бесполезность архитектора.
Вещать общие слова что работать надо хорошо, плохо не надо, зубы чистить надо, падать не надо, а если упали то надо это делать с контролируемым и минимальным радиусом поражения, в душ ходить надо каждый день я все еще могу сам. Непонятно только зачем.
Проблема не в этом, а в то как это все сделать вот в таких реальных условиях. Непадающий и нетормозящий круд сделать несложно, проблемы они со сложным кодом. И проблема с людьми кто умеет с ним работать и тем более его проектировать.
Культура архитектуры как-то ушла, не развившись. Она живёт кое-где в кровавом энтерпрейзе, а на уровне стартапов архитекторов или нет или это болтуны и жулики.
Но в прошлом веке и в начале нулевых годов культура была. Процветал UML. Хотя его мало кто понимал и часто применяли неправильно.
Но это было и при правильном использовании было эффективнее, чем сейчас.
Архитектура позволяет структурировать и специализировать код. И тогда его становится меньше. И code review начинает означать совсем другое.
Вы путаете роль "архитектор" и конкретного человека.
Задачи для роли архитектор есть всегда. Просто иногда они решаются самим разработчиками, тем более, когда нет выделенного человека с ролью "архитектор".
В маленькой команде, где у всех относительно неплохие знания обо всём (4, максимум 5 человек), такая схема будет работать, но будет обходиться очень дорого, потому что "дежурный по ревью" - это будет 20-25% ресурса; если не предполагать, что в большинстве случаев никаких доработок и не потребуется и большинство времени человек будет не ревьюить, а заниматься какими-то своими задачами - но в этом случае и никакой страшно нарисованной проблемы задержек и бесконечных циклов не будет - проверяющий быстро посмотрит, скажет ок и вернется к своим делам. Ревью откладывается на потом именно в командах, где в большинстве случаев это трудный, долгий и мучительный процесс с большим количеством итераций.
А в большой команде, где ревью много, а знания о системе распределены неравномерно, процесс, построенный по описанию в статье, выродится в одну из двух сторон в зависимости от степени добросовестности ревьюера. Либо это будет чисто формальная процедура, которая никаких багов конечно не предотвратит, кроме самых явных и тупых - будет проверено соответствие кода формальным критериям, имен переменных соглашениям и т.п.: то, что мог бы сделать линтер и человек для этого не нужен. Либо ревьюер, сидя над кодом часами - сначала сам разбираясь в нём, потом разбираясь вместе с автором, как его исправить - сам станет узким место процесса, потому что пока он ковыряется в одном реквесте, там его уже ждут ещё 5, а он должен закончить этот, чтобы бесконечных циклов и вываливаний из потока не произошло
В общем, там где описанное выглядит хорошо - нет проблемы, которую надо решать, а там где есть проблема - там оно хорошо не выглядит
Кроме того, в финальной диаграмме цикл просто из явной формы переведен в неявную, так как никак не детализировано, каким же образом будет происходить "парная доработка". А он будет описываться такой же диаграммой, только быть может без явного открытия PR.
Так там и вторая задержка никуда не делась, просто автор ее решил не рисовать для "красоты". Кто сказал что программисту удобно делать парное ревью когда скажет ревьюер? Вот и добавьте время на "договориться когда созвониться"
Да, это время стоит добавить. Но стоит добавить его в пункт «Когда ревьюер освобождается от прочих дел», то есть в первую и единственную задержку. В том виде, в котором я сейчас внедряю процесс, предполагается, что ревью происходит синхронно. То есть и ревьюер, и автор кода должны быть доступны в течение ближайшего часа после открытия PR'а.
Парная разработка происходит уже после открытия PR'а. Ревьюер и автор кода созваниваются, производят доработки. Можно просто через Zoom, можно через более подходящие для этого средства. У VS Code и IntelliJ, например, есть плагины для удалённой работы.
Спасибо за содержательный комментарий. В своей команде я сейчас провожу эксперимент «ревью за час». Его задача состоит в том, чтобы обнаружить продолжительные активности, случающиеся в время ревью кода, и явно передвинуть их на этап разработки. Таким образом мы пытаемся уйти и от узости горлышка, и от того, что на ревью будут проскакивать серьёзные ошибки.
Команда большая, 20 человек.
В команде из 20 человек у вас один дежурный превращается в продвинутый линтер скорее всего. Он не знает архитектуры этой части системы с высокой долей вероятности, он не знает подводных камней и нюансов, он не понимает этого фрагмента предметной области как следует. В итоге либо он на всё это забивает и проверяет код по чеклисту; либо автор кода ему всё это рассказывает долго и мучительно (другие авторы в это время ждут) - и хорошо если ревьюер честно просит это сделать, а не начинает выдавать замечания одно бесячее другого; либо если автор примерно таким же уровнем знаний обладает, то они совместно демонстрируют пример эффекта Даннинга-Крюгера и плохой код улетает в продакшен
Ревью кода - конечно несовершенный процесс, но его не сделаешь лучше, выплеснув вместе с его недостатками его достоинства
Кстати, у меня есть довольно хороший критерий непродуктивного ревью - когда в его ходе возникает очень много замечаний к коду не этого реквеста, а к коду, который находится в эпсилон-окрестности вокруг него. Это означает, что либо ревьюер в первый раз в жизни этот модуль видит вообще, либо что он вообще не понимает что и зачем тут написано и начал изучать всё вокруг чтобы понять
И ещё один момент. Вы пишете о важном качестве — добросовестности. Это прекрасная характеристика, но, к сожалению, непостоянная. У кого-то добросовестность означает один результат, у кого-то другой, да и от типа задачи зависит. Вот эту самую добросовестность хочется ожидать не от конкретного человека, а встроить в сам процесс разработки, чтобы он выдавал добросовестный результат.
Добросовестность - это черта характера, она либо есть либо нет. Встроить в процесс ревью вы можете только мотивацию на тщательный поиск дефектов. Это можно сделать разными способами, и любой из них не универсален, так что это довольно сложно. Но я вообще не вижу, какое отношение к этому имеет описанное в статье изменение
У автора кода имеется своё видение. В большинстве случаев автор даже не
знает о том, что именно хотят наиболее уважаемые товарищи
Ну значит имеем косямбу то ли от товарищей, то ли от автора. Написать гайд по основным моментам - невеликий труд. Прочитать его и уточнить неясности - тоже.
Далее начинается ожидание, и это самый интересный для нас этап.
Продолжительность такого ожидания неизвестна и зависит от загруженности
ревьюера.
Договоритесь об SLA на данную процедуру среди своих коллег. Например, МР не может висеть без внимания больше рабочего дня. Закрепите соглашение на общем собрании, а потом карайте нарушителей.
Ведь автор кода свернул на кривую дорожку уже достаточно давно и уже долго продвигался по ней, принимая одно решение за другим.
Судя по контексту, автор кода свернул не туда, когда решил запихнуть всю задачу в один МР размером с кита. Это решается декомпозицией задачи. У меня была такая проблема в одном стартапе, куда разом запустили трех джунов. После нескольких жестких разборов полетов и одного увольнения все чудом научились делать МРы с более скромными скоупами.
В общем я вижу в вашей истории проблему не с ревью, а с коммуникациями в команде и с общей культурой разработки.
Ну значит имеем косямбу то ли от товарищей, то ли от автора. Написать
гайд по основным моментам - невеликий труд. Прочитать его и уточнить
неясности - тоже.
Частично соглашусь в этом моменте. Хочу заметить при этом, что ограничений обычно достаточно много, и гайд может и не смочь закрыть все моменты. Даже если он и будет где-то находиться и покрывать необходимые моменты, то в каких-то решениях о нём могут забыть.
Договоритесь об SLA на данную процедуру среди своих коллег. Например, МР
не может висеть без внимания больше рабочего дня. Закрепите соглашение
на общем собрании, а потом карайте нарушителей.
Про кару нарушителей — весёлый момент.? В подавляющем большинстве случаев этот момент зависит не от разработчиков, а от устройства процесса разработки, то есть системы. Поэтому карательные усилия я рекомендую направлять на его преобразование.
Судя по контексту, автор кода свернул не туда, когда решил запихнуть всю задачу в один МР размером с кита. Это решается декомпозицией задачи. У меня была такая проблема в одном стартапе, куда разом запустили трех джунов. После нескольких жестких разборов полетов и одного увольнения все чудом научились делать МРы с более скромными скоупами.
В общем я вижу в вашей истории проблему не с ревью, а с коммуникациями в команде и с общей культурой разработки.
Да, про декомпозицию задачи — хорошее замечание, тут я согласен. Подскажите, пожалуйста, о команде какого размера вы говорите? Думаю, что это может иметь значение.
С документацией, как правило, всегда сложно и это вообще отдельный вопрос, как ее вести, чтобы она экономила больше времени, чем тратила.
Про кары - это, конечно, смешно, да.
По поводу декомпозиции могу только сказать, как этого иногда можно попытаться достичь (работало только на тех проектах, где я был, так что может быть ошибка выжившего): идея в том, чтобы каждый кусок ревью собирался, проходил все тесты и вообще включал в себя как можно меньше разных кусков проекта. В идеале - чтобы разные ревью одного "большого ревью" могли проводить разные люди (не обязательно, чтобы это были разные люди). Это тратит немного больше времени разработчика на грамотное разбиение (и на поддержку последующих кусков ревью, если вдруг на них могут повлиять итоги ревью предыдущих), но сильно разгружает ревьювера, ТК ему можно загружать в голову намного меньше контекста и можно проводить ревью небольшими подходами.
Ревью бывают разными, о чем вы и начали писать в самом начале статьи. Но потом все смешали в одну кучу, вот у вас и получился вывод, что "ревью ненужно".
На самом деле важно осознавать, что разные виды ревью требуют разных подходов и разных аудиторий. В ревью для 10 строчек кода действительно можно найти 10 ошибок.
А вот ревью на 5000 строк может пройти без проблем, но совершенно по другой причине. Все понимают, что такой объем кода быстро проанализировать невозможно, поэтому естественно "режим" ревью будет отличаться от "режима" для 10 строк.
Если в первом случае будет анализироваться каждая строка, то второй случай буде скорее ревью архитектурным на общую концепцию, а не на реализацию. А раз анализируется архитектура кода, то и мелкие косяки в реализации можно игнорировать (например, можно включить в большое ревью первые 10 строк кода и они с большой вероятностью пролетят без замечаний).
Вот кстати заметка, как можно организовать архитектурно ревью с помощью Хабра, который ума палата :-)
Да, хорошее замечание про различные объёмы PR'ов.
Для меня в работе важно иметь выровненный поток разработки, и поэтому для ревью на 5000 строк не ставится вопрос о целесообразности ревью. Ставится вопрос, каким образом нам так перепроектировать нашу деятельность, чтобы такие здоровые PR'ы не возникали.
Я не писал, что ревью не нужно, а то, что в качестве цели можно поставить себе отказ от него. Но для этого нужно будет серьёзно поработать.
За заметку спасибо!?
К вопросу инспекции кода (текста): "борцы за чистоту языка" различают понятия "методология" и "методика"
Оставлю свой майндмеп по код ревью здесь
Проблема тут как мне кажется не в том.
Работал я в проекте где было с десяток команд. Каждый реквест требовал как минимум 3 ревью (архитектор, аналитик, разработчик) иногда больше. И к тому же сначала из своей ветки в общую dev, потом в релиз.
И никаких особых проблем с этим не было. Если надо быстро - то пишешь, звонишь, связываешься другим способом с людьми и просишь посмотреть. Есть несогласие или недопонимание - пишешь, звонишь, связываешься другим способом с людьми и обсуждаешь.
Ну то есть если люди адекватные с обеих сторон, то коммуникации решают все вопросы и быстро.
ЗЫ: я вижу в ревью еще сильную обучающую модель.
Общая проблема подобных текстов - концентрация на мелочах.
Да, автор проделал полезную работу, но полезна она в узких рамках одного из множества процессов разработки. Очевидно, что оптимизируя всю разработку, то есть комплекс процессов, можно получить во много раз больший результат. Но как раз замахиваться на всё целиком желающих нет. Точнее на это замахивается молодёжь, но им абсолютно разумно и быстро бьют по рукам, ибо они навертят такого, что потом точно всё встанет.
Главный стопор, как всегда - бизнес. Им надо быстро, потому всё оптимизировать никогда не дадут, ибо дорого. В результате поощряется кусочная оптимизация, частичная автоматизация, а в целом - страшное убожество, если смотреть глобально.
С другой стороны, если и мелочью не заниматься, то получится вообще печально - никакого развития, даже в виде крайнего убожества. Но тратить время на убожество тоже ничуть не радостная перспектива. Отсюда мораль - бизнес обязан находить в себе ресурсы для какого-то оптимального улучшения самого себя. Проблема здесь следующая - бизнес платит деньги и ожидает, что за деньги ему сделают красиво. Но это, как большинству из нас отлично известно, явное заблуждение. Или отмазка, то есть самооправдание бизнеса и перекладывание вины с себя на работников. Вот такие отмазки и нужно вылечить. Чья это задача? Уж точно не программиста.
Ох, очень нравится мне ваш комментарий, с него и начну отвечать.?
Да, действительно эта статья концентрируется только на одной локальной оптимизации, которая в глобальном смысле может и не дать организации никакого преимущества. Но практикующему менеджеру она может быть полезна хотя бы с точки зрения понимания того, каким образом можно думать о процессе, как его дорабатывать.
К счастью, наличие статьи о некой локальной оптимизации не является свидетельством того, что я не осуществляю попытки производить и более глобальные оптимизации. Идут они с куда большим трением, но всё-таки идут. Например, процесс разработки интересных для пользователя эпиков ускорился в три раза просто потому, что мы стали тщательно собирать статистику. Не факт, что ускорение вообще было, возможно мы просто обнаружили действительные очертания системы, но тем не менее.
Однако из этой статистики видно, что КПД процесса разработки эпиков и после тройного ускорения в лучшем случае сейчас составляет 30%, и нам есть куда двигаться.
К слову, мне очень странно, что бизнес в этом не заинтересован. Ускорить разработку в 9 раз, как по мне, так очень бизнес-ориентированно.
Прекрасно написали. Вот потому я работаю на значительное сокращение времени процесса ревью, ведь часто открытие PR'а ощущается как «дело сделано».
Есть такой замечательный общий принцип — встраивание качества в процесс производства/разработки. Мне он кажется очень перспективным для нашей индустрии.
А что за инструменты?
Этап между премкой задачи и ее кодированием, который в статье называется "видение решения", также имеет название "Solution Design", так же как и документ, получаемый в результате этого этапа. Ранее, я как тимлид просил писать такой документ только в случаях, когда сложность задачи выше среднего.
Однако, времена меняются. Сейчас, с появлением Github Copilot такой документ можно будет использовать как подсказчик. Поэтому, думаю, сделать его обязательным элементом решения задачи.
И, сейчас еще появился ChatGPT, его наверное тоже можно будет использовать. Как раз как помощник в написании таких документов. Но, этот момент требует изучения.
В тексте статьи есть упоминание "Инспекция решения (design inspection);", но в итоге предложенное решение всё равно лечит симптомы, а не причины.
Вот этот этап должен быть ДО начала разработки. То есть "подумать над решением, предложить и обосновать (если надо)". На этом же этапе могут быть чеклисты, про что не забыть при имплементации (в т.ч. "если планируются тяжелые sql-запросы или многопоточка, то проконсультироваться/показать эксперту").
После апрува такого дизайна, обращаться к ревьюеру дизайна имеет смысл только в том случае, если во время имплементации стало понятно, что появился drift-решения от согласованного и нужно "пересогласование".
Далее необходимость в код-ревью в принципе отпадет, т.к. если выстроены процессы, где верификация работоспособности кода автоматическая - код проходит все необходимые acceptance-тесты и static analysis (aka "автоматические инспекции кода"), то нет смысла копаться в деталях имплементации. Код ревью становится опциональным и post-factum (ну, типа, разраб может попросить более опытного товарища посмотреть и дать фидбэк, если у автора остались какие-то сомнения)
Когда я работал в "кровавом энтерпрайзе" то у нас был этап "Технический Дизайн" где мы описывали суть изменений которые собираемся сделать (чем короче и понятнее - тем лучше). Потом этот дизайн передавался архитектору - он его смотрел и если что-то не нравится то оставлял коменты и "аппрувил". Только после этого начинался кодинг - и само ревью было минимальным.
(архитектор тоже писал дизайн и код, советуясь с синьорами)
Как думаете это норм подход в современных реалиях?
Откуда вообще пошла эта история с инспекциями кода? Дядюшка Боб Мартин любезно предоставил мне ссылку на то, что можно считать первым документальным свидетельством существования инспекций кода. Майкл Фэган (Michael Fagan) задокументировал информацию о них в своей статье 1976-го года Design and code inspections to reduce errors in program development.
Интересно стало, что с этим моментом. Википедия говорит "подробно описан", читай: первый, кто решил написать книгу.
The historically first code review process that was studied and described in detail was called "Inspection" by its inventor Michael Fagan.[7]
Но вот интернет породил оцифровку учебного фильма 1975г. "Critical Program Reading" - с одной стороны под этим названием описывается в т.ч. рефакторинг кода, а с другой стороны - только ли метод представления или нет - по сути представляется ревью кода тремя людьми + ведущий. (По-моему еще именно названо "code review" где-то в видео)
Поиск в Google Books по "Critical Program Reading" показал много результатов из книг после публикации видео, но вот и ссылка на книгу 1979г. "Ethnotechnical Review Handbook":
Ethnotech, Inc.
Critical Program Reading, Course Notes, 1972
These notes concern methods of code review. Their essence is contained in the IPTO Support Group report...
Предполагаю, что термин code review был популяризирован именно Фаганом, но использовался он и ранее. И еще раньше, наверно, под другим именем. Называть его inventor... необъективный такой перебор, Википедия.
Процесс ревью кода структурно порочен. Вот, как его исправить