Comments 63
Я обычно на ревью пытаюсь понять, что человек сделал, зачем и как. Если я не могу этого понять, то ревью failed (либо я не подходящий ревьюер, потому что я не знаю проекта, либо объём изменений слишком большой).
... На самом деле есть два вида ревью - по "речекрякам" и "по сути". По сути ревью может делать только человек, глубоко вовлечённый в проект, который понимает куда проект идёт и как он построен.
Ревью на речекряки проще - мы смотрим, читаем ли код, нет ли странных выкрутасов (когда есть простые решения) и т.д.
Но ревью "по сути" может занимать времени не меньше, чем время написания MR, хотя оно самое продуктивное (как для проекта, так и для читающего, и для пишущего).
Вопрос интересный. Достоин отдельного поста. Подумаю как кратко сформулировать и отвечу позже.
Тут конкретный ответ зависит от технологического стека.
В общем стоит настроить автоматическое форматирование и анализаторы кода как статические так и динамические на каждый комит ну и конечно запуск различных тестов.
Тогда весь код будет единообразно выглядеть и не будут проходить мелкие недочеты.
Кстати, это применимо также и если проводится ревью кода, чтобы не тратить время на болтовню вроде «запятая не там стоит», «а оно точно не падает?» и т.п.
Для JS прежде всего нужен TypeScript , решает добрую половину всяких опечаток :)
А далее Prettier или аналоги для форматтирования.
Как минимум ESLint для однообразия кода и всяких правил, или что-нибудь похожее.
Для .NET , я так понимаю это C# ?
Сегодня уже много встроенно в сам dotnet tool, скажем есть dotnet formatter.
Есть уйма анализаторов кода icrosoft.VisualStudio.Threading.Analyzers , Roslynator.Analyzers и другие.
А также конечно ReSharper с форматированием и анализом кода.
Конечно не лишним будет напомнить про .editorconfig с которым умеют работать все редакторы. В нём много расширений для правил dotnet , ReSharper и других утилит.
P.S.
Тут коллеги подсказали про небольшой хак с прекомит хуками.
Чтобы их не нужно было настраивать каждый раз вручную, есть https://pre-commit.com/ , который это сделает автоматически для вас.
Ко всему уже насоветованному добавлю, что если нет код ревью, то я б сосредоточилась на других способах улучшения качества: тестирование, continuous integration, статические и динамические анализаторы кода (если это применимо к .net и js).
Ну и стараться делать код более устойчивым к изменениям. В смысле, чтоб небольшое изменение не приводило к тому, что надо делать правки в десяти местах. Для этого помнить про SOLID и прочих GoF в процессе разработки.
Для .NET - https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/overview Это анализаторы самого кода, а для форматирования или, как уже подсказали, R#, или старый добрый https://www.nuget.org/packages/StyleCop.Analyzers/
Прикрутить автоматизацию: линтеры форматеры. Это вешается на прекоммит хуки и CI (если есть). Насроить всё это для питона оказалось достаточно затратно, но зато теперь таскаю это с собой.
На больших объемах кода тесты сильно ускоряют процесс. Но тут нюансов море.
Само-ревью. Пишешь код и перед тем как его отправить смотришь на него самостоятельно. Много мелких недочётов всплывает. Чем лучше память тем долше должен быть перерыв между написанием и ревью. Иначе будет чтение не с экрана, а из головы.
Я еще используею инкрементальный коммит в гите ( git add -i > 5: patch > * > yyy). В отличии от UI вариантов это не даёт проскролить в конец. Но тут стоит учесть, что я часто комичу.
Ибо баги в этом коде исправлять все равно тебе.
Не вижу логики - платит то
все равно компания. И программисту и пользователям, которые будут натыкаться на эти баги и репортить, пока их не исправят.
Ну т.е. внедряйте типизацию, внедряйте статический анализ, но имейте в виду, что скажем мне сонар каждый день предлагает в Java коде заменить лямбду ссылкой на метод. И в 99% случаев этот совет статического анализатора не имеет никакого смысла — потому что ссылка на метод предлагает переиспользование метода, а лямбда — что он уникален. И инструменты на сегодня достаточно тупы, и таких тонкостей не понимают. И правила у них зачастую достаточно тупы.
>Ибо баги в этом коде исправлять все равно тебе.
Ну я пару раз писал достаточно крупные проекты в одиночку. Благо, со сроками никто не гнал, и на то чтобы думать о качестве, было достаточно времени. Но review не было все равно, потому что не было больше никого, кто мог бы это делать. Очень часто хотелось посоветоваться, но прямо скажем, на качество это влияет не сильно. Сроки, а точнее их адекватность, влияют сильнее.
И в 99% случаев этот совет статического анализатора не имеет никакого смысла
Отключите это правило. Зачем тратить на это время?
Наработающие правила очень мешают при работе в команде, те кто еще не прониклись плюсами статического анализа спотыкаются на таких вещах и в результате весь код утыкан комментами для анализатора и появляется мнение: статический анализ зло.
> на качество это влияет не сильно
Код ревью это еще и обмен опытом и знаниями о проекте. Разница между "что они тут наворатили" и "я смотрел этот код" огромна.
То есть когда тебе нужно что-то сделать в коде, а его автор не доступен, ты можешь быстро включиться. Когда нет кодревью то вполне реален вариант: автор может поправить за 5 минут, а остальные за 2 дня.
Качество понятие относительное. То что хорошо для одиночки может быть плохо для команды.
Я как-то зарефакторил адапторы написанные на 80% мной из кучи функций в иерархию классов. Когда ученый, сделал нормальный адаптор вообще не задавая мне вопросов, я понял, что получилось нормально. Со старой версией он бы самостоятельно не справился. Старая версия писалась с внимательным код ревью.
Я не всегда это могу. Хотя это и неправильно.
С остальным практически полностью согласен. Одиночка тут как пример, когда ревью просто некому делать, и чтобы пояснить, что статический анализ и пр. его не заменяют, а скорее дополняют.
Что же до качества, то имелось в виду, что мое-то качество (при отсутствии ревью и при его наличии) не сильно меняется, так как все же больше зависит от моей квалификации, нежели от возможности ревью. Но это предполагает некоторый базовый уровень — т.е. в случае джуна скорее всего это будет не так.
а у нас не только ревью - тестов тоже нет)
Вы же, как я понимаю, отвечаете за результат, что называется «в одно лицо», схалявить на долгом промежутке у вас возможности нет, а потому code review вам не нужен.
Чем его заменить — зависит от того, что вы понимете для себя под качеством кода. Если ваш подход к качеству является объективным, в соответствии с вашими личными материальными интересами — т.е. писать код так, чтобы меньше грузить себя лишней работой для достижения приемлемого уровня удовлетворенности нанимателя (чтобы не уволил, грубо говоря) — то, возможно, вам вообще не нужно делать ничего (разве что, не забывать напоминать нанимателю/его представителю, что сделанное быстро сейчас будет долго править потом). Просто выработайте оптимальные именно для вашего случая баланса срочности/объема поддержки/квалификации/лени практики написания кода. Они, кстати, не факт, что совпадут с рекомендуемыми в книжках про «совршенный код» — со всеми этими SOLID, GoF и рекомендациями от Дядюшки Боба. Хотя, если вы там что-то найдете полезное лично вам — не стесняйтесь использовать: пусть они все и решали несколько другую проблему, но с вашей она имеет немало сходства.
В отдельных случаях, когда вы чувствуете, что явно зашли не с того конца (у меня такое всего пару дней назад было, причем не на работе, а в проекте, который я чисто для себя делаю, для статьи на Хабре: зашел не с той стороны буквально — начал реализовывать некую сложную логику обработки некоего массива с первого элемента, а не с последнего, как по уму оказалось надо) — отложите этот кусок (можно даже не дописывая) и дайте ему вылежаться (примерно как Andrey_Solomatin выше написал), а в это время займитесь чем-то другим.
Из своей практики (возможно, устаревшей, но я со времен перфокарт так привык) подскажу метод «прогон без компьютера» (dry run) — пробовать время от времени протестировать в уме, как будет выполняться код при тех или иных значениях переменных. Впрочем, совет этот — возможно, устаревший: при нынешнем уровне развития средств программирования, наверное, будет проще провести «неформальное тестирование» — запустить нужный модуль в отладчике, поставить в нужном месте точку останова, в ней, прямо в отладчике, задать для переменной тестовое значение, и пройти модуль по шагам, сопоставляя его реальное поведение с ожидаемым: это значительно менее трудоемко, чем написание формальных test case, которые вам вряд ли нужны.
А вот если для вас «качественный код» означает, что вы хотите лучше писать код для условий коллективной разработки, то тут совет fkthat чуть выше — сменить работу — совершенно правильный: в одиночной и коллективной разработке нужны таки несколько разные навыки.
для галер
Можно использовать кодревью для контроля, но это далеко не едеинственное его применение.
Код ревью из под палки будет неэфективным.
пробовать время от времени протестировать в уме, как будет выполняться код при тех или иных значениях переменных
Классный метод. Но есть ограничение по сложности и воспроизводимости. После того как я открыл для себя прекрасный мир тестирования, то понял что накидать юнитов быстрее, и изменения в коде не требуют усилий по перепроверке.
Можно использовать кодревью для контроля, но это далеко не едеинственное его применение.
С материалистической точки зрения, т.е. достижения результата процесса разработки ПО, это применение — основное.
Код ревью из под палки будет неэфективным.
Правильно. Поэтому владельцы предприятия по производству ПО стремятся дать своим наемным работникам и другую мотивацию. Иногда это делается сравнительно честным обрзом — например, получением дохода от развития предприятия и, как следствие, роста курса его акций посредством опционов.
Но чаще эта мотивация навязывается работникам манипулятивными методами. Например — техникой гранфаллуна, которая описана, например Чалдини в его монографии «Психология влияния» («Влияние» в последующих изданиях). Суть манипуляции в этой технике состоит в том, что группа манипулируемых, объективно не имеющих общности в своих интересах, объявляется командой, имеющей, якобы, общий интерес — чтобы побудить членов команды действовать совместно в интересах манипулятора.
В современной отрасли разработки ПО эта техника манипуляции применяется довольно широко (и на откровенных галерах, в том числе) и, пока — с немалым успехом.
Классный метод. Но есть ограничение по сложности и воспроизводимости.Он, вообще-то — из прошлого, из времен, когда нужно было долго ждать результатов тестового прогона. Сейчас он IMHO опрадван только когда программа ещё далека от завершения и существует в значительной части в виде псевдокода и комментариев, которые ни один компилятор не приведет к пригодному для реального запуска виду. Когда код уже почти написан, описанное мной чуть ниже неформальное тестирование выполнить проще.
Ну и, естественно, оправдано это только для нетривиальных программ — вряд ли типовой CRUD стоит таких затрат усилий.
С формальным тестирование та проблема, что тестов там «по науке» требуется много (в идеале — «100% покрытия»), что избыточно трудоемко для кода, который, в основной своей части, достаточно прост и будет достаточно часто изменяться. В индивидуальном программировании такое количество тестов вряд ли оправдано IMHO.
Необходимо ограничивать время код ревью. Смотрите на размер пул реквеста и говорите себе, например, «не более получаса». Лучше немного пожертвовать качеством ревью чем убить 3 часа старшего разработчика
Я не смотрю на это как на "убить 3 часа". Скорее как на "поделиться опытом". Но да, ограничивать себя временными рамками полезно. Ещё полезно делегировать. Этим тоже пользуюсь.
Я бы ограничивал по размеру.
Время растёт не линейно с количеством строк кода.
Разбивайте изменения. Не мешайте много фич в один реквест. Не машайте рефакторинг и изменения.
Для себя выбрал стратегию: делаю все вперемешку в одной ветке, и в процессе работы часть изменений выношу в отдельные ветки и отправляю её на ревью. Изменения не большие ревьювятся быстро и качественно. В этом мне помогают мои друзья: merge, rebase, cherry-pick, `checkout <branch> -- file`, `cp` . Это требует времени, можно запутаться, но это быстрей чем ждать ревью на гиганские изменения.
Например ревью на +-200 строк на переименование переменной делаются со скоростью скролла и качественно. А вот если среди этих 200 будет еще пара фиксов, то будет либо не быстро (нужно понимать где фикс, а где рефакторинг), либо не качетственно (фиксы не заметят).
Да, прямо плюсик вам в карму. Очень важный момент про "разбивать изменения". Это всего процесса разработки касается, а не только код-ревью.
Считаю вредной практикой пул-реквесты на 1000+ строк. Надо уметь "есть слона по кусочкам"
Скажем так, получается практически всегда, но есть множество ситуаций, когда работа по подготовке pr так, чтоб ревьюверу было удобно, комфортно, и он не дай бог бы не перенапрягся — выливается в разы больший объем работы, чем просто сделать всё одним pr.
Исключения это нормально. Как только начинаешь осознанно разбивать изменения, то их становится меньше.
Спасибо за развернутый и хорошо структурированный пост
Основная проблема при ревью - это мотивация. Мотивацию дает понимание разработчиками цели ревью. "Улучшить качество кода" - это не цель. "найти баги" - не цель. Цель должна быть измеримой, и должна приносить удовлетворение разработчикам. Хорошая цель, например, - это сократить количество возвратов фичи от QA к разработчикам и обратно. Все разработчики ненавидят возвраты из-за "лажанули, забыли корнер кейс покрыть".
Для того, чтобы это стало реальной целью ревьювера - нужно чтобы при возврате от QA баги разгребались не только автором пулла, а всей командой. Тогда при ревью каждый будет заинтересован в том, чтобы лажа не ушла в тестирование.
То же самое с простотой поддержки. Вот это не мотивирует ревьювера:
Хорошо ли такой код будет расширять и поддерживать в дальнейшем [автору пулла]?
А вот это мотивирует:
Лично тебе придется расширять расширять и поддерживать этот код в дальнейшем. Подписывашься на такое?
Насчет "старший ревьювает за младшим". Старший просто умрет от такого объема. Это решается через peer code review - все за всеми, рандомно. Младшие должны ревьювать код за старшими, иначе у них не будет картины "как делать хорошо", или "как принято делать в команде". Новичкам, даже не джунам, очень тяжело это понять, особенно если codebase большой. Возможность посмотреть вживую, как делаются конкретные изменения, вместо попыток разобраться по всему объему кода, очень хорошо помогает в онбординге.
По личному опыту - основная проблема при внедрении code review - это то, что пуллы медленно разбираются. Мы используем самописный бот, который выдает рекомендации на основе недавней истории ревью, но даже просто канал в слеке, куда падают все новые пуллы, хорошо помогает в разгребании очереди.
Слова не мальчика, но мужа.
Хотя про метрики не могу согласиться. Это скорее релевантно для тестирования, а не для ревью. Как измерить "поддерживаемость" и "расширяемость" кода?
Про мотивацию согласна. Для меня лучшей мотивацией является понимание, что на ревью код не делится на Мой и Твой. Это всё Наш код, общий, нашего продукта.
И если автор уйдёт в отпуск (возможно, декретный), нормально остальным будет этот код поддерживать?
У нас диктатура и нельзя коммитить код без ревью другой вопрос, что бывает ревьювится "на отвали". Человеческий фактор никто не отменял.
Ну и про "старшего" и "младшего". У нас в этом полный разнобой. Автор сам выбирает себе ревьюэра (или нескольких). Просто бывает так получается, что кто-то одна прикольно ревьюит и все к ней набегают
Поддерживаемость и расширяемость - да, не померять. Поэтому надо строить процесс так, чтобы ревьювер был прямо заинтересован в результате - потому что это ему придется поддерживать и расширять.
Диктатура - да, у нас те же правила. Branch protection rule на гитхабе + явно озвученное "за результат отвечает ревьювер, а не автор". Ну и плюс - у нас ревьювер должен просмотреть еще и задачу на тестирование, проверить, что там упомянуты все затронутые места, и дать отмашку QA.
Из нерешенных проблем - у нас несколько команд, работающих над разными фичами. Команды быстро просекли, что им выгоднее ревьювать изменения в рамках команды (один сделал, второй просмотрел и отдал в тест), т.к. свой userstory ближе к телу, чем user story другой команды. И из-за этого code ownership понижается. Как это чинить без еще большей диктатуры - непонятно.
Тогда при ревью каждый будет заинтересован в том, чтобы лажа не ушла в тестирование.
Это замена реального тестирования на стенде на мысленное тестирование в голове ревьюера. А если ревьюер будет не мысленно тестировать, тогда он будет выполнять работу тестировщика. Я бы лучше посмотрел новые изменения отдельным коммитом, чем мысленно выискивать хитрые ошибки в выполнении.
Да, если использовать термины из пентеста - это мысленное поверхностное white box тестирование. Работа тестировщика - это тщательный grey box. Есть проекты, где это довели до абсолюта - тестировщиков нет, тестируют ревьюверы (
Но "лажа" это обычно не хитрые ошибки в выполнении, а что-то, что значительно влияет на результат, или что потом будет сложно и дорого поддерживать [лично ревьюверу и остальным в команде].
Я бы лучше посмотрел новые изменения отдельным коммитом
Не совсем понимаю, почему тут "лучше, чем". Гитхаб показывает пулл в виде одного большого диффа. Но почему это противопоставляется поиску недочетов в коде?
Я о том, что мы посмотрели код, отправили на тестирование, нашлись ошибки, и автор сделал изменения для их исправления. Проще во второй раз посмотреть только новые изменения, чем заново весь дифф. И проще посмотреть второй раз, чем мысленно искать такие ошибки в первый, пытаясь не допустить второй.
Ну так можно и во второй раз не смотреть на суть фикса и не искать мысленно ошибки, и в третий, и в четвертый. Просто у нас по метрикам и по удовлетворению разработчиков оказалось проще поискать лежащие на поверхности ошибки в первый раз, чем затягивать фичу из-за возвратов. Это решение команды на основе опыта на конкретном проекте, вполне возможно что оно у вас не зайдет. Я же просто опытом ревью на своем проекте делюсь, а не уверждаю "у все должно быть так".
это мысленное поверхностное white box тестирование
Звучит как что-то от авторов "вот тебе доска и фломастер, побудь компилятором часик".
И у вас это реально работает? Или там вся работоспособность этого подхода создана целиком через кнут "за результат отвечает ревьювер, а не автор"? И просто разработчики половину времени работают тестерами за зарплату разработчиков?
Из моего собственного опыта, ревью на "работоспособность" кода вообще не работает, если мы конечно не подразумеваем, что ревьювер должен поработать существенную часть времени тестером. На качество кода — вполне, на недопущение техдолга — тоже, ловить архитектурную лажу на ранних этапах, пока она не укоренилась в коде — весьма. Что код делает ровно то, что ожидается — уже сложнее, и хорошо идёт только тогда, когда документация по требованиям ведется хорошо и скрупулезно (а это даже в забюрократизированных конторах не всегда выполняется). Что в коде нет явных багов — работает только тогда, когда ревьювер надевает шапку QA и некоторое время занимается именно тестированием кода. А иначе не работает.
Я нигде и не утверждал, что у нас ревьювают код на "работоспособность". Это вы додумали :)
Мы всего лишь стараемся поймать лажу любого вида до ухода в тестирование. Включая лажу в виде плохого качества кода, лажу в архитектуре и прочее. Это все лажа, которая все равно вернется от QA, но неявно, и в виде отдаленных последствий, которые будет уже слишком дорого починить. Из явных багов - ну, ревью помогает что-то трудноуловимое ручным тестированием поймать, race conditions, плохую сложность алгоритмов, отсутствие атрибутов авторизации - то, что прилетит уже не от QA, а потом, с продакшена. И да, даже ревью архитектуры - это все равно тестирование, просто его всегда делают разработчики. Как и юнит тесты - это тоже тестирование, которое должны делать разработчики.
Есть проекты, где даже ручное тестирование лежит на ревьюверах - в kibana так принято, насколько я знаю. Но на мой взгляд это уже перебор :)
Я вообще писал о конкретной мотивации для ревью, а не о том, что ревьювать, а что - нет. Что именно ревьювать - решает команда. Если у вас QA получают в 10 раз меньше разработчиков, и security/performance баги не критичны и при этом проект короткий "написать и забыть" - то да, ни ревью, ни юнит тесты себя не окупят.
Я нигде и не утверждал, что у нас ревьювают код на "работоспособность". Это вы додумали :)
Пардон, значит плохо читал.
Включая лажу в виде плохого качества кода, лажу в архитектуре и прочее. Это все лажа, которая все равно вернется от QA, но неявно, и в виде отдаленных последствий, которые будет уже слишком дорого починить.
Вот тут что-то не вижу связи. Наоборот, это обычно такие вещи, которые не возвращаются из QA на обозримом горизонте планирования. Зато отлично выстреливают сильно потом, когда уже может быть и команда состоит из совсем других людей. Короче, сильно ухудшить качество кода на проекте так, чтоб никто этого даже и не заметил (кроме других программистов, работающих с этим же кодом) — это на самом деле очень легко. Почему и жизнь без ревью — это как правило тот еще ад.
Я вообще писал о конкретной мотивации для ревью
Я читал, да. Просто как-то оно не очень убедительно звучит, совершенно непонятно, как делить на "прошло ревью, потому что просто незамеченый баг в коде, ревьювер не смог побыть процессором", "прошло ревью, но аукнулось только через пару лет", и "прошло ревью, хотя по нашим установкам как раз такое должен был поймать ревьювер".
Возврат от QA и горизонт очень сильно зависит от продолжительности проекта. У нас до ревью больше всего было проблем не с возвратом от QA по инициативе QA (баг нашли), а от неявного ревью - отдавали в тестирование, а через неделю кто-то лез в тот же кусок, видел там треш, и фичу возвращали на разработку. Собственно, правила "ревьювать до отдачи QA" и "ревьювер перечитывает таск на тестирование" у нас именно QA провели - им надоело перетещивать по несколько раз.
Ну и еще из специфики - у нас проект долгий, и текучка низкая. "После введения ревью стало аукаться через пару лет/месяцев реже, чем раньше" - ощутимый эффект, и его чувствую те же люди, а не другая команда. Так что их это очень мотивирует. Жизнь без ревью - да, согласен, еще тот ад, причем у нас это ад именно для той же команды, которая изначальный код написала :)
Использовать комментарии
Здесь не все так просто. С одной стороны, хорошо написанный код не нуждается в комментариях. С другой стороны, мало кто пишет хороший код, бывают часто довольно сложные куски логики, которые пришлось написать из-за косяков бэкенда, ошибок в библиотеке или использования алгоритма для выполнения специфичной операции.
Я работал в компании, в которой было запрещено оставлять комментарии, и мой старший коллега рефакторил тонны кода. Такое себе было занятие.
Конечно, встречалась с такой точкой зрения, что хорошему коду комменты не нужны.
Возможно, в каких-то отраслях программирования реально написать хороший код без комментов.
Но я всю дорогу программировала высокопроизводительные алгоритмы. Куча оптимизаций, эвристик, нетривиальные структуры данных и схемы распараллеливания. Там даже с хорошими комментариями бывает сложно въехать в чужой код.
Интересно было бы посмотреть на пример большого проекта без комментариев.
Куча оптимизаций, эвристик, нетривиальные структуры данных и схемы распараллеливания.
В таком коде комментарии нужны, но у вас написано так, как будто это общий совет для любого кода. Такого кода гораздо меньше, чем обычного. Мне кажется, лучше добавить пояснение в статью.
С первого раза не получилось хорошо сформулировать.
Моя мысль в том, что кодом без комментариев можно описать Что делает этот код. Но нельзя описать, Почему он делает это именно так.
Как известно, при написании программы можно сделать одно и то же разными способами. Без комментариев бывает неочевидно, почему выбран этот конкретный случай. И то самое "почему" – один из частых вопросов, возникающих при чтении чужого кода.
Бывает читаешь скудно откомментированный код и думаешь "ну зачем, зачем он тут эти сложности навертел?". А потом спрашиваешь автора и он тебе объясняет, что при определённых сценариях эти сложности вполне оправданы. И ты такая "ах вот оно что! Напиши это в комментарии"
В теории так, а на деле пример: задача перейти с Flutter 1 на Flutter 2 в большом приложении. То есть переехать на dart null safety. Это затрагивает все dart-фалы в проекте, около четырех тысяч ошибок в проекте при переезде. Пул-реквест не соберется, пока все ошибки не будут устранены. А окончательный ПР будет огромный.
Я не спорю, что бывают случаи, когда ну никак пул-реквест не попилить на кусочки. Или можно, но слишком сложно.
Но в других случаях надо всё-таки стараться как-то разбивать изменения на удобочитаемые кусочки. А то есть деятели, которые по паре недель фигачат свою фичу, а потом ревью присылают на 2000 строк. Таких надо обучать.
Если это одна новая фича на 2000 строк — зачем её делить?
То есть да, иногда поделить можно, но в общем случае — скорее нет. И если никакой промежуточной ценности отдельные кусочки фичи не несут, то делить pr — это в чистом виде вопрос отношения цены времени пишущего код к цене времени ревьювера.
Вот когда выкатывается новая фича на 1000 строк, а к ней еще в довесок 1000 строк изменений "ну я тут порефакторил, чтоб всё было красиво" — вот тут да, обучать нужно. Чтоб в один pr не попадала работа в разных направлениях.
Один реквест на 1000? Не страшно.
Я бы такой вообще не ревьювил. Это бесполезно. Собралось и совсем всё не разъехалось - можно мерджить. А уже потом небольшими кусочками доводить до ума и красоты.
Такие вещи надо делать быстро. Если это затянуть и не остановить работы в основной ветке, то будет очень больно.
Во мне процесс code review вызывает смешанные чувства: с одной стороны, это безусловное благо.
Это благо — никоим образом не безусловное. Для наемного работника оно объективно благом не является, еcли смотреть с точки зрения его материальных интересов (т.е. баланса затрат труда и получаемых денег). Точно так же, как благом для него не является и качество продукта, в создании которого этот работник участвует, и которое практика code review призвана повысить. Качество продукта — от которого зависят затраты владельца на его развитие и поддержание, а также приносимые владельцу доходы — это, если подходить с материалистической точки зрения, есть благо исключительно для владельца бизнеса, опосредовано — для лиц, которым он поручил руководить разработкой продукта: владелец им именно за это платит деньги. А code review — это инструмент для достижения целей владельца.
Так что если вы входите в число таких лиц (я ничего не знаю про вашу должность) — то для вас вопрос повышения эффективности code review актуален вполне объективно (с материальной точки зрения). То же самое — и для ваших читателей. Если же вы или ваш читатель входит в число рядовых работников, то для такого человека объективно актуален совсем другой вопрос: как легче пройти code review и не нарваться.
PS Слово «команда» в тексте статьи я видел, и основанные на этом понятии возражения для меня вполне ожидаемы, и ответ на них (отрицательный) у меня есть. Если кого это интересует — пишите в комметарий — я поясню, в чем там реально (т.е. со стороны исключительно материальных интересов) дело, и почему возражения эти не объективны.
Здесь пока писать об этом не буду, чтобы не раздувать комментарий (я и так слишком многословен).
Это так, если полностью игнорировать мотивацию. Т.е. если исходить из того, что все действия разработчика мотивированы исключительно внешним фактором - деньгами. Вот только на практике разработчиками движет внутренняя мотивация - ощущение того, что они создают что-то полезное для людей, желание делится знаниями с другими, удовольствие от работы с профессионалами и прочие субъективные ощущения.
Если у вас галера - то да, там все упрется во внешнюю мотивацию и "просто пройти ревью". Поэтому там код ревью не взлетает - он там объективно вообще никому не нужен, ни стейкам, ни девелоперам. На продуктовых конторах - нет, там код ревью - это объективное благо, и команды сами его поддерживают. То же самое и с остальными best practices - от юнит тестов до скрама.
Это так, если полностью игнорировать мотивацию.
Да, комментарий написан именно с чисто объективной, материалистической точки зрения, и я это в нем всячески подчеркивал. С упором на то, что основная причина, почему наемный работник-разработчик работает на предпринимателя, а не творит код сам или с вместе с друзьями — это необходимость зарабатывать себе на жизнь в рамках существующего способа производства.
И что описываемая вами внутренняя мотивация неадекватна объективному положению разработчика в рамках этого способа производства: по факту наемный работник отчужден от результатов собственного труда — т.е. не получает от рынка никаких материальных ни плюсов, ни минусов от того, сдеал ли он продукт хорошо или плохо, все эти материальные плюсы и минусы достаются владельцу предприятия. Ну, а я, будучи материалистом, верю, что под действием материальных факторов эта самая мотивация наемного работника будет для основной их массы стремиться к тому, чтобы соотвествовать их материальному положению.
Что касается галеры, то вы, вообще говоря, заблуждаетесь в том, что code review как инструмент обеспечения качества кода не нужен ее владельцу (stakeholder): конкуренция на рынке услуг по разработке ПО рано или поздно заставит производить из качественный продукт, несмотря на тот трэш и угар, который, наприме, судя по материалам Прекрасного.ИТ, творится на конкретном украинском рынке разработки ПО.
С материалистической точки зрения внутренняя мотивация неадекватна, на практике - она объективно первична, и объяснение тут только одно - чисто материалистическая модель не описывает реальную ситуацию, т.к. просто не учитывает психологию и определяет "успех", "счастье" и "удовлетворенность" всего-лишь как "количество заработанных денег".
На практике чисто материалистическая модель дает настолько плохие результаты, что компании, опирающиеся только на нее, быстро проигрывают конкурентам, которые опираются на более общую модель и учитывают мотивацию. Даже компании "треш и угар" учитывают мотивацию - например, обрабатывают студентов, и пропускают их через себя за год-два, пока у тех внешняя мотивация (деньги) работает, и закладывают "человек захочет внешней мотивации и уйдет" в свою модель. Просто они осознанно выбирают нишу, в которой можно жить чисто за счет внешней мотивации сотрудников, без тестов и без ревью.
Простой тест на реальность внутренней мотивации - на текущем месте вам недавно повысили зп на 500$, ревью - через полгода. Вы получили оффер из другой конторы на 10$ выше. Вы, как материалист, примите оффер без раздумий? Или вас что-то удержит?
Поэтому предлагаю обсуждение этого вопроса, если вам онинтересен, перенести куда-нибудь на следующий раз.
Я может немного идеалистка и витаю в облаках. Но у меня пока такое работает: чем больше заработает мой работодатель, тем скорее он поднимет мне зарплату.
Хорошие работодатели обычно не только под себя гребут, но и с сотрудниками делятся. А иначе – текучка кадров, низкий комплаенс – кому это надо?
В общем, лично мне выгодно работать в интересах работодателя.
Я, когда уже более 15 лет назад ушел из программирования в системное администрирование Windows, наблюдал там точно такую же ситуацию, потому что рынок специалистов в этой области тоже рос аналогчиным образом.
Но с тех пор ситуация на этом рынке поменялась на противоположную, и подобный идеализм на нем уже выглядит неоправданным (хотя ещё и встречается).
Я может немного идеалистка и витаю в облаках. Но у меня пока такое работает: чем больше заработает мой работодатель, тем скорее он поднимет мне зарплату.
Хорошие работодатели обычно не только под себя гребут, но и с сотрудниками делятся. А иначе – текучка кадров, низкий комплаенс – кому это надо?
В общем, лично мне выгодно работать в интересах работодателя.
В тысячный раз о code review