Как стать автором
Обновить

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

Время на прочтение3 мин
Количество просмотров13K
Всего голосов 34: ↑33 и ↓1+32
Комментарии64

Комментарии 64

НЛО прилетело и опубликовало эту надпись здесь

так вот почему у Windows 10 столько проблем с апдейтами.

Вот я смотрю все ругаются на придирки к "красоте кода", "паттернам", названиям переменных, форматированию (ну ладно, форматирование и правда автоматом можно сделать), но блин, как у вас вообще получается отсмотреть и понять, что делает код (и делает ли он это так, как задумывалось), если из-за всех вышеперечисленных проблем код невозможно "распарсить"?


отсюда же и растут все эти "diff 500+ -> ок, не глядя"

Я вот типичный стайлгайд-фашист и пусть меня закидают камнями.


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

Потому что на С++ пишу, и как по мне любой С++ проект превращается в лютое адище, если не быть фашистом.

Если быть, то всё равно превращается.

Лично у меня есть единственный способ нормально отсмотреть крупный PR — запулить к себе ветку и посмотреть, что там происходит.
Иначе — только и остается, что апрув неглядя. Но какой в этом смысл?

Всё так. У нас в Yii даже это есть в чеклисте — затащить локально и смотреть в IDE.

Было бы крайне удобно, если бы можно было еще писать комменты в шторме и сразу их передавать в PR на гитхабе. А то скачешь между штормом из гитхабом и пытаешься контекст не потерять — сначала «так, А где в PR эта строчка?»… находишь и «а чего я хотел тут сказать?».

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

А разве нельзя?

ну значит я чего-то не знаю. Как то можно напрямую в шторме кидать комменты в gitlab? если да, то я буду очень этому рад
НЛО прилетело и опубликовало эту надпись здесь
В моем понимании, code-review — это не процедура обеспечения качества ПО (на что почему-то часто надеются...). В отличие, например, от pair-programming. Потому что это же как скрытые работы в строительстве: если ты лично видел как бетон лили, и что в него клали — тогда можно ручаться за результат. А если тебе показывают уже готовое — то чтобы его освидетельствовать — это куча мала экспертиз и расходов… И можно так аккуратно дерьма внутрь наложить, что оно все-равно рухнет со всеми экспертизами и подписями… :-(

Что тогда дает ревью кода? По-моему, только обратную связь автору. То есть это в чистом виде инструмент обучения. Отсюда — нет смысла делать ревью на 5000 строк. Это никого и ничему не научит… Да и вообще, не следует давать малоопытным разработчикам писать большие куски системы. Код-ревью нужны в период адаптации (чтобы один залетевший дятел случайно не разрушил цивилизацию), и для воспитания джунов. На уровне миддла, разработчик уже должен сам устойчиво контролировать качество своего кода и уметь вовремя обращаться к старшим, если не очевиден наиболее уместный способ реализации той или иной фичи. А с третьей стороны, для обучения гораздо важнее обратная связь по ходу разработки, а не в конце. В программировании слишком много способов сделать работу неправильно и неэффективно, чтобы надеяться методом коде-ревью отучить разработчиков делать неправильно. Надо, наоборот, учить правильным и эффективным методам работы.
Сравнение с бетоном и стройкой хорошее. Но такой подход крайне трудозатратен :) Поэтому Code Review частично спасает — есть шанс не пропустить совсем треш в релиз.

Насчет милдов врядли с вами соглашусь — любой код должен быть отсмотрен хоть кем-то перед выливкой. Иначе можно пропустить какие-нибудь критичные вещи, которые не видно из-за «замыленного глаза»
ппц. Опечатался — с вами НЕ соглашусь)

Не согласен. Ревью нужны, например, мне для моих pull request по Yii. И это при том что я этим проектом занимаюсь уже много лет. Глаз замыливается, какие-то штуки не учитываю… всякое бывает.

Вот я именно про это! Вы полагаетесь на код-ревью для обеспечения качества. При этом у вас нет гарантий, что ревьювер понимает точно, что вы написали. Что он с утра свеж и у него не болит голова, и т.д. Мы где-то в районе 2008 года поняли что в нашей сфере это добром не кончится — взяли в работу идеи testable design, и начали писать тесты. И теперь меня не очень интересует, замылился глаз у разработчика или нет. Если он замылился настолько, что это ломает normal execution path — тесты покажут. А искать глазами ошибки, которые не выявляются тестами — ну так себе занятие… И да, естественно — у нас написание тестов является обязанностью разработчика.
Твой любимый мем про код-ревью...
Ну а что вы хотели? Только в IT инженер вообще, даже в теории, не несёт никакой ответственности за свои упущения в работе. Если такое сделает врач, юрист, строитель, автомеханик, пилот, кассир, водитель автобуса, машинист метро и даже полицейский, то его ждёт, как минимум, конец карьеры, а как максимум, тюрьма. И только в IT везде развешены дисклеймеры «ой, а мы в нашем детском садике ни за что никогда ответственности не несём, кликните галочку, что согласны». Максимум, что растяпе может быть, это с работы уволят, а на следующий день устроится в соседний офис.

Зависит от проекта.

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

Справедливости ради, врач несёт ответственность только за назначенное лечение. Если пациент будет принимать другие лекарства или например скроет, что у него аллергия, к врачу претензий не будет никаких.

Например в проектах, где ошибка в программе может принести реальный ущерб. Недавние аварии с боингом явный тому пример.
И что, кто-нибудь уже сидит в тюрьме за убийство 346 человек?
А почему сразу нужно сажать?
Каждого врача сажают, если его пациент умер? Каждого строителя сажают, если мост упал? Каждого конструктора сажают за упавший самолёт?

Всегда есть человеческий фактор. Только в случае с другими профессиями есть хоть какие-то ограничения в виде инструкций иизмеримые показатели типа качества бетона или соответствие лечения правилам.
Что есть у программистов? Вот серьёзно. Я знаю только ТЗ, а всё остальное программисты практически не могут контролировать.
А почему сразу нужно сажать?
А какое наказание Вы предлагаете? Погрозить пальчиком?

Каждого врача сажают, если его пациент умер? Каждого строителя сажают, если мост упал? Каждого конструктора сажают за упавший самолёт?
Я не юрист, но насколько мне известно, если доказана явная халатность, то да — уголовная ответственность.

Что есть у программистов? Вот серьёзно.
Ну, для начала не удалять тест просто потому, что он фейлится. Не править и не аппрувить код, работу которого на 100% не понимаешь. И ещё сотня столь же очевидных правил.

И мне кажется, скоро до законодателей дойдёт, что в IT нужны такие же строгие правила, как в строительстве, медицине и т.п.
И мне кажется, скоро до законодателей дойдёт, что в IT нужны такие же строгие правила, как в строительстве, медицине и т.п.
И как только это произойдёт, прощай развитие, IT законсервируется на века и каждую новую практику придётся годами с боем отбивать у всяких «комиссий по IT».
1. По Вашему медицина, авиация, строительство, транспорт и ещё куча регулируемых отраслей не развиваются?
2. По-моему, IT начнёт развиваться даже быстрее, ведь сейчас до 80% времени занимает борьба с последствиями чьей-то халатности. Не будет ситуаций, когда две недели извёл на то, что пытался понять почему же сторонняя библиотека перестаёт работать в новолуние.

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


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


Многократно обсуждалось же и на хабре в том числе: большая надёжность стоит больше. И по времени, по деньгам. И эта стоимость ложится на потребителя. Хотите платить больше за большую надёжность не критичного для жизни софта или железа? Ваше право. Но я вот — не хочу. И сразу же уточню — не пытайтесь переиначить мои слова в "он хочет фиггю, зато но дёшево". Нет, я хочу сбалансированную ситуацию с ценой и качеством — и на мой взгляд она уже достигнута сегодня везде, где нет монополии. Подход "давайте всё зарегулируем" скорее всего просто приведёт к неадекватному росту стоимости и сильному уменьшению разнообразия софта.

Ну давайте отменим регулирование еды и фармацевтики? Знаете сколько сразу появится недорогих и новых лекарств? А как еда подешевеет! Хотите? Нет? Вот и я не хочу. А если авиацию перестать регулировать, знаете как авиабилеты подешевеют?
А что в домашнем ПК изменилось за последние 20 лет, кроме того, что раньше всё летало на 128 мегабайтах, а сейчас 8 гигабайт памяти — это уже мало? А что Вам в автомобильной магнитоле не хватает?
Хотите сбалансированную ситуацию? Дык вот сейчас, она не сбалансирована.
У вас есть только две крайности: нет регулирования вообще или регулируем всё до последней буквы?
Опять пропагандистский приём. Сейчас у нас регулирования нет вообще. А насколько сильно регулировать — отдельный и важный вопрос.
Ваше право. Но я вот — не хочу.

а я хочу выбор. Потому что по сути — сейчас его нет.

А выбора не будет — без регулирования низкокачественное будет всё, так рынок устроен.
  1. По Вашему медицина, авиация, строительство, транспорт и ещё куча регулируемых отраслей не развиваются?

очень медленно развиваются. Если бы в айти не было такого бума — мы до сих пор на ЭНИАК сидели и ни у кого не было айфонов...

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

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


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

вот как пример. С/C++ — регуляция через комитет бюрократов. Стандарты. Ну, и что? Все равно каждый разработчик компилятора пилил свои собственные фишки и стандарты отстали от жизни чуть более, чем полностью. По ощущениям, сейчас ситуация стала выравниваться. Но это сейчас.

По Вашему медицина, авиация, строительство, транспорт и ещё куча регулируемых отраслей не развиваются?
Отличный пример. Клинические испытания лекарство и сертификация самолётов занимают в прямом смысле годы и требую миллионов, а иногда и миллиардов долларов. Многим «современным» моделям по 15-20 лет, некоторые так вообще — основательно пропатченные старички из 70-х. Оно вам точно надо?
Не будет ситуаций, когда две недели извёл на то, что пытался понять почему же сторонняя библиотека перестаёт работать в новолуние.
Ну да, зато будут передаваемые по наследству научные проекты библиотек, которые гарантированно не падают в новолуние.
Ну и на моей практике, большинство таких проблем вызвано не самой библиотекой, а неумением её правильно использовать, и решается через RTFM.
Оно вам точно надо?
Если по другому никак — то да. Лучше так, чем бардак как сейчас.

Ну и на моей практике, большинство таких проблем вызвано не самой библиотекой, а неумением её правильно использовать, и решается через RTFM.
Это я уже слышал 100500 раз. Обычно после этого показываю текущую проблему и говорю «ну давай, орёл, утри мне нос, покажи как тут надо правильно использовать». Обычно орёл быстро перестаёт хлопать крыльями и признаёт, что вот, да, это реально поломано и пользоваться нельзя.
Лучше так, чем бардак как сейчас.
Ну если уж вот это для вас такая большая проблема, что ради неё стоит вводить массовые расстрелы посадки, то я даже не знаю, как вы вообще в этом мире выживаете.
Обычно орёл быстро перестаёт хлопать крыльями и признаёт, что вот, да, это реально поломано и пользоваться нельзя.
Уверены, что сами-то останетесь орлом, когда чтоб добавить на сайт форму логина, надо будет защитить докторскую диссертацию?
Вот кто-то один с докторской диссертацией напишет эту форму логина, без багов и поедания памяти и все её будут использовать. Как это сейчас происходит с реализацией криптоалгоритмов, их никто на коленке давно не пишет.

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

А потом найдут уязвимость в одном из алгоритмов, и вы будете сидеть на дырявой форме пару лет, пока новая не пройдет сертификацию. Щикарно.
Многим «современным» моделям по 15-20 лет, некоторые так вообще — основательно пропатченные старички из 70-х

ну, а мы в айти ездим на технологиях 30-летней давности. Unix (linux), сети, протоколы, что там еще… Никто еще не умер от этого )

А какое наказание Вы предлагаете? Погрозить пальчиком?

Недовольство «безответственностью» IT сферы выразили вы, а не я. Меня всё устраивает.
Я не юрист, но насколько мне известно, если доказана явная халатность, то да — уголовная ответственность.

Если халатность да, будет нести ответственность. А если врач всё сделал по правилам и стандартам, к нему вопросов после расследования не будет.
Знаете фразу «У каждого врача есть своё маленькое кладбище»? Она не от хорошей жизни получилась.
Вопрос вам на подумать. Привезли тяжелого пациента, который без операции умрёт в любом случае, а при операции вероятность смерти 30%. Почему в этом случае хирурга не наказывают, если пациент умрёт на операционном столе (вариант, что врач полоснул скальпелем по горлу не рассматриваем)?
Кроме того есть такая вещь как стечение обстоятельств. Когда конструктор моста закладывает прочностные характеристики, вряд-ли будет рассматриваться вариант, что в начале произойдёт мощное землетрясение, потом фура с газом влетит в опору и взорётся, а в довесок ещё террорист решит бомбу взорвать, в результате чего мост упадёт. Тут есть халатность или это маловероятная ситуация, которую нельзя было предусмотреть?

На моей практике был случай, когда тестировщик прислал баг, который происходил только в случае строгого повторения порядка 20 действий. Если хоть парочку из этой цепочки заменить, бага не было. Обнаружил он это случайно, когда при тестировании стал упарываться и проводить вообще нереальные в работе варианты. Проблема была последствиях, которые конкретно в этой ситуации оказывались серьёзные.

Внимание вопрос. Меня как разработчика надо наказывать, если на промышленной среде это бы произошло? Или всё-таки можно списать на стечение обстоятельств?
Ну, для начала не удалять тест просто потому, что он фейлится. Не править и не аппрувить код, работу которого на 100% не понимаешь. И ещё сотня столь же очевидных правил.

Воспринимать гротескный ролик как реальность, которая происходит везде? Браво. Вы слышали про сатиру и гротеск, где всё специально преувеличивается?
И мне кажется, скоро до законодателей дойдёт, что в IT нужны такие же строгие правила, как в строительстве, медицине и т.п.

Прощай IT. Если это произойдёт мы так и будем до сих пор сидеть на фортране, т.к. новые языки недостаточно надёжны и нас посадят за них.
1. Не уходите от ответа на мой вопрос — какое наказание Вы предлагаете за халатность?
2. Мы все можем отличить халатность от ситуации когда пациент не мог выжить, а мост устоять — не надо тут передёргивать.
3. Про Чернобыль слышали? Там вот тоже получилось «нереальное стечение обстоятельств». А про Фукусиму? А про Ухань? А про сотни сгоревших заживо, потому что при падении давления газа в трубопроводе утечку искать не стали, просто увеличили подачу газа для восстановления давления, слышали?
4. Наказывать Вас или нет должен будет решать суд.
5. Вы слышали про сатиру и гротеск, где всё специально преувеличивается? — Да, это гротеск, но, увы, не столь далёкий от реальности, как Вы пытаетесь изобразить.
6. Кто Вам сказал, что мы будем сидеть на фортране? Будут появляться новые операционные системы, языки и фреймворки, только надёжные. И возможно, даже в 5 раз быстрее, отрасль перестанет постоянно подскальзываться на собственных соплях, ведь сейчас до 80% времени разработчика занимает борьба с последствиями чьей-то халатности. Не будет ситуаций, когда две недели пыташься понять почему же сторонняя библиотека перестаёт работать в новолуние. (это, кстати, к Вашему примеру про «нереальные варианты»)
И кстати, знаете почему до сих пор всё ещё работает куча кода, написанного на Коболе? Потому что при теперешней инженерной культуре написать то же самое с той же надёжностью, увы, не предсталяется возможным.
Или всё-таки можно списать на стечение обстоятельств?

Давайте отделим наказание и катастрофу. Любая катастрофа — это стечение обстоятельств. К сожалению, мешки из кожи и мяса не в состоянии предусмотреть ВСЕ возможные ситуации. Посмотрите "расследования авиакатастроф" — очевидно, что нужны правильные подходы, которые минимизируют и импакт, и вероятность нестандартного стечения обстоятельств, правильные регламенты и все прочее. А когда "дикий запад" — факапы будут случаться постоянно (почитайте — и гитлаб базу удалял, и облачные сервисы регулярно ложатся, и додо пицца чуть было не зафакапила федеральную рекламную кампанию и сами об этом писали тут).

А какое наказание Вы предлагаете? Погрозить пальчиком?

экономическое наказание (=штраф и возмещение убытков) гораздо эффективнее. Профессионала отправлять в тюрьму — это категорически невыгодно по всем пунктам.


И мне кажется, скоро до законодателей дойдёт, что в IT нужны такие же строгие правила, как в строительстве, медицине и т.п.

с этим соглашусь. Нужны четкие правила игры.

экономическое наказание (=штраф и возмещение убытков) гораздо эффективнее.
Дык и этого тоже нет.
Если ПО пишется для конкретного заказчика и требуется надёжность, в договоре часто прописывают ответственность разработчика за косяки.
Если же говорить про ПО, которое может поставить условно «каждый», сразу возникает куча вопросов к железу, которое может тупо глючить, ПО не удовлетворять системным требованиям, на компьютере может быть вредоносное ПО и т.д. и т.п. В общем ситуации, которые разработчик ПО не может контролировать никак.
Как вы предлагаете регулировать такую ситуацию, чтобы соблюсти баланс между разработчиками и пользователями?
Как в регулируемых отраслях, разбираться в каждом конкретном случае, что именно и как глючит.
Вегулируемых отраслях, где тот же конструктор или разработчик лекарства может сказать: «Для моста нужен бетон М100500», «для лекарства нужно 0.003 миллиграмма действующего вещества чистоты 99.9999%» и после этого не должно не произойти ситуации, что бетон решили применять другой (причём в процессе заливки моста), в лекарстве использовать действующие вещества с другой чистотой и т.п. Если же это произойдёт, будет проблема на конкретном месте, которая локализована и её можно исследовать и виноват будет не инженер, а принявшие решение использовать другие составляющие.

В случае с ПО в конфигурации может поменяться всё: железо, операционная система, драйвера, стороннее ПО. Разработчик ни на что из этого не может повлиять.
Вы лично готовы отвечать, если при работе именно вашей программы будет зависать компьютер, но причина не в вашем ПО, а в новом процессоре? (Вот типичный пример habr.com/ru/company/pt/blog/274939)
В теории конечно можно написать, что программа протестирована только на оборудовании X, Y, Z. Но вы же понимаете, что на практике это утопия, которая нереальна.
У меня был случай, когда одна моя подопечная уходила в слезах после моего код-ревью. Зато спустя какое-то время её PR практически не требовали правок, а я мог абсолютно спокойно доверить ей ревью других разработчиков.


Звучит практически как текст из «1984» Orwell'a.
У меня в команде так новенький не выдержал постоянного стресса и ушел. Хотя я всеми силами старался ему помочь, по доброму, чтобы его код был лучше. Оказалось, он воспринимал на свой счет, а я не распарсил (
Я стараюсь всегда избегать в своих PR ревью придирок ко всему, что не является багом, ударом по скорости кода, или отклонением от бизнес логики. Наверное перестану себя уважать, если когда-нибудь сделаю коммент по поводу отступов, скобок и пр.
code-style проверки автоматизированы в CI и непонимаю людей, которые до сих пор этого не сделали
Самое раздражающее в ревью — когда пулреквест возвращают на первой же ошибке (лишний пробел? не так названа переменная?), ты её исправляешь и тебе возвращается реквест уже со второй придиркой…
Когда придирки все исправлены, выясняется что требовалось сделать совсем другое, надо всё переписывать.
Капец. Прочитал, вспомнил аж передернуло:) У нас так клиенты работы принимают иногда…

Вообще концепция «проверки чего-либо» до первой ошибки и потом разворот — достаточное условие для наличия отдельного котла в аду)

Я так делал много раз :)

Ну ты с помощью Yii карму очистил себе:)
Ну ты с помощью Yii карму очистил себе:)

Смотря с какой стороны посмотреть :D


Помнится, где-то в заначках Саши был ишшью или пост на реддите, где его обвиняли во всех смертных грехах, а Yii — творением дьявола.

Ага, еще при обсуждении есть такой тип людей — напишешь ему письмо с 5 четко выделенными и пронумерованными вопросами по обсуждаемому проекту и получаешь письмо с ответом только на один из вопросов. В итоге потом переписка вся в кашу превращается и потом найти в ней нужные детали крайне сложно.
Когда придирки все исправлены, выясняется что требовалось сделать совсем другое, надо всё переписывать.

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

Жестко. Я не слышал лично о таких случаях и прямо страшно становится от того что увольняют за «долгие PR».
Да, адочек. Я даже как-то раз уволился из одной компании именно из-за таких ревью. После нажатия Create request поднималось давление, поскольку точно понимал, что сейчас будет волна непоследовательных придирок, в конце которых запросто может быть вообще «переделать заново»
НЛО прилетело и опубликовало эту надпись здесь
Зарегистрируйтесь на Хабре, чтобы оставить комментарий