Почему в тысячный?
Потому что я вбила здесь в поиске "code review" и обнаружила 50 страниц по 20 результатов на каждой.
Штуки три с высоким рейтингом прочитала. Но всё по понятным причинам не осилила.
Вероятно, не все мысли в этом посте будут новыми. Но некоторых из мыслей я не нашла в тех трёх рейтинговых постах. Поэтому считаю, что этому посту быть.
Итак. Есть много способов для повышения качества кода, создаваемого командой: тестирование, статический анализ, экстремальное программирование,... Сегодня расскажу о code review.
Для тех, кто не в курсе, code review – это процесс просмотра кода, сделанного одним разработчиком, другим разработчиком с последующей выдачей комментариев. В идеале, это повышает качество кода.
Во мне процесс code review вызывает смешанные чувства: с одной стороны, это безусловное благо. Правильно настроенный процесс повышает культуру разработки кода, позволяет старшим сотрудникам ненавязчиво делиться опытом с новичками, улучшает качество инженерных решений и т.д. С другой стороны, если ты та самая старшая разработчица, грамотно и вдумчиво проводящая ревью, то каждый первый младший разработчик в твоей команде стремится заполучить твое мнение по поводу своего кода. И вот ты проводишь до трёх часов в день просматривая чужой код вместо того, чтоб писать свой.
Это цена. Не все команды согласны её платить в погоне за качеством. Мне везло работать в тех, что согласны.
Итак, поехали. Как проводить code review, чтобы извлечь из процесса максимальную пользу?
Вежливо. Даже если вам кажется, что коллега написал это код в пьяном угаре не иначе, нужно выдавать свои комментарии в максимально корректной форме. Все ошибаются. Это не повод их стыдить, высмеивать и прочее. Цель всего - улучшить качество вашего совместного продукта. А блеснуть своим белым пальто можно и в другом месте, если хочется. Пишите комментарии так, как вы бы хотели, чтоб их писали вам.
В одиночестве. Лучше, если сначала вы посмотрите чужой код одни, в спокойной обстановке. Свежим взглядом. Потом уже можно обсудить его с автором.
Задавать вопросы в случае недопониманий. Тут всё просто. Нужно стремиться понять чужую мысль на 100%. Как вы знаете, если когда-нибудь читали чужой код, это не всегда бывает просто. Если что-то непонятно – спросите автора, можно по телефону, если так быстрее.
Использовать комментарии. В коде имею ввиду. Хорошо откомментированный код снижает количество созвонов, требующихся в предыдущем пункте Помните об этом. Экономьте чужое время.
Соблюдать определённые ограничения. Эксперты советуют просматривать за раз не более 500 строчек кода, а также ограничивать длительность ревью шестьюдесятью минутами. Тут бы ссылку на источник, но я не помню, где видела это правило. Но оно правда дельное. Пользуйтесь.
Использовать чек-лист. Чтобы повысить эффективность себя как ревьюэра, хорошо бы завести список того, на что надо обратить внимание. Мои три общеупотребимые вещи для проверки: дубликации кода; грамотное использование RAII (это ещё что такое? Кто не знает - объясню как-нибудь ещё); грамотное именование сущностей. Остальное специфично для языка и для проекта.
Принимать чужое мнение. Обычно в программировании нет единственно правильного способа сделать что-то. И если автор сделал что-то не так, как бы сделали вы и у вас есть смутное чувство дискомфорта по этому поводу. Но грамотно объяснить, чем способ автора хуже, чем ваш вы не можете. Встаньте на сторону автора. Вот правда. Никому не нужна эта священная война с непонятной выгодой. От разнообразия решений ваш проект только выиграет, возможно
Как НЕ НАДО проводить code review?
Юмор за 300. Как мы помним, вежливось очень ценна для грамотного code review. Я понимаю, что бывает присылают на ревью такое, что сложно не взоржать, и что мы все тут дофига юмористы. Но правда, лучше поржите с подружками после работы. Если у вас есть подружки-программистки не из вашей команды, расскажите им эту фееричную шутку – они оценят. Code review достаточно личная штука и шутки над кодом могут быть восприняты как личное оскорбление. Не надо так.
Пытаться делать работу по отлову ошибок. Это не то, на чём нужно концентрироваться. Для отлова ошибок существует тестирование, валидация в конце концов. Оставьте эту работу тестам. А при ревью лучше сконцентрироваться на том хорошо ли код спроектирован. SOLID принципы, идиомы объектно-ориентированного программирования. Вот это всё. На этом концентрируйтесь. Хорошо ли такой код будет расширять и поддерживать в дальнейшем? Эти вопросы должны вас волновать в первую очередь, на мой взгляд.
Проверять соответствие стилю (в смысле coding style/coding guidelines). Не нужно. Все проверки правильного количества пробелов, длинны строк, наличия табов и т.д. оставьте роботам. Т.е. просто введите в ваши процессы автомат по выравниванию стиля. Indent, clang-format и т.д. вам в помощь.
Если нужно что-то индивидуальное, наличие правильной "шапки" в файлах проверять или какое-то особенное именование сущностей - скрипт напишите. Всё, что может быть автоматизировано недорогой ценой, должно быть автоматизировано. Программисты мы или кто в конце концов? Это цель нашей работы – упрощать людям жизнь. Ну и себе можно жизнь упростить по дороге
Всем хороших code review.
А с меня хватит на сегодня, пожалуй.
Комментарии (63)
Akturian
25.08.2021 13:39+1Вопрос на миллион: а моей текущей компании, нет кода ревью. Более того, здесь акцент на быстрое выполнение задачи, а насколько ты пишешь плохой код, мало кого волнует(не it компания). Ибо баги в этом коде исправлять все равно тебе.
Чем я могу заменить код ревью, чтобы повысить качество своего кода?
she_codes Автор
25.08.2021 13:49Вопрос интересный. Достоин отдельного поста. Подумаю как кратко сформулировать и отвечу позже.
NN1
25.08.2021 13:50+2Тут конкретный ответ зависит от технологического стека.
В общем стоит настроить автоматическое форматирование и анализаторы кода как статические так и динамические на каждый комит ну и конечно запуск различных тестов.
Тогда весь код будет единообразно выглядеть и не будут проходить мелкие недочеты.
Кстати, это применимо также и если проводится ревью кода, чтобы не тратить время на болтовню вроде «запятая не там стоит», «а оно точно не падает?» и т.п.
Akturian
25.08.2021 14:11.Net, js
NN1
25.08.2021 15:00+1Для 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/ , который это сделает автоматически для вас.Akturian
25.08.2021 15:52Спасибо) пошел гуглить и строить)
she_codes Автор
25.08.2021 22:29Ко всему уже насоветованному добавлю, что если нет код ревью, то я б сосредоточилась на других способах улучшения качества: тестирование, continuous integration, статические и динамические анализаторы кода (если это применимо к .net и js).
Ну и стараться делать код более устойчивым к изменениям. В смысле, чтоб небольшое изменение не приводило к тому, что надо делать правки в десяти местах. Для этого помнить про SOLID и прочих GoF в процессе разработки.
Caraul
25.08.2021 19:23Для .NET - https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/overview Это анализаторы самого кода, а для форматирования или, как уже подсказали, R#, или старый добрый https://www.nuget.org/packages/StyleCop.Analyzers/
Andrey_Solomatin
25.08.2021 13:56+2Прикрутить автоматизацию: линтеры форматеры. Это вешается на прекоммит хуки и CI (если есть). Насроить всё это для питона оказалось достаточно затратно, но зато теперь таскаю это с собой.
На больших объемах кода тесты сильно ускоряют процесс. Но тут нюансов море.
Само-ревью. Пишешь код и перед тем как его отправить смотришь на него самостоятельно. Много мелких недочётов всплывает. Чем лучше память тем долше должен быть перерыв между написанием и ревью. Иначе будет чтение не с экрана, а из головы.
Я еще используею инкрементальный коммит в гите ( git add -i > 5: patch > * > yyy). В отличии от UI вариантов это не даёт проскролить в конец. Но тут стоит учесть, что я часто комичу.
anonymous
00.00.0000 00:00
ApeCoder
25.08.2021 14:36Ибо баги в этом коде исправлять все равно тебе.
Не вижу логики - платит то
все равно компания. И программисту и пользователям, которые будут натыкаться на эти баги и репортить, пока их не исправят.
sshikov
25.08.2021 22:34Полноценно — ничем. Тут уже предложили статический анализ, типы и прочее — так это все лишь в какой-то степени способно заменить человека на review. Просто потому, что человек понимает не просто что написано, но и зачем. Роботы и инструменты этого не могут или почти не могут.
Ну т.е. внедряйте типизацию, внедряйте статический анализ, но имейте в виду, что скажем мне сонар каждый день предлагает в Java коде заменить лямбду ссылкой на метод. И в 99% случаев этот совет статического анализатора не имеет никакого смысла — потому что ссылка на метод предлагает переиспользование метода, а лямбда — что он уникален. И инструменты на сегодня достаточно тупы, и таких тонкостей не понимают. И правила у них зачастую достаточно тупы.
>Ибо баги в этом коде исправлять все равно тебе.
Ну я пару раз писал достаточно крупные проекты в одиночку. Благо, со сроками никто не гнал, и на то чтобы думать о качестве, было достаточно времени. Но review не было все равно, потому что не было больше никого, кто мог бы это делать. Очень часто хотелось посоветоваться, но прямо скажем, на качество это влияет не сильно. Сроки, а точнее их адекватность, влияют сильнее.Andrey_Solomatin
26.08.2021 13:34И в 99% случаев этот совет статического анализатора не имеет никакого смысла
Отключите это правило. Зачем тратить на это время?
Наработающие правила очень мешают при работе в команде, те кто еще не прониклись плюсами статического анализа спотыкаются на таких вещах и в результате весь код утыкан комментами для анализатора и появляется мнение: статический анализ зло.
> на качество это влияет не сильно
Код ревью это еще и обмен опытом и знаниями о проекте. Разница между "что они тут наворатили" и "я смотрел этот код" огромна.
То есть когда тебе нужно что-то сделать в коде, а его автор не доступен, ты можешь быстро включиться. Когда нет кодревью то вполне реален вариант: автор может поправить за 5 минут, а остальные за 2 дня.
Качество понятие относительное. То что хорошо для одиночки может быть плохо для команды.
Я как-то зарефакторил адапторы написанные на 80% мной из кучи функций в иерархию классов. Когда ученый, сделал нормальный адаптор вообще не задавая мне вопросов, я понял, что получилось нормально. Со старой версией он бы самостоятельно не справился. Старая версия писалась с внимательным код ревью.sshikov
26.08.2021 16:46>Отключите это правило. Зачем тратить на это время?
Я не всегда это могу. Хотя это и неправильно.
С остальным практически полностью согласен. Одиночка тут как пример, когда ревью просто некому делать, и чтобы пояснить, что статический анализ и пр. его не заменяют, а скорее дополняют.
Что же до качества, то имелось в виду, что мое-то качество (при отсутствии ревью и при его наличии) не сильно меняется, так как все же больше зависит от моей квалификации, нежели от возможности ревью. Но это предполагает некоторый базовый уровень — т.е. в случае джуна скорее всего это будет не так.
rickshinova
26.08.2021 11:27а у нас не только ревью - тестов тоже нет)
yorren
26.08.2021 23:43А проект находится в стадии MVP?
rickshinova
27.08.2021 12:30проекты запущены и работают.
"тестируют" уже пользователи, так как проекты относительно небольшие и делаются для конкретных компаний.
mvv-rus
27.08.2021 02:24-2Code review вам не нужно. Это — инструмент для организаций с коллективной разработкой ПО, т.е., если перейти на терминологию сайта Прекрасное.ИТ (она IMHO здесь очень к месту) — для галер. На галере гребцов много, а потому у каждого гребца есть возможность схалявить, грести в полсилы, т.е. сделать свою работу так, чтобы гребцу здесь и сейчас было полегче. Гребец — он ведь объективно не заинтересован в качестве продукта (отсутствии ошибок, снижении затрат на поддержку и т.д.), а он просто тратит свое время и силы на греблю и получает за это оговоренную сумму. Так вот, code review — это инструмент, который владелец предприятия вручает сотрудникам, доводящим его интересы до наемных работников (на языке Прекрасного.ИТ — погонщикам), чтобы заставлять работников работать с определенным качеством.
Вы же, как я понимаю, отвечаете за результат, что называется «в одно лицо», схалявить на долгом промежутке у вас возможности нет, а потому code review вам не нужен.
Чем его заменить — зависит от того, что вы понимете для себя под качеством кода. Если ваш подход к качеству является объективным, в соответствии с вашими личными материальными интересами — т.е. писать код так, чтобы меньше грузить себя лишней работой для достижения приемлемого уровня удовлетворенности нанимателя (чтобы не уволил, грубо говоря) — то, возможно, вам вообще не нужно делать ничего (разве что, не забывать напоминать нанимателю/его представителю, что сделанное быстро сейчас будет долго править потом). Просто выработайте оптимальные именно для вашего случая баланса срочности/объема поддержки/квалификации/лени практики написания кода. Они, кстати, не факт, что совпадут с рекомендуемыми в книжках про «совршенный код» — со всеми этими SOLID, GoF и рекомендациями от Дядюшки Боба. Хотя, если вы там что-то найдете полезное лично вам — не стесняйтесь использовать: пусть они все и решали несколько другую проблему, но с вашей она имеет немало сходства.
В отдельных случаях, когда вы чувствуете, что явно зашли не с того конца (у меня такое всего пару дней назад было, причем не на работе, а в проекте, который я чисто для себя делаю, для статьи на Хабре: зашел не с той стороны буквально — начал реализовывать некую сложную логику обработки некоего массива с первого элемента, а не с последнего, как по уму оказалось надо) — отложите этот кусок (можно даже не дописывая) и дайте ему вылежаться (примерно как Andrey_Solomatin выше написал), а в это время займитесь чем-то другим.
Из своей практики (возможно, устаревшей, но я со времен перфокарт так привык) подскажу метод «прогон без компьютера» (dry run) — пробовать время от времени протестировать в уме, как будет выполняться код при тех или иных значениях переменных. Впрочем, совет этот — возможно, устаревший: при нынешнем уровне развития средств программирования, наверное, будет проще провести «неформальное тестирование» — запустить нужный модуль в отладчике, поставить в нужном месте точку останова, в ней, прямо в отладчике, задать для переменной тестовое значение, и пройти модуль по шагам, сопоставляя его реальное поведение с ожидаемым: это значительно менее трудоемко, чем написание формальных test case, которые вам вряд ли нужны.
А вот если для вас «качественный код» означает, что вы хотите лучше писать код для условий коллективной разработки, то тут совет fkthat чуть выше — сменить работу — совершенно правильный: в одиночной и коллективной разработке нужны таки несколько разные навыки.Andrey_Solomatin
27.08.2021 12:12для галер
Можно использовать кодревью для контроля, но это далеко не едеинственное его применение.
Код ревью из под палки будет неэфективным.пробовать время от времени протестировать в уме, как будет выполняться код при тех или иных значениях переменных
Классный метод. Но есть ограничение по сложности и воспроизводимости. После того как я открыл для себя прекрасный мир тестирования, то понял что накидать юнитов быстрее, и изменения в коде не требуют усилий по перепроверке.
mvv-rus
27.08.2021 16:12-1Можно использовать кодревью для контроля, но это далеко не едеинственное его применение.
С материалистической точки зрения, т.е. достижения результата процесса разработки ПО, это применение — основное.Код ревью из под палки будет неэфективным.
Правильно. Поэтому владельцы предприятия по производству ПО стремятся дать своим наемным работникам и другую мотивацию. Иногда это делается сравнительно честным обрзом — например, получением дохода от развития предприятия и, как следствие, роста курса его акций посредством опционов.
Но чаще эта мотивация навязывается работникам манипулятивными методами. Например — техникой гранфаллуна, которая описана, например Чалдини в его монографии «Психология влияния» («Влияние» в последующих изданиях). Суть манипуляции в этой технике состоит в том, что группа манипулируемых, объективно не имеющих общности в своих интересах, объявляется командой, имеющей, якобы, общий интерес — чтобы побудить членов команды действовать совместно в интересах манипулятора.
В современной отрасли разработки ПО эта техника манипуляции применяется довольно широко (и на откровенных галерах, в том числе) и, пока — с немалым успехом.Классный метод. Но есть ограничение по сложности и воспроизводимости.
Он, вообще-то — из прошлого, из времен, когда нужно было долго ждать результатов тестового прогона. Сейчас он IMHO опрадван только когда программа ещё далека от завершения и существует в значительной части в виде псевдокода и комментариев, которые ни один компилятор не приведет к пригодному для реального запуска виду. Когда код уже почти написан, описанное мной чуть ниже неформальное тестирование выполнить проще.
Ну и, естественно, оправдано это только для нетривиальных программ — вряд ли типовой CRUD стоит таких затрат усилий.
С формальным тестирование та проблема, что тестов там «по науке» требуется много (в идеале — «100% покрытия»), что избыточно трудоемко для кода, который, в основной своей части, достаточно прост и будет достаточно часто изменяться. В индивидуальном программировании такое количество тестов вряд ли оправдано IMHO.
agoncharov
25.08.2021 13:39+1Необходимо ограничивать время код ревью. Смотрите на размер пул реквеста и говорите себе, например, «не более получаса». Лучше немного пожертвовать качеством ревью чем убить 3 часа старшего разработчика
she_codes Автор
25.08.2021 13:52+2Я не смотрю на это как на "убить 3 часа". Скорее как на "поделиться опытом". Но да, ограничивать себя временными рамками полезно. Ещё полезно делегировать. Этим тоже пользуюсь.
Andrey_Solomatin
25.08.2021 14:11+3Я бы ограничивал по размеру.
Время растёт не линейно с количеством строк кода.
Разбивайте изменения. Не мешайте много фич в один реквест. Не машайте рефакторинг и изменения.
Для себя выбрал стратегию: делаю все вперемешку в одной ветке, и в процессе работы часть изменений выношу в отдельные ветки и отправляю её на ревью. Изменения не большие ревьювятся быстро и качественно. В этом мне помогают мои друзья:merge,rebase,cherry-pick, `checkout <branch> -- file`, `cp` . Это требует времени, можно запутаться, но это быстрей чем ждать ревью на гиганские изменения.
Например ревью на +-200 строк на переименование переменной делаются со скоростью скролла и качественно. А вот если среди этих 200 будет еще пара фиксов, то будет либо не быстро (нужно понимать где фикс, а где рефакторинг), либо не качетственно (фиксы не заметят).she_codes Автор
25.08.2021 15:12+1Да, прямо плюсик вам в карму. Очень важный момент про "разбивать изменения". Это всего процесса разработки касается, а не только код-ревью.
Считаю вредной практикой пул-реквесты на 1000+ строк. Надо уметь "есть слона по кусочкам"
sshikov
25.08.2021 22:37+3Это не всегда получается. Бывают большие фичи, которые не укладываются в 1000 строк. Это плохо, но разбить их на два-три куска иногда выходит еще хуже.
JustDont
26.08.2021 00:51Скажем так, получается практически всегда, но есть множество ситуаций, когда работа по подготовке pr так, чтоб ревьюверу было удобно, комфортно, и он не дай бог бы не перенапрягся — выливается в разы больший объем работы, чем просто сделать всё одним pr.
sshikov
26.08.2021 07:21Ну да, можно и так сказать. По сути же «не получается» обычно и значит, что это слишком трудоемко.
Andrey_Solomatin
26.08.2021 13:45Исключения это нормально. Как только начинаешь осознанно разбивать изменения, то их становится меньше.
sshikov
26.08.2021 16:48Ну, это просто время. По этому тут обычно вопрос стоит так — чье время дешевле, того кто пишет, или того кто читает? С учетом того, что читать потом будут еще и еще, и все такое. Если тот кто пишет, примерно одинаковой с вами (высокой) квалификации, и у него не получилось сходу разбить на куски — я ожидаю, что конкретный случай реально непростой. И скорее всего посмотрю большой PR, хотя возможно и внесу предложение, как его разбить — если у меня такое возникнет.
ivanych
27.08.2021 01:43Нет. Это выльется потом в исправление последствий, которое займёт 3 дня старшего разработчика.
PashaPash
25.08.2021 15:56+2Основная проблема при ревью - это мотивация. Мотивацию дает понимание разработчиками цели ревью. "Улучшить качество кода" - это не цель. "найти баги" - не цель. Цель должна быть измеримой, и должна приносить удовлетворение разработчикам. Хорошая цель, например, - это сократить количество возвратов фичи от QA к разработчикам и обратно. Все разработчики ненавидят возвраты из-за "лажанули, забыли корнер кейс покрыть".
Для того, чтобы это стало реальной целью ревьювера - нужно чтобы при возврате от QA баги разгребались не только автором пулла, а всей командой. Тогда при ревью каждый будет заинтересован в том, чтобы лажа не ушла в тестирование.
То же самое с простотой поддержки. Вот это не мотивирует ревьювера:
Хорошо ли такой код будет расширять и поддерживать в дальнейшем [автору пулла]?
А вот это мотивирует:
Лично тебе придется расширять расширять и поддерживать этот код в дальнейшем. Подписывашься на такое?
Насчет "старший ревьювает за младшим". Старший просто умрет от такого объема. Это решается через peer code review - все за всеми, рандомно. Младшие должны ревьювать код за старшими, иначе у них не будет картины "как делать хорошо", или "как принято делать в команде". Новичкам, даже не джунам, очень тяжело это понять, особенно если codebase большой. Возможность посмотреть вживую, как делаются конкретные изменения, вместо попыток разобраться по всему объему кода, очень хорошо помогает в онбординге.
По личному опыту - основная проблема при внедрении code review - это то, что пуллы медленно разбираются. Мы используем самописный бот, который выдает рекомендации на основе недавней истории ревью, но даже просто канал в слеке, куда падают все новые пуллы, хорошо помогает в разгребании очереди.
she_codes Автор
25.08.2021 16:59Слова не мальчика, но мужа.
Хотя про метрики не могу согласиться. Это скорее релевантно для тестирования, а не для ревью. Как измерить "поддерживаемость" и "расширяемость" кода?
Про мотивацию согласна. Для меня лучшей мотивацией является понимание, что на ревью код не делится на Мой и Твой. Это всё Наш код, общий, нашего продукта.
И если автор уйдёт в отпуск (возможно, декретный), нормально остальным будет этот код поддерживать?
У нас диктатура и нельзя коммитить код без ревью другой вопрос, что бывает ревьювится "на отвали". Человеческий фактор никто не отменял.
Ну и про "старшего" и "младшего". У нас в этом полный разнобой. Автор сам выбирает себе ревьюэра (или нескольких). Просто бывает так получается, что кто-то одна прикольно ревьюит и все к ней набегают
PashaPash
25.08.2021 18:21Поддерживаемость и расширяемость - да, не померять. Поэтому надо строить процесс так, чтобы ревьювер был прямо заинтересован в результате - потому что это ему придется поддерживать и расширять.
Диктатура - да, у нас те же правила. Branch protection rule на гитхабе + явно озвученное "за результат отвечает ревьювер, а не автор". Ну и плюс - у нас ревьювер должен просмотреть еще и задачу на тестирование, проверить, что там упомянуты все затронутые места, и дать отмашку QA.
Из нерешенных проблем - у нас несколько команд, работающих над разными фичами. Команды быстро просекли, что им выгоднее ревьювать изменения в рамках команды (один сделал, второй просмотрел и отдал в тест), т.к. свой userstory ближе к телу, чем user story другой команды. И из-за этого code ownership понижается. Как это чинить без еще большей диктатуры - непонятно.
she_codes Автор
25.08.2021 22:06Мне кажется, если в командах где-нибудь по 5+ человек, то и нормально всё – пускай ревьювят внутри себя.
Вот если человека по 3, то это проблема. Надо какую-нибудь жеребьёвку вводить при ревью. Кидать кубик, я не знаю :-/
michael_v89
26.08.2021 08:26Тогда при ревью каждый будет заинтересован в том, чтобы лажа не ушла в тестирование.
Это замена реального тестирования на стенде на мысленное тестирование в голове ревьюера. А если ревьюер будет не мысленно тестировать, тогда он будет выполнять работу тестировщика. Я бы лучше посмотрел новые изменения отдельным коммитом, чем мысленно выискивать хитрые ошибки в выполнении.
PashaPash
26.08.2021 12:04Да, если использовать термины из пентеста - это мысленное поверхностное white box тестирование. Работа тестировщика - это тщательный grey box. Есть проекты, где это довели до абсолюта - тестировщиков нет, тестируют ревьюверы (
Но "лажа" это обычно не хитрые ошибки в выполнении, а что-то, что значительно влияет на результат, или что потом будет сложно и дорого поддерживать [лично ревьюверу и остальным в команде].
Я бы лучше посмотрел новые изменения отдельным коммитом
Не совсем понимаю, почему тут "лучше, чем". Гитхаб показывает пулл в виде одного большого диффа. Но почему это противопоставляется поиску недочетов в коде?
michael_v89
26.08.2021 12:14Я о том, что мы посмотрели код, отправили на тестирование, нашлись ошибки, и автор сделал изменения для их исправления. Проще во второй раз посмотреть только новые изменения, чем заново весь дифф. И проще посмотреть второй раз, чем мысленно искать такие ошибки в первый, пытаясь не допустить второй.
PashaPash
26.08.2021 14:46Ну так можно и во второй раз не смотреть на суть фикса и не искать мысленно ошибки, и в третий, и в четвертый. Просто у нас по метрикам и по удовлетворению разработчиков оказалось проще поискать лежащие на поверхности ошибки в первый раз, чем затягивать фичу из-за возвратов. Это решение команды на основе опыта на конкретном проекте, вполне возможно что оно у вас не зайдет. Я же просто опытом ревью на своем проекте делюсь, а не уверждаю "у все должно быть так".
JustDont
26.08.2021 12:14это мысленное поверхностное white box тестирование
Звучит как что-то от авторов "вот тебе доска и фломастер, побудь компилятором часик".
И у вас это реально работает? Или там вся работоспособность этого подхода создана целиком через кнут "за результат отвечает ревьювер, а не автор"? И просто разработчики половину времени работают тестерами за зарплату разработчиков?Из моего собственного опыта, ревью на "работоспособность" кода вообще не работает, если мы конечно не подразумеваем, что ревьювер должен поработать существенную часть времени тестером. На качество кода — вполне, на недопущение техдолга — тоже, ловить архитектурную лажу на ранних этапах, пока она не укоренилась в коде — весьма. Что код делает ровно то, что ожидается — уже сложнее, и хорошо идёт только тогда, когда документация по требованиям ведется хорошо и скрупулезно (а это даже в забюрократизированных конторах не всегда выполняется). Что в коде нет явных багов — работает только тогда, когда ревьювер надевает шапку QA и некоторое время занимается именно тестированием кода. А иначе не работает.
PashaPash
26.08.2021 14:43Я нигде и не утверждал, что у нас ревьювают код на "работоспособность". Это вы додумали :)
Мы всего лишь стараемся поймать лажу любого вида до ухода в тестирование. Включая лажу в виде плохого качества кода, лажу в архитектуре и прочее. Это все лажа, которая все равно вернется от QA, но неявно, и в виде отдаленных последствий, которые будет уже слишком дорого починить. Из явных багов - ну, ревью помогает что-то трудноуловимое ручным тестированием поймать, race conditions, плохую сложность алгоритмов, отсутствие атрибутов авторизации - то, что прилетит уже не от QA, а потом, с продакшена. И да, даже ревью архитектуры - это все равно тестирование, просто его всегда делают разработчики. Как и юнит тесты - это тоже тестирование, которое должны делать разработчики.
Есть проекты, где даже ручное тестирование лежит на ревьюверах - в kibana так принято, насколько я знаю. Но на мой взгляд это уже перебор :)
Я вообще писал о конкретной мотивации для ревью, а не о том, что ревьювать, а что - нет. Что именно ревьювать - решает команда. Если у вас QA получают в 10 раз меньше разработчиков, и security/performance баги не критичны и при этом проект короткий "написать и забыть" - то да, ни ревью, ни юнит тесты себя не окупят.
JustDont
26.08.2021 14:55+1Я нигде и не утверждал, что у нас ревьювают код на "работоспособность". Это вы додумали :)
Пардон, значит плохо читал.
Включая лажу в виде плохого качества кода, лажу в архитектуре и прочее. Это все лажа, которая все равно вернется от QA, но неявно, и в виде отдаленных последствий, которые будет уже слишком дорого починить.
Вот тут что-то не вижу связи. Наоборот, это обычно такие вещи, которые не возвращаются из QA на обозримом горизонте планирования. Зато отлично выстреливают сильно потом, когда уже может быть и команда состоит из совсем других людей. Короче, сильно ухудшить качество кода на проекте так, чтоб никто этого даже и не заметил (кроме других программистов, работающих с этим же кодом) — это на самом деле очень легко. Почему и жизнь без ревью — это как правило тот еще ад.
Я вообще писал о конкретной мотивации для ревью
Я читал, да. Просто как-то оно не очень убедительно звучит, совершенно непонятно, как делить на "прошло ревью, потому что просто незамеченый баг в коде, ревьювер не смог побыть процессором", "прошло ревью, но аукнулось только через пару лет", и "прошло ревью, хотя по нашим установкам как раз такое должен был поймать ревьювер".
PashaPash
27.08.2021 15:21Возврат от QA и горизонт очень сильно зависит от продолжительности проекта. У нас до ревью больше всего было проблем не с возвратом от QA по инициативе QA (баг нашли), а от неявного ревью - отдавали в тестирование, а через неделю кто-то лез в тот же кусок, видел там треш, и фичу возвращали на разработку. Собственно, правила "ревьювать до отдачи QA" и "ревьювер перечитывает таск на тестирование" у нас именно QA провели - им надоело перетещивать по несколько раз.
Ну и еще из специфики - у нас проект долгий, и текучка низкая. "После введения ревью стало аукаться через пару лет/месяцев реже, чем раньше" - ощутимый эффект, и его чувствую те же люди, а не другая команда. Так что их это очень мотивирует. Жизнь без ревью - да, согласен, еще тот ад, причем у нас это ад именно для той же команды, которая изначальный код написала :)
dasenkiv
25.08.2021 20:09Использовать комментарии
Здесь не все так просто. С одной стороны, хорошо написанный код не нуждается в комментариях. С другой стороны, мало кто пишет хороший код, бывают часто довольно сложные куски логики, которые пришлось написать из-за косяков бэкенда, ошибок в библиотеке или использования алгоритма для выполнения специфичной операции.
Я работал в компании, в которой было запрещено оставлять комментарии, и мой старший коллега рефакторил тонны кода. Такое себе было занятие.she_codes Автор
25.08.2021 22:16Конечно, встречалась с такой точкой зрения, что хорошему коду комменты не нужны.
Возможно, в каких-то отраслях программирования реально написать хороший код без комментов.
Но я всю дорогу программировала высокопроизводительные алгоритмы. Куча оптимизаций, эвристик, нетривиальные структуры данных и схемы распараллеливания. Там даже с хорошими комментариями бывает сложно въехать в чужой код.
Интересно было бы посмотреть на пример большого проекта без комментариев.
michael_v89
26.08.2021 08:47Куча оптимизаций, эвристик, нетривиальные структуры данных и схемы распараллеливания.
В таком коде комментарии нужны, но у вас написано так, как будто это общий совет для любого кода. Такого кода гораздо меньше, чем обычного. Мне кажется, лучше добавить пояснение в статью.
she_codes Автор
26.08.2021 11:21С первого раза не получилось хорошо сформулировать.
Моя мысль в том, что кодом без комментариев можно описать Что делает этот код. Но нельзя описать, Почему он делает это именно так.
Как известно, при написании программы можно сделать одно и то же разными способами. Без комментариев бывает неочевидно, почему выбран этот конкретный случай. И то самое "почему" – один из частых вопросов, возникающих при чтении чужого кода.
Бывает читаешь скудно откомментированный код и думаешь "ну зачем, зачем он тут эти сложности навертел?". А потом спрашиваешь автора и он тебе объясняет, что при определённых сценариях эти сложности вполне оправданы. И ты такая "ах вот оно что! Напиши это в комментарии"
Pro-invader
26.08.2021 20:08В теории так, а на деле пример: задача перейти с Flutter 1 на Flutter 2 в большом приложении. То есть переехать на dart null safety. Это затрагивает все dart-фалы в проекте, около четырех тысяч ошибок в проекте при переезде. Пул-реквест не соберется, пока все ошибки не будут устранены. А окончательный ПР будет огромный.
she_codes Автор
27.08.2021 10:58Я не спорю, что бывают случаи, когда ну никак пул-реквест не попилить на кусочки. Или можно, но слишком сложно.
Но в других случаях надо всё-таки стараться как-то разбивать изменения на удобочитаемые кусочки. А то есть деятели, которые по паре недель фигачат свою фичу, а потом ревью присылают на 2000 строк. Таких надо обучать.
JustDont
27.08.2021 12:37Если это одна новая фича на 2000 строк — зачем её делить?
То есть да, иногда поделить можно, но в общем случае — скорее нет. И если никакой промежуточной ценности отдельные кусочки фичи не несут, то делить pr — это в чистом виде вопрос отношения цены времени пишущего код к цене времени ревьювера.
Вот когда выкатывается новая фича на 1000 строк, а к ней еще в довесок 1000 строк изменений "ну я тут порефакторил, чтоб всё было красиво" — вот тут да, обучать нужно. Чтоб в один pr не попадала работа в разных направлениях.
Andrey_Solomatin
27.08.2021 12:22Один реквест на 1000? Не страшно.
Я бы такой вообще не ревьювил. Это бесполезно. Собралось и совсем всё не разъехалось - можно мерджить. А уже потом небольшими кусочками доводить до ума и красоты.
Такие вещи надо делать быстро. Если это затянуть и не остановить работы в основной ветке, то будет очень больно.
mvv-rus
27.08.2021 02:49Во мне процесс code review вызывает смешанные чувства: с одной стороны, это безусловное благо.
Это благо — никоим образом не безусловное. Для наемного работника оно объективно благом не является, еcли смотреть с точки зрения его материальных интересов (т.е. баланса затрат труда и получаемых денег). Точно так же, как благом для него не является и качество продукта, в создании которого этот работник участвует, и которое практика code review призвана повысить. Качество продукта — от которого зависят затраты владельца на его развитие и поддержание, а также приносимые владельцу доходы — это, если подходить с материалистической точки зрения, есть благо исключительно для владельца бизнеса, опосредовано — для лиц, которым он поручил руководить разработкой продукта: владелец им именно за это платит деньги. А code review — это инструмент для достижения целей владельца.
Так что если вы входите в число таких лиц (я ничего не знаю про вашу должность) — то для вас вопрос повышения эффективности code review актуален вполне объективно (с материальной точки зрения). То же самое — и для ваших читателей. Если же вы или ваш читатель входит в число рядовых работников, то для такого человека объективно актуален совсем другой вопрос: как легче пройти code review и не нарваться.
PS Слово «команда» в тексте статьи я видел, и основанные на этом понятии возражения для меня вполне ожидаемы, и ответ на них (отрицательный) у меня есть. Если кого это интересует — пишите в комметарий — я поясню, в чем там реально (т.е. со стороны исключительно материальных интересов) дело, и почему возражения эти не объективны.
Здесь пока писать об этом не буду, чтобы не раздувать комментарий (я и так слишком многословен).PashaPash
27.08.2021 15:11Это так, если полностью игнорировать мотивацию. Т.е. если исходить из того, что все действия разработчика мотивированы исключительно внешним фактором - деньгами. Вот только на практике разработчиками движет внутренняя мотивация - ощущение того, что они создают что-то полезное для людей, желание делится знаниями с другими, удовольствие от работы с профессионалами и прочие субъективные ощущения.
Если у вас галера - то да, там все упрется во внешнюю мотивацию и "просто пройти ревью". Поэтому там код ревью не взлетает - он там объективно вообще никому не нужен, ни стейкам, ни девелоперам. На продуктовых конторах - нет, там код ревью - это объективное благо, и команды сами его поддерживают. То же самое и с остальными best practices - от юнит тестов до скрама.
mvv-rus
27.08.2021 15:46Это так, если полностью игнорировать мотивацию.
Да, комментарий написан именно с чисто объективной, материалистической точки зрения, и я это в нем всячески подчеркивал. С упором на то, что основная причина, почему наемный работник-разработчик работает на предпринимателя, а не творит код сам или с вместе с друзьями — это необходимость зарабатывать себе на жизнь в рамках существующего способа производства.
И что описываемая вами внутренняя мотивация неадекватна объективному положению разработчика в рамках этого способа производства: по факту наемный работник отчужден от результатов собственного труда — т.е. не получает от рынка никаких материальных ни плюсов, ни минусов от того, сдеал ли он продукт хорошо или плохо, все эти материальные плюсы и минусы достаются владельцу предприятия. Ну, а я, будучи материалистом, верю, что под действием материальных факторов эта самая мотивация наемного работника будет для основной их массы стремиться к тому, чтобы соотвествовать их материальному положению.
Что касается галеры, то вы, вообще говоря, заблуждаетесь в том, что code review как инструмент обеспечения качества кода не нужен ее владельцу (stakeholder): конкуренция на рынке услуг по разработке ПО рано или поздно заставит производить из качественный продукт, несмотря на тот трэш и угар, который, наприме, судя по материалам Прекрасного.ИТ, творится на конкретном украинском рынке разработки ПО.PashaPash
27.08.2021 16:22С материалистической точки зрения внутренняя мотивация неадекватна, на практике - она объективно первична, и объяснение тут только одно - чисто материалистическая модель не описывает реальную ситуацию, т.к. просто не учитывает психологию и определяет "успех", "счастье" и "удовлетворенность" всего-лишь как "количество заработанных денег".
На практике чисто материалистическая модель дает настолько плохие результаты, что компании, опирающиеся только на нее, быстро проигрывают конкурентам, которые опираются на более общую модель и учитывают мотивацию. Даже компании "треш и угар" учитывают мотивацию - например, обрабатывают студентов, и пропускают их через себя за год-два, пока у тех внешняя мотивация (деньги) работает, и закладывают "человек захочет внешней мотивации и уйдет" в свою модель. Просто они осознанно выбирают нишу, в которой можно жить чисто за счет внешней мотивации сотрудников, без тестов и без ревью.
Простой тест на реальность внутренней мотивации - на текущем месте вам недавно повысили зп на 500$, ревью - через полгода. Вы получили оффер из другой конторы на 10$ выше. Вы, как материалист, примите оффер без раздумий? Или вас что-то удержит?
mvv-rus
27.08.2021 16:41Обсуждая вопросы стратегии предприятий «здесь и сейчас» мы уклонимся сильно в сторону от темы статьи — которая все же написана про работника и взгляд с его позиции (и лично мне интересна именно с этой стороны).
Поэтому предлагаю обсуждение этого вопроса, если вам онинтересен, перенести куда-нибудь на следующий раз.
she_codes Автор
27.08.2021 15:24Я может немного идеалистка и витаю в облаках. Но у меня пока такое работает: чем больше заработает мой работодатель, тем скорее он поднимет мне зарплату.
Хорошие работодатели обычно не только под себя гребут, но и с сотрудниками делятся. А иначе – текучка кадров, низкий комплаенс – кому это надо?
В общем, лично мне выгодно работать в интересах работодателя.
mvv-rus
27.08.2021 15:52Этот идеализм адекватен действительности ровно потому, что рынок труда в области разработки ПО в странах ex-СССР растет, вместе с ростом стоимости разработчиков на рынке.
Я, когда уже более 15 лет назад ушел из программирования в системное администрирование Windows, наблюдал там точно такую же ситуацию, потому что рынок специалистов в этой области тоже рос аналогчиным образом.
Но с тех пор ситуация на этом рынке поменялась на противоположную, и подобный идеализм на нем уже выглядит неоправданным (хотя ещё и встречается).
she_codes Автор
27.08.2021 11:04Я может немного идеалистка и витаю в облаках. Но у меня пока такое работает: чем больше заработает мой работодатель, тем скорее он поднимет мне зарплату.
Хорошие работодатели обычно не только под себя гребут, но и с сотрудниками делятся. А иначе – текучка кадров, низкий комплаенс – кому это надо?
В общем, лично мне выгодно работать в интересах работодателя.
amarao
Я обычно на ревью пытаюсь понять, что человек сделал, зачем и как. Если я не могу этого понять, то ревью failed (либо я не подходящий ревьюер, потому что я не знаю проекта, либо объём изменений слишком большой).
... На самом деле есть два вида ревью - по "речекрякам" и "по сути". По сути ревью может делать только человек, глубоко вовлечённый в проект, который понимает куда проект идёт и как он построен.
Ревью на речекряки проще - мы смотрим, читаем ли код, нет ли странных выкрутасов (когда есть простые решения) и т.д.
Но ревью "по сути" может занимать времени не меньше, чем время написания MR, хотя оно самое продуктивное (как для проекта, так и для читающего, и для пишущего).