Нужны ваши мысли по code review

частые конфликты при мерджах являются индикатором того что что-то идет не так.

Угу,например когда идет рефакторинг или какие-то глобальные изменения по всему проекту из-за внешних факторов, например резкое изменение в одной из используемых библиотек или остановка ее разработки, что требует переезда на что-то еще.

коммиты лежат и ждут своего часа до мерджа неделями, иногда - месяцами, при достаточно активной разработке (косяк в методологии работы).

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

получается все свести до 1-2 конфликтов в месяц и меньше.

Это своего рода «дневная норма выработки», показатель что проект развивается по плану и идет в заданном направлении.

Но опять же, проект ведь не существует в вакууме и поскольку сама среда постоянно меняется - рано или поздно все летит к чертям.

Дело не в конкретно моих талантах, поскольку речь все же о разработке софта для проведения ревью.

Тут необходимо абстрагироваться и поставить кого-то попроще как на место ревьюера так и на место коммитера, чтобы адекватно оценить весь процесс.

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

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

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

Ближе к QA по духу ) Но я про другое - про то как и почему переписка превращается в бесконечную.

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

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

Для народа что-нибудь сделай лучше, а не для программистов. Что-то облегчающее жизнь рабочих и служащих, а не очередную хрень для задротов.

По ходу работы ты увидел, что колонка в БД выбивается из общего стиля именования и изменил ее имя на «правильное»

После чего немедленно умер от рук DBA, у которого сломалось штук 10 отчетов, использующих эту колонку.

Вообщем такое точно делать не стоит, тем более закладывать какой-либо функционал )

За редкими исключениями вроде R&D-отделов, качество команды разработки будет примерно одинаковым, что в Гугле что в каком-нибудь Люксофте.

Небо и земля. Разница примерно такая же как между Арсеналом и ярославским Шинником.

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

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

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

Вход в B2C это сразу так $10 млн бюджета, окстись )

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

Разница примерно такая же как между Арсеналом и ярославским Шинником.

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

После чего немедленно умер от рук DBA, у которого сломалось штук 10 отчетов, использующих эту колонку.

Ну послушай, я тебе просто пример привел не ради переименования колонки а ради того, что при разработке принимается 100500 решений, и далеко не все они достойны быть увековечены в комментариях.
Хочешь другой пример? — переименовал колонку и написал в PR «пацаны, я с DBA утряс изменения, не переживайте, от этого все не развалится».

Вообщем такое точно делать не стоит, тем более закладывать какой-либо функционал )

АПИ постоянно меняется, параметры переименовываются, удаляются, добавляются. То же самое и с БД.

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

Да обычные команды. Ты хочешь сказать, что в Люксе и Гугле в среднем одинаковый уровень разработчиков?

Есть две стадии: «ранний этап» жизни проекта, когда вся описанная тобой дичь спокойно творится и «доработка чего-то живого».

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

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

Разумеется все написанное про более-менее серьезный проект, не про внутреннюю автоматизацию в компании.

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

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

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

речь все же о разработке софта для проведения ревью.

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

Что делает подобное ручное ревью?

• проверка форматирования? Это уже сделал форматтер.
• проверка правил по котором должен быть написан код? Это уже сделал линтер.
• проверка что нет конфликтов? Это уже сделал git
• проверка не уронит ли изменение прод? Это уже сделали автоматизированные тесты.
• проверки количества изменений? Это уже сделал git diff и git blame.

Что остается?

Остается 2 вещи на мой взгляд:

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

2. Проверка насколько сильно человек корежил именно это место в коде в последнее время т.к. не был уверен что сделал верно или не знал.

Т.е. такой ревьювер нужен тому кому приходится просматривать коммиты джунов и может, первое время, новых мидлов на проекте.

Исходя из этого пожелания к интерфейсу (при условии что используется git):

1. Светла/темная темы. Глаза не казенные.
2. Цвет и толщина объектов которыми рисовать поверх кода (линия, прямоугольник, стрелка и тд)
3. Цвет и размер текста которым писать поверх кода.
4. Возможность вывода 10-20 строк кода выше/ниже измененного.
5. Возможность вывода слева построчно автора текущей версии кода
6. Автоматическое изменение фона строк кода которые были изменены больше х раз за последнюю неделю этим человеком как индикатор что это место нужно вдумчиво просмотреть.

Это в первом приближении. Более детально вам сможет подсказать тот кто постоянно с джунами работает.

Еще раз: на проектах где задействовано много народу срабатывает закон Парето и какой-то нереальной «общей» разницы в результатах не будет.

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

Маленькая команда суперпрофи бьет и маленькую и большую команду дебилов, большая команда где есть и профи и дебилы примерно равна команде из одних дебилов.

В какой-то мере это еще и эффект «бочки меда и ложки говна».

написал в PR «пацаны, я с DBA утряс изменения, не переживайте, от этого все не развалится».

А с автоматизированными тестами которые не дают применить код если зафейлятся вы тоже утрясли? :)

Есть две стадии: «ранний этап» жизни проекта, когда вся описанная тобой дичь спокойно творится и «доработка чего-то живого».

Все проекты живут и меняются. Любой проект берешь, хоть GNU coreutils, хоть Spring и там постоянно меняется API. Да, не со скоростью звука, но меняется и объявляется устаревшим.

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

Это решается миграциями. От тривиальных, в духе `alter table rename` и до того, что данные мигрируют постепенно, в несколько этапов.

Ощущение, что ты говоришь про какие-то коробочные проекты, или проекты что написаны, отданы заказчику и забыты.

Это в первом приближении.

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

По-сути сейчас речь про глубокую интеграцию с git и проведение ревью в рамках коммита или пул-реквеста а не одного куска текста.

хоть Spring и там постоянно меняется API. Да, не со скоростью звука, но меняется и объявляется устаревшим.

Там на самом деле целый ритуал, с версиями и обновлением документации. Насколько я знаю, у команды Spring есть отдельный тулчейн для формирования версии, поскольку там задействована и документация и примеры и смежные библиотеки. Вообщем все сложно.

Ощущение, что ты говоришь про какие-то коробочные проекты, или проекты что написаны, отданы заказчику и забыты.

В основном - да, их просто большинство.

Остается 2 вещи на мой взгляд:

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

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

Как по мне, то главное в аудите это проверка решения. А именно, правильно ли задание решено, совпадает ли решение с ожиданиями заказчика (в роли которого выступает бригадир или ПМ).

По-сути сейчас речь про глубокую интеграцию с git и проведение ревью в рамках коммита или пул-реквеста а не одного куска текста.

Тогда обратите внимание на возомжности sublime merge. Он немного перегружен, но в нем есть все что нужно по ревью кода кроме рисования поверх и автоизменения фона часто изменяемых кусков кода.

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

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

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

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

Еще раз: на проектах где задействовано много народу срабатывает закон Парето и какой-то нереальной «общей» разницы в результатах не будет.

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

Это же чистый клиент, в таком стиле конечно тоже можно реализовать всю затею с рисованием, но не очень понятно что в этом случае дальше делать с результатом ревью - в файл локально? Как-то мало.

Свой сервер дает больше возможностей в этом плане.

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

Как по мне, то главное в аудите это проверка решения. А именно, правильно ли задание решено, совпадает ли решение с ожиданиями заказчика (в роли которого выступает бригадир или ПМ).

Обычно, у ревьювера есть свои критерии необходимости и достаточности качества решения чтобы утвердить его. Никто не требует идеальный вариант на 5 баллов, более того, бизнесу (заказчику) наплевать на идеальный/лучший вариант, его устроит 3 балла, но сегодня. Программиста также это устроит.

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

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

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

Ну как разный, когда на поле играют одно и тоже количество людей )

Футбольная команда - тот же спецназ, разумеется что в Арсенале он лучше.

Но стоит этому спецназу дать в нагрузку 100 человек с вилами и от профессионализма ничего не останется, все силы уйдут на организацию процесса работы (бега и забивания мяча), который будет усреднен и упрощен, чтоб все смогли его делать.

К стати, вспомнил чего мне временами не хватает в Github. Там нет возможности оставить комментарий произвольному куску кода, только тем которые меняли.
Глупо, как по мне. Вот хочу я попросить автора изменить еще строку №152. Или хочу его вопросить проверить, не сломается или условие в строке №153. А если еще и в другом файле, который он даже не менял?

Линус наш, Торвальдс, по этому поводу хорошо на ГХ гонит.

Это же чистый клиент

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

Со своим сервером, из фич можно еще добавить теги. Т.е. возможность создать набор тегов с проблемами которые найдены и назначать их на ревью.

Эти теги не видит тот кто писал код. Это для того чтобы потом по всем программистам можно было сформировать у кого какие теги вылезли и понять кого на какие курсы/занятия/ беседу в каптерку по рекурсии с проказником архитектом (это еще и как обоснования если хотите у себя в конторе бюджет на этой выбить - отлавливаем кто что плохо знает и доучиваем именно этому не тратя время и деньги на остальное). Т.к. это явно для галер, то и фичи соответствующие.

Вот хочу я попросить автора изменить еще строку №152.

Для этого же есть комментарии как в самом Issue так и комментарии в коммите.

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

Понятно, ну что тут сказать - они повторили тренд из VS Code по внешнему оформлению, не все решения которого считаю удачными.

Из того что увидел: снова сильная ориентация на коммиты, есть «стандартное красивое» отображение веток в репозитории, которое на практике всегда бесполезно.

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

А что хотя-бы зашло лично вам?

А что хотя-бы зашло лично вам?

Я не являюсь вашей ЦА т.к. не работаю с джунами, а реьвью кода у спецназа проходит тогда когда появляется какая-то «мистика» что довольно редко.

В идеале, вам бы найти кого-то кто постоянно делает эти ревью (в вашей конторе или еще где) и напроситься чай попить - рядом посидеть в процессе. Посмотреть на процесс со стороны, обычно тогда всплывают какие-то очевидные моменты которые почему-то при размышлении «а что нужно?» как-то проскакивают мимо. Это касается не только ревью, а любой сферы которую предстоит автоматизировать.

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

Он был изначально отделен от репозитория и механизма коммитов, поскольку когда начинал было несколько разных систем контроля версий на разных проектах. Был mercurial, subversion и git, достаточно разные внутри, со своей спецификой.

Плюс много чего начиналось не со стороны исходного кода, а с куска лога со stacktrace внутри, отсюда и все эти рисунки - ими обводились проблемные места в таких трейсах.

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

Если деревенскому недоучке пришить погоны на пиджак, надеть фуражку на голову и выдать гаишную дубинку в руки, то ему найти повод для нарушения правил дорожного движения будет очень просто к каждому второму водителю на дороге. «Грязью» в коде может быть не только оформление, а не тот ход мысли, не тот способ получения результата. Любую программу можно написать несколькими способами. Какой из них будет правильным, если требования удовлетворены и тесты успешно пройдены? Если надсмотрщик в душе - гнилой человек, то своей «дубинкой» он будет махать в любом случае.

Ущербный человек мыслит так: «Я не пропущу изменения в код этого инженера, а потом выставлю его тупым лентяем перед руководством, потому как внесенных изменений к общую ветку кода от этого товарища не будет». Почему этот человек так поступает? Он сам натерпелся подобного унижения и понимает только жесткую вертикаль в служебных отношениях: я - начальник, ты - дурак. Разговаривать на равных такие люди не умеют. Раб не мечтает о свободе, раб хочет иметь своих рабов.

Если кто-то столкнулся с подобным отношением к себе на работе, то не молчите - в этом случае придётся уволиться с позором неумехи. Ради выживания придётся пойти на открытое противостояние с задротом. Собирайте совещание руководства и говорите открыто, что этот товарищ мешает работать. Только сначала добейтесь работоспособности своих изменений в тестах. Как запустить тесты, если задрот не позволяет внести изменения в общую ветку кода и их запустить? Делайте свой тестовый сервер или тестируйте на другом сервере. Главное - показать результат работоспособности своих изменений кода. Не бойтесь открытого противостояния. Если проиграете, то уволитесь, а если промолчите - тоже, но позорно. Тут хоть душу отведёте в открытом бою.

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

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

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

Да, но так же можно и все замечания просто в общий комментарий засунуть. Неудобно.

сейчас речь про глубокую интеграцию с git

кстати что планируете использовать для интеграции? jgit, вызов бинарника или ещё что-то?

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

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

Нужны ваши мысли по code review

Не до чего докопаться — докопайся до скобочек и пробелов :)

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

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

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

Старая школа - лучшая школа ;)

а как иначе обеспечить качество кода?

Не до чего докопаться — докопайся до скобочек и пробелов :)

Я хоть такие замечания стараюсь не писать, но меня каждый раз аж трисёт, когда вижу `if(expression) {`. Сейчас есть коллега, у которого так каждая пятая строка.
Повбивав би. (c)

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

Ну меня такое читать сильно раздражает. Это как читать книгу, в которой каждая N-я буква смещена относительно горизонта.
Плюс, при исправлении, это постоянно делает ненужный шум в последующих ПРах.
Прикрутить Checkstyle к старому проекту дело далеко не тривиальное.

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

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

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

Это лечится https://youtu.be/F3AIE8iWomI?si=zjMqTj_J-o3iJ0aq