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

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

Если коротко — не будь упрямым мудаком) Думаю этого достаточно, чтобы описать всю статью.
Упрямый мудак — слишком абстрактное понятие, а здесь как раз уточняется, как определить свою вхожесть в него.
Вовсе нет. Я сам часто руководствуюсь правилом «не будь м*ком», но применительно к code review автор делает очень верные выводы. Их, конечно, теоретически можно вывести, что называется, из общих соображений, но лично мне это понимание пришло лишь с многолетним опытом.
Коротко обрисую ситуацию: В команду пришла новая сотрудница, которая фигачит говнокод и слушает только начальника (напомню, автор находится на одном уровне с сотрудницей). При этом автор гораздо более компетентен технически чем сотрудница и, я так полагаю, чем менеджер. Напомню, сотрудница в компании уже нескольно лет и выработала поведение при ревью. Вместо того, чтобы жадно впитывать новые знания, она вредит продукту, команде и компании.
Вывод: не теряйте время, ищите людей таких же целеустремленых и желающих учиться, как вы.
Однако автор не влияет на подбор персонала, потому я предложу другой вывод: налаживайте отношения с коллегами, какими бы тупыми говнокодерами они вам ни казались. Более того, может оказаться, что ваше мнение о них изменится в будущем.
И будет у вас идеальная компания с идеальными разработчиками, живущими где-то в параллельном мире.
Я же говорю про людей которые хотят учиться. Почему бы не делать компанию только из таких людей.
потому что такие люди стоят на порядок дороже
их еще и на порядок меньше
и еще я с ужасом представляю себе команду из 7-8 человек, все из которых хотят учится.

Либо я не понял сарказма, либо я не понял вообще. :)
Почему с ужасом? Они будут тратить на обучение слишком много времени вместо того, чтобы работать?

Ну, на работе ведь работают, так что какая разница сколько они свободного времени тратить будут. Я например себе иногда позволяю пол часика выделить в рабочее время на интересные вещи, но вообще все самообразование, в т.ч. и по неинтересным но нужным работодателю темам только в свободное время, я же свое рабочее время продаю. Иногда конечно нужно что изучить «вот прям щас», но это уже по производственной необходимости, если сроки поедут в случае изучения в свободное время.
Плохо, что вы рабочее время продаете, а не вклад :)
У 1сников распространен термин «жопочасы» для этого :)
А я кем работаю? Если надрываться и решать задачи быстрее и лучше — никто не оценит, только больше навалят при той же оплате. Ну и естественно что отработано в месяц должно быть не меньше чем 8*количество дней рабочих. Именно отработано, а не на обучение. На какой результат в таких условиях работать можно?
потому что команды из них не выйдет…

Ну, у программеров учёба — неотъемлемая часть профессиональной деятельности. Хоть ты джуниор, хоть синьор… Надоело учиться — конец профессионализму.
Или я опять неверно понял?

просто для хорошей команды именно как команды, важны совсем другие качества, совершенно перпендикулярные, такие как например: «умение находить компромис», «не зашкаливающее самомнение», «умение работать на общий результат» ну и т.п.
Потому что если вы вывалите на человека 100500 замечаний, то любой, сколько угодно «желающий учиться» человек «встанет в позу». Особенно если вы будете наставивать на каких-то мелочах (или чем-то, что кажется мелочью «с другой стороны»).
Например, потому что «хочет учиться» это не неотъемлемое свойство человека, а скорее одно из его состояний.
НЛО прилетело и опубликовало эту надпись здесь
я долго руководил командой из 10 разработчиков из которых 6 были девушки. Мой ответ — нет. Но есть исключения, когда две девушки могут друг друга нелюбить, скажем так, но это все таки больше исключение чем правило.
При этом автор гораздо более компетентен технически чем сотрудница и, я так полагаю, чем менеджер.

Нет. Автор думает, что более компетентен, но несмотря на это, не может доказать свою точку зрения новичку. Либо он не настолько компетентен, либо не умеет общаться (именно на это упор в статье), либо изменения того не стоят и спорны.
Либо выбранный формат код-ревью не дает возможности полноценно высказать свою точку зрения.
никто не запрещает ему в дополнение к выбранному формату код-ревью подойти к новичку и обговорить-обсудить спорные моменты. возможно, объяснив свою точку зрения, он продвинется дальше чем у него вышло, выписывая повести в код-ревью.
Но он почему-то этого не сделал. Возможно, он верующий в инструмент для код-ревью…
скорее, недальновидный сотрудник, не способный сделать шаг назад и рассмотреть картину целиком, замкнувшийся на факте того, что новичок не хочет выполнять разумные (с его точки зрения) требования изменить код. и вместо того, чтобы решить проблему при помощи диалога и продолжить работу, он три недели провел, злясь каждый день, пререкаясь в код-ревью с новичком. возможно, технически этот человек отличный специалист, но soft skills у него не на высоте, что отражается на продуктивности команды.
Вывод: не теряйте время, ищите людей таких же целеустремленых и желающих учиться, как вы.

абстрактные Мэллори/Вася/Петя/Айрат могут быть отличными профессионалами своего дела и некомпетентными чуть вне его. Задача ревью в первую очередь — не допустить говнокод в продакшн, во вторую — научить сотрудника. Если научить не получается (иногда это на самом деле и не нужно), то зачастую лучше и проще сделать как надо самому.
и потом все сам так и будешь делать
очевидно же, что прибегать к такому методу решения следует лишь в единичных случаях. Зачем пытаться научить коллегу технологии, которой он воспользуется единожды в жизни, если быстрее сделать самому?

Ну такое. Сейчас работаем вдвоем с человеком, желающим учиться и вообще таким же как я целеустремлённым. Код ревью иногда в ад превращается. Оба рогом упираемся и пишем друг другу поэмы в комментариях. Так что команда, состоящая из золотых специалистов — не всегда гарантия безстрессовой работы. Где есть два человека, там конфликт неизбежен.
Но в последнее время заметил, что код ревью всегда проходит проще, когда ревьювер (в данном случае я сам) искренне задаёт себе вопрос "как я могу помочь этому человеку?". Обычно сразу после этого вопроса приходит соломоново решение. Главное вспомнить о нём вовремя :)

Где есть два человека, там конфликт неизбежен.

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

Ах как вам повезло что я у вас не работаю!
Но, если серьёзно, то я бы лучше не хвастался хорошими отношениями с людьми, а то можно беду накликать.

Я не суеверный. Наши отношения в наших руках)
Просто я хотел сказать, что где два человека — конфликт вероятен, но не неизбежен)

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

Не всегда так, иногда компании нужны не стахановцы, а те, кто за свои 8 рабочих часов будет делать, что от них требуют. Да и не по карману многим целеустремленные…

Вывод: Только воспитывая можно добиться таких результатов
Дичь какая-то. Сказали переделывать — переделывай. Нравится, не нравится, считаете что ваш код лучше — вооружитесь от начальниками списком правок и в случае чего тыкайте туда пальцем, мол сказали делать так. А тут какое-то раздувание проблемы на ровном месте. Программисты похоже стали забывать, что отсутствие «вербальной» субординации не отменяет фактическую и начальник остается начальником, а значит требования его нужно выполнять. Как максимум обрисовать ему свое видение ситуации, но не настаивать на своем решении как единственно верном.
Речь о peer-review, насколько я понял.

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

>Любой код ревью портит отношения между коллегами по разработке
У нас в команде не портит. Замечания все воспринимают как должное

>Также любой код, как известно (мне, по крайней мере), может получить приставку г.
Одна из прелестей ревью — через некоторое время уровень и ожидания от кода в команде синхронизируются и люди уже не отдают на ревью г-код
У нас тоже не портит. Потому что любое код-ревью — это помощь со стороны (а-ля две пары глаз лучше, чем одна), а не менторство «я знаю лучше, что и как, делай, как говорят».
В случае сервисов, где баг может послужить причиной алерта и соответственно звонка посреди ночи с последующим дебагингом, за любой косяк, найденный в код-ревью, еще и благодарят от души.
Я вообще обожаю посылать свой код на ревью. Жадно хватаюсь за каждое предложенное улучшение, даже если это code-style или небольшой трюк, который позволяет написать код короче и чище. Не говоря уже о серьезных недочётах, мне же потом и поддерживать.

Сделал такой маленький эксперимент — высказываю свое мнение (подчеркивая это) и получаю кучу минусов — это и есть естественная реакция на любой код ревью

Что-то с вами не так. Люди намекают вам, что ваше мнение не незыблемо, возможно стоит его пересмотреть.

А у вас зыблемо?

Любой код ревью портит отношения между коллегами по разработке

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

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

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

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

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

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

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

То есть, они сидели в одной комнате, но вместо того чтобы выяснить отношения по человечески, 3 недели перекидывались письмами? Я бы уволил обоих за имитацию деятельности вместо работы.
Угу, у нас есть негласное правило — начался срач в ревью — садимся в переговорку и обсуждаем. Пару раз приходилось привлекать арбитра :)
Всецело поддерживаю, единственный нормальный (без выпячивания XCD) комментарий в этом топике! Два бездельника тратят деньги компании на вопрос, который выеденного яйца не стоит… kvasvik, в нашем с Вами идеальном мире такого бы не могло случиться!

Но, увы: уж сколько я за свою профессиональную деятельность в software development наблюдал таких… «работничков»… По средневзвешенной оценке, порядка 50% в «молодых стартапах», и до 80-85% в «старых, солидных компаниях» (там, где врастают корнями в свое офисное кресло, и десятилетиями колупают с умным видом протухший говнокод). Как правило, такие «работнички» вместо работы очень любят постить во всевозможные блоги, а также обожают всяческие митинги — «типа профессиональная этика» (а, может, просто не любят/умеют/хотят) не дает им сидеть в рабочее время в социальных сетях/играть в игры/ваять homebrew «нетленку»/«халтурить», вот потому они и так рады бессмысленным митингам и «конфликтам», высосанным из пальца.

100% no hire! Но, увы, в идеальной вселенной… В реальной я сам лично наблюдал, как человек просто-напросто «подставил» компанию, принеся реальный убыток, мотивируя лишь тем… что у сына школьная игра в бейсбол на уровне районных школ (т.е. даже не county, и не штата), которую он не может пропустить! И, самое странное, что его не уволили тут же — не знаю даже, почему (возможно, испугались судебных издержек — сложно сказать).
Не захотел овертаймить в субботу и пошел на игру сына? Мужик!
Нет. Отказался билдить релиз (который ему по рабочим обязанностям положено делать, как релиз менеджеру), «послал» CTO и ушел в два часа дня. И это на фоне showstopper-а и просьбы от клиента с многомилионным контрактом выдать багфикс ASAP.

P.S. Понятное дело, что сделали и без него (вообще, по хорошему, его должность была практически чистая синекура), но сам факт…
дичь какая-то. чувак занимается тем, что старается не обидеть коллегу, т.к. она просто не хочет делать то, что ей рекомендует более опытный коллега.
Видимо менеджеры-технари смотрят на статью несколько иначе. Еще раз вводные из статьи
Она никогда раньше не писала на Python и никогда не разрабатывала систему поверх неуклюжей легаси-системы, которую мне приходилось поддерживать.

Девушка написала свое первый код на Питоне, и не хочет выполнять замечания более опытного коллеги который, судя по всему, повел себя как упрямый баран и вместо помощи выкатил ей список на 100 листов замечаний ожидая от неё кода синьора да еще и в легаси, которое он сам долго причесывал?! Кто из них двух разумнее? Если бы ваш первый код зеревьювил бы по самое «не балуйся» гуру из Гугла? Вряд ли бы вы сейчас программировали, т.к. редко кто готов чувствовать себя полным ничтожеством.
Оба хороши. И ревьюер мог снизить планку, и коллега мог спокойно поправить все замечания, разобравшись с тем что хочет ревьюер.
и вместо помощи выкатил ей список на 100 листов замечаний

А замечания это разве не помощь? (Разумеется, я предполагаю замечания вида "тут желательно сделать отдельную функцию" или "не соответствие стилю XYZ" при наличии руководства по стилю, а не "код говно, переделывай").
Есть совершенно конкретный список, что не устраивает, и чего хотелось бы видеть вместо. Что ещё может быть "помощью" на ревью?


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

Ревью от гуру из Гугла имеет смысл для кода для гугловского продукта, но тогда это не может быть чей-то первый код (не считая тривиальнейших правок в две строчки).
И точно так же гуру из Гугла может быть ревьюером для кода своего ребёнка, смягчая требования до подъёмных соответственно возрасту и опыту, но этот код не будет предназначен для гугловского продукта.


Девушка написала свое первый код на Питоне

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

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

Если бы ваш первый код зеревьювил бы по самое «не балуйся» гуру из Гугла?

… то некоторых моментов в том коде я бы сейчас не стыдился :-) А ваши предположения о том что я бы сейчас не программировал не имеют под собой основания.

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

Вполне возможно, что его замечания не питоно-специфичны, так что ее малый опыт на данном ЯП и не имеет значения.
Автор странный. Коллега первая нарушила правила нормального взаимодействия, проигнорировав фидбек. Кнопочка review rejected и до свидания чинить свои ~100 ошибок. в следующий раз научится, что каждый коммент ревьюверов надо или выполнять, или открывать по нему обсуждение, если не согласна.
О, вот и роботы подтянулись, упомянутые в первой статье. Автор пытается получить результат, а не с помощью силы почесать свое ЧСВ. В одиночку большой проект не вытянуть, а значит надо уметь кооперироваться, договариваться и иные непонятные для роботов слова.
Не надо договариваться с менее опытным сотрудником, если он не понимает как надо сделать и при этом еще и качает права. Ему надо популярно объяснить, что так дела не делаются, отправить переделывать, а в случае повторения выгонять к чертовой матери — все равно не работает.
А потом на проекте все зависит от одного человека — выгоревшая суперзвезда, у которой глаз дергается. Договариваться надо со всеми участниками, а иначе это верный способ убить проект. Вообще, Я бы рекомендовал свое ЧСВ как-то иначе выражать. Спортом там заняться, где результаты формализованы, или кибер-спортом. Там, да, можно точно сказать, что ты круче и бить пяткой в грудь.
lpwaterhouse правильно говорит.
В коде тоже довольно легко формализовать оценку отдельно взятого фрагмента.
Например, если я в ревью придираюсь к тому, соответствует ли код стандартам PEP-8 — то это означает, что у нас заранее было оговорено, что мы придерживаемся PEP-8.
Если то, как предлагаю сделать я — быстрее/безопаснее/надёжнее/читабельнее, и я могу это доказать — значит так и надо делать. Если мне, при ревью моего кода, аргументированно такое доказывают — я не упираюсь рогом и не трачу время ни ревьювера, ни своё, ни кого-либо ещё из команды. Это то, что я считаю профессионализмом. Формулируя чуть короче — профессионализм, это когда стараешься делать своё дело лучше и быстрее, избавляясь от того, что тебя сдерживает.

А заниматься выстраиванием отношений в команде вместо работы над продуктом — это хипстерская херня. Да, коммуникация важна. Да отношения с коллегами надо поддерживать.
Но всё это не должно быть самоцелью, мы здесь работать собрались, а не играть клуб анонимных алкоголиков.

Сейчас в среде разработчиков принято называть «суперзвёздами» или «рок-звёздами» тех, кто кроме своего эго ничего не видят, не умеют делегировать, и, в результате всего этого — замедляют работу над продуктом, а не ускоряют. И, в итоге, сгорают сами.

Я считаю, это неправильно.

Те, кто выгорел — это не суперзвёзды. Настоящие суперзвёзды, как Линус Торвальдс, год за годом локомотивом пробивают пробивают дорогу не только себе, но и другим.
В данной ситуации конечно же коллегу, отказавшуюся вносить правки профессионалом назвать нельзя. Но и к ревьюеру это тоже относится. Можно было снизить планку по требованиям, поговорить с коллегой и убедить внести правки, на худой конец заапрувить а затем отрефакторить. Но нет, вместо этого три недели потраченного впустую времени и незакрытая задача.
>можно было снизить планку по требованиям,

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

> и убедить внести правки

Вот здесь — да. Но я сам неоднократно сталкивался с тем, что менее опытные коллеги упираются рогом просто потому, что не понимают сути моих претензий.
Я в таких случаях рогом в ответ не упираюсь. Не нравится моя оценка — либо докажи мне, что я неправ, либо мёрджи без моего подтверждения, а потом сам объясняйся с руководством, почему ты так сделал. Я больше на это своё время тратить не буду. Sapienti sat.
Я в таких случаях рогом в ответ не упираюсь
Уммм.
Не нравится моя оценка — либо докажи мне, что я неправ, либо мёрджи без моего подтверждения, а потом сам объясняйся с руководством, почему ты так сделал. Я больше на это своё время тратить не буду. Sapienti sat.
Вообще-то такое поведение и называется «упереться рогом» и является лучшим способом добиться того, что работа будет выполняться «на от$бись».
Можно было снизить планку по требованиям

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

Пока это выглядит так: новенькой девочке было то ли лень, то ли просто обидки, а потому — она устроила саботаж на работе, убила 15-30 человеко-дней, испортила атмосферу в команде и… получила положительное подкрепление — работу по объемному фидбеку она так и не сделала.

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

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

Короче решение из статьи — как писать в штаны на морозе. Сейчас тепло, конфликт вроде замяли. В средне-срочной перспективе — одни минусы.

Должен заметить, что такое решение прекрасно подходит для низкосортных галер, где нанимают всех подряд, 3-6 месяцев из них выжимают все сроки с графиком 80 часов в неделю, а потом проект — толкают, команду — выбрасывают, поддержку отдают на аутсорс в Индию. Краткосрочный выигрыш от решения конфликта — крайне важен, а среднесрочные риски могут не успеть сработать.
Так а кто решает, дельные они или нет?

Мне вообще непонятен кодревью в стиле «вот это и это неверно, переделай, или не получишь некий аппрув». Это как вообще? Код-ревьювер — твой партнер, а не учитель и не ментор. Он не имеет права требовать каких-то изменений. Он твой… дублер, твоя страховка на случай, если ты что-то упустил. Да, он может дать рекомендации по поводу того, что можно было бы сделать лучше (типа именований, что субъективно), но требовать это выполнять — … новости с другой планеты какие-то.
Код-ревьювер — твой партнер, а не учитель и не ментор. Он не имеет права требовать каких-то изменений.

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


Типовой стиль Gerrit, например, настроен на это — для допуска ревью к коммиту кто-то должен поставить +2 на "Review". В принципе, можно поставить самому себе :), но будет заметно.


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


новости с другой планеты какие-то.

Ну я на этой планете несколько лет работал. И уверен, даже среди читателей этой статьи таких найдётся немало. И не скажу, что это было как-то ужасно — наоборот, (повторюсь) польза была видна всем.

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

Не обязаны. Отметить несоответствие и выдать рекомендацию — да. Требовать — нет.
Вынужден повториться: я работал именно с такой организацией.
Видишь просто несоответствие стиля, недочёты, не влияющие на работу, но которые могут вылезти потом => ставь -1.
Видишь, что сломается в принципе => ставь -2 (срабатывает запрет, пока отметка не будет снята).
Если не поставил, пропустил коммит => принимаешь долю ответственности.

Если именно у вас так не делают, это не значит, что не делают нигде.

И я считаю, что так в очень многих случаях правильно — когда работающие над некоторым участком кода обязаны участвовать в ревью любого изменения этого участка. Иначе слишком легко оторваться от знания, что там происходит.
Да, не в 100% случаев. Точную цифру бы не дал. Но полагаю, что на большинство случаев.
Потом, увы, времени на исправление не будет — будут в работе другие задачи.
Серьёзно? То есть вы вот так — берёте и сходу пишите идеальный код?

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

А раз код всё равно нужно улучшать и менять — то так ли важно когда это делать? Я не говорю про патологические случаи типа 50 копий одной функции для разных типов данных вместо одой шаблонной. Я говорю про вещи, которые можно сделать так — или иначе. А по закону тривиальности именно такие вещи будут вызывать больше всего споров на code review.

Назвать переменный destination и source или dest и src? Если Вася делает так, а Петя — иначе, то код действительно выглядит слегка неряшливо, но… настолько ли это серьёзная проблема, чтобы тратить на наё часы и дни?

Если хочется какого-то одного стандарты — ну выдилите вы пару дней, чтобы подобные механические вещи сделать. Если все переменные паре сотен функций называется dest и src, то даже Вася, скорее всего, не будет спорить.

Короче решение из статьи — как писать в штаны на морозе. Сейчас тепло, конфликт вроде замяли. В средне-срочной перспективе — одни минусы.
Ну дык. Хочу только напомнить, что фраза про «писанье в штаны на морозе» — была сказана Ансси Ваньоки в отношении Андроида. Типа: в краткосрочной перспективе — это даст производителям телефонов краткосрочное облегчение, но в долгосрочной перспективе — у них будут проблемы. И он, в общем-то, был прав: у производителей, выбравших Андроид (который был как раз разработан с подходом, описанным в статье — «стремитесь повысить уровень качества кода только на одну-две ступени») куча проблем… а вот гордая Нокия, выбравшая другой подход — телефоны перестала производить вообще.

Цитирую пост:


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

Как видно, все тривиальные вещи были без вопросов исправлены, а откладывались именно признанные серьезными.

Самые ужасные проекты (в первую очередь с точки зрения бизнеса), за которыми мне удавалось наблюдать, почему-то велись адептами «идеального» кода, серебряных пуль, патернов и принципов. Бывает, что адепты создают и качественный продукт, но вот ни разу не наблюдал, чтобы люди умеющие договариваться создавали такой адЪ. В этих проектах все было замечательно: пяток языков программирования, столько же разношерстных баз, адовое нарушение этих самых принципов (с горой оправданий, что мы все по уму, но нам бизнес все испортил своими желаниями), а главное заправлено это все зависимостью от единого разработчика. Как следствие безумные расходы на поддержку монстра. Но формально все хорошо, стандартов придерживаемся, код ревьюим (ну и что, что разработчики разбегаются).

Смею предположить, что на заводском конвейере «хипстерская херня» Вас не достанет. А в IT, все же напомню, единственный ресурс это люди. Кодовая база моментально устаревает и теряет актуальность и без человеческого ресурса просто умирает.
Самые ужасные проекты (в первую очередь с точки зрения бизнеса), за которыми мне удавалось наблюдать, почему-то велись адептами «идеального» кода, серебряных пуль, патернов и принципов


Вот вы взяли и навесили мне ярлык. А я разве где-то говорил про идеальный код? Я говорю о том, что во главе угла должна стоять задача, а не умащивание нежных чувств отдельных членов команды. Если человек пишет говнокод — значит он должен его исправить. Если я вижу, как его исправить с уровня F (в системе координат автора статьи) до уровня A, то почему мы должны остановиться на D? Нет, если переход на A требует неадекватных затрат времени, то мы можем остановиться и на B. Но почему D? Просто потому что автор кода — эмоциональная девушка, не желающая уступать?

но вот ни разу не наблюдал, чтобы люди умеющие договариваться создавли такой адЪ


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

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

Не важно же кто прав или не прав, проблема в том, что два человека в команде не смогли договориться и тратят свои сили не туда

Или я как-то непонятно изъясняюсь, или вы странно меня читаете.
Я как раз предлагаю тратить ресурсы не вникуда, а на полезную деятельность.
И зачем вы продолжаете приписывать мне то, чего я не говорил. Где я предлагаю «силовое решение»?
Вот у нас с вами происходит то же самое, что у персонажей заметки.
Так что давайте я вам на живом примере и поясню, что я имею ввиду, поставив в этом споре точку. Вот она:.
И зачем вы продолжаете приписывать мне то, чего я не говорил. Где я предлагаю «силовое решение»?
Вот тут:
Я в таких случаях рогом в ответ не упираюсь. Не нравится моя оценка — либо докажи мне, что я неправ, либо мёрджи без моего подтверждения, а потом сам объясняйся с руководством, почему ты так сделал. Я больше на это своё время тратить не буду. Sapienti sat.
В армии это называется «дедовщина». Ну и что, что зубной щёткой драить сортир долго и мезко? Я «дед», ты — «салага», иди, драй.

Так ваше поведение выглядит со стороны новичка.
В армии это называется «дедовщина». Ну и что, что зубной щёткой драить сортир долго и мезко? Я «дед», ты — «салага», иди, драй.

Нет, на дедовщину это совершенно непохоже. И мы не в армии.
Похоже до степени неразличимости.

Драить сортир зубной щеткой — это бессмысленное действие, придуманное специально для издевательства, которое "дед" сам уже давно не делает.


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

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

Тем не менее, ключевая разница между дедовщиной и обучением в команде — именно в этом.


"Дед" заставляет делать то, чего он уже не делает сам. Наставник заставляет делать то что он делает сам.

«Дед» заставляет делать то, чего он уже не делает сам. Наставник заставляет делать то что он делает сам.
Хорошо если так. Мой самый ужасный конфликт по поводу код-ревью случился тогда, когда я получил с десяток самых разнообразных замечаний. Когда я на каждое из них нашёл CL, залитый «наставником» в последний месяц и их нарушающий — то история начала развиваться почти как в статье. С нажимом «делай так, как я говорю», и «я так сделал, потому что нужно было сделать срочно, но ты пока делаешь не критичную по времени часть и дожен делать правильно» и прочее.

Дедовщина в чистом виде. Как Macin говорит: до степени неразличимости.

В вашем случае — возможно, но мы же обсуждаем как надо делать код-ревью?


Так вот, есть случае когда лично я бы на код-ревью "уперся рогом" и не стал бы допускать такой код. И у самого меня подобных коммитов, которые я бы сам не пропустил — нет.

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

А вот если есть общее решение для всего проекта (особенно если оно зафиксировано официально) — то это повод выставить оценку F и не пускать такой код дальше, да, разумеется.
Если для вас требовать от нового члена команды соответствовать стандартам, в этой команде принятой — это дедовщина, то тут и обсуждать нечего, мы в разных плоскостях работаем.
Рекомендую читать внимательно и вдумчиво, отвечать на слова собеседника, а не голосам в своей голове.
Очень интересный у вас метод ведения дискуссии.

А вот тут +100. Новичку часто очень сложно поверить в объективность некоторых правил, происхождение которых он не понимает. Особенно когда на это намешиваются вопросы стиля с местными тараканами, которые всегда есть и которые в основном потому, что так "исторически сложилось". Отделить после этого реально-объективные требования от традиций часто не может даже гуру-старожил, а новички или ломают себя психологически, просто привыкая к странному, или не врабатываются вообще.

Вы представьте на секунду, что вам аппендицит удаляет стажёр. Под надзором опытного хирурга.
Всё еще считаете, что «неопытному специалисту» можно что-то прощать?
Я могу согласиться с тем, что в ревью, которое более опытный разработчик делает на код менее опытного, он должен стараться нести конструктив, поясняя суть своих замечаний, предлагая варианты решений и т.п.
Но снижать планку требований? Да с каких это вдруг таких заслуг? Не можешь тянуть лямку такого проекта — работай над проектами попроще.
Это я говорю, как человек, поработавший более, чем с десятком «младших» коллег, но и неоднократно потрудившийся «ведомым».
На свой собственный код, кстати, я давно привык делать ревью ещё до пуша, в gitk-е. Последний у меня запускается автоматически скриптом при ежедневном начале работы над проектом.

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

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

Потому что опыт — он в любой профессии опыт. Он не является неким абсолютным мерилом всего, но всё равно крайне важен при оценке авторитетности мнения.
Я не с той стороны баррикад, о которой вы подумали. Я на месте этой женщины. Но в отличие от нее я прекрасно понимаю, что более опытный человек не просто так советует мне делать так, а не иначе. И уж тем более он это делает не для того, чтобы потешить свое ЧСВ, а потому, что и ему самому за результат работы отвечать. Да, я конечно могу выразить свою точку зрения, объяснить что я сделал так потому-то и потому-то, но настаивать и вот так вот упираться я не вижу смысла. И вот такой вот расклад я считаю правильным порядком вещей, а не пытаться три недели восстанавливать свое оскорбленное чувство прекрасного.
Я говорю о том же самом — надо думать и работать, а не разводить демагогию в комментах к пулл-реквестам.
Но в отличие от нее я прекрасно понимаю, что более опытный человек не просто так советует мне делать так, а не иначе.
Значит вы ещё просто не доросли до понимания того, что «более опытный человек» — тоже не Бог. Не далее как неделю назад я, к примеру, выпилил тот код, который меня пытался заставить создать «более опытный коллега», что я отказался делать категорически.

В конце-концов точно также, как в статье — привлекли третьего, в качестве «третейского судьи», было принято решение, что «более опытный товарищ» всё сделает сам, чем он, разумеется был страшно недоволен… но я прекрасно понимал, что делал: об этот код потом буквально вся команда «спотыкались» два года и, когда «опытный товарищ» свалил радостно изжила. А что было бы, если бы сдался? А очень просто: все шишки падали бы не на «опытного товарища», а на меня. Он же, как вы выразились, «не просто так советует» — а что в результате грабли получились… ну так это я накосячил, без опыта-то…
Обычно в таких случаях необходима аргументация с каждой стороны и решение команды (иногда, в конфликтных ситуациях демократия — лучший выбор, ведь потом ВСЕЙ команде поддерживать этот).

А детский сад из разряда «ты старая пердящая звезда, а я тут яркая молодая звездочка, потому нибуду делать, как ты сказал, сам так делай» — это крайне неконструктивно.

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

Больше всего меня убивает аргументация в стиле «а что если мы наш паром, перевозящий почту через Ла-Манш превратим в ракету по доставке писем на Марс?» — вот когда встанет задача по доставке писем на Марс, тогда и будем думать, а пока у нас в багтрекере такой задачи нет и приоритета у неё тоже нет… забудьте. Всего учесть невозможно и лучше простой код хорошо решающий задачи, которые у нас уже есть, чем сложный, который плохо решает и задачи, которые у нас уже есть и что-то, что есть только в больном воображении ревьюера…
Проблема в случае девушки из статьи как раз не в том что она сделала слишком просто, когда автор хотел сложно, гибко и т.п. Позволю себе процитировать: «неопределённое поведение кода для искажённых входных данных или то, что структуры потока управления в одной из её функций находились на шестом уровне вложенности» — это никак не желание автором чрезмерной гибкости и усложнения архитектуры. Скорее наоборот, автор просто хотел четкости, ясности и простоты в коде.
Ну, «обработка искажённых данных» — это вообще «высший пилотаж». В долгосрочной перспективе от этого скорее вред, но если у вас данные уже есть — то без того, чтобы их обрабатывать вам не обойтись.

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

Это смотря что за блок, с какими данными он работает.

Проблема авторитетов решается очень просто. Начальник тыкает пальцем в небо и выбирает один из вариантов. Более демократичные команды выбирают вариант общим голосованием.


Дальше уже в ревью ссылаются не на авторитетов — а на однажды принятое на проекте решение.

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

В первом случае начальник всегда может тыкнуть пальцем в ту же сторону повторно. Во втором случае — да, нужно обязательно принятые решения где-то документировать.

И, тем не менее, часто это не делается. Высшую степерь опофигения, которую я наблюдал была «у нас так принято, но в style guide мы это не вписываем, потому что он и так большой».

Пришлось то ли на один уровнь, то ли на два подняться, чтобы принцип «если какое-либо решение недостаточно важно для того, чтобы быть внесённым в style guide, то оно недостаточно важно для того, чтобы отказывать принятии CL'я» было официально зафиксировано.

Кстати и style guide был изменён. Раньше говорилось, что можно писать int* x; или int *x;, но не int*x; или int * x; как в Goggle Style Guide, теперь говорится, что в каждом файле должен использоваться только один вариант из двух. Это — меня устраивает, мне, в общем-то, пофигу куда пробел ставить, главное, чтобы в code review на это время не тратить…

Если бы автор и правда пытался получить результат — он бы не переписывался 3 недели с непонятным результатом.

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

такие как неопределённое поведение кода для искажённых входных данных

Правильно ли понял, что в конце, неопределенное поведение так и не было исправлено?

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

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

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

Очевидно, да. Вопрос только в том, зачем конкретно мне думать о том, чего хочет работодатель, и ещё и других этому учить? Если я писатель, должен ли я писать книгу так, чтоб она нравилась издательству? Я так не думаю. Имеет смысл написать так первую книгу. Получить свой контракт. А вот потом писать так, как хочу я, ведь какой смысл быть писателем, если ты пишешь строго так, как хочет издатель?
Если я писатель, должен ли я писать книгу так, чтоб она нравилась издательству?
Да, разумеется. Вы Пушкина знаете? Почитайте «разговор книготорговца с поэтом». Там всё очень поэтичненько описано.

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

Как раз первую книгу — вы можете писать как угодно. А уже последующие — будьте добры придерживаться заранее оговоренных форматов и сроков. Или готовьтесь к серьёзному разговору с издательством…
чудный стих, как-то прошел мимо меня «Не продается вдохновенье, Но можно рукопись продать» :)
Сам код никому не нужен, нужно решение задач бизнеса. Если разработчик этого не хочет признавать, то наверное ему место только в опенсорс/собственных проектах. Хороший код нужен бизнесу только тогда, когда его написание и поддержка обходится дешевле говнокода, а это наступает далеко не сразу.
О, господа тролли подтянулись. А слабо, завтра подойти к своему гендиру и сказать это в лицо?)
О, господа тролли подтянулись.

О, вот и роботы подтянулись, упомянутые в первой статье.

У вас есть подходящий ярлычок для каждого, кто посмел думать иначе?
Да думайте, на здоровье иначе, я за свободу самовыражения, но только не надо лицемерить. Мне вот не будет стыдно, если мое начальство ознакомиться с моими комментариями.
Вам никто не мешает работать над open source проектами для души с таким отношением. Работодатель же, платя вам зарплату, покупает ваши услуги. Разумеется, эти услуги должны удовлетворять работодателя, ваш код, написанный в рамках этого договора, должен отвечать целям бизнеса, собственно, за что вам и платят. Не думаю, что где-то платят просто за абстрактный красивый и правильный код. Платят за то, что работает с минимальными затратами средств и времени и соответствует целям работодателя.
PS: я сталкивался вот с таким отношением а-ля «Я художник, я так вижу» от одного программиста в одном проекте. Итогом стал срыв сроков и прочие не менее неприятные вещи.
в продолжение к статье возник вопрос:
— дядя Боб смержил код к которому есть достаточно притензий, мы все такие на белом коне, ворвались и выровняли ситуацию с назревающим конфликтом. Но как выровнять ситуацию с кодом? В рамках каких задач заставить этого человека отрефакторить только что смерженый код?
— писать //TODO в коде? Нет ничего более постоянного чем временное — тудушки будут только множиться в коде и контекст задачи уже потеряется к моменту рефакторинга
— оставить как есть? Тогда зачем код ревью вобще проводить

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

Ведь речь не идёт о том, что код настолько ужасен, что непонятно — будет он работать или нет. А просто с ним что-то «не совсем так»: константам не даны имена, копи-паста много или что-нибудь подобное.

Ну и? Убиваться-то поэтому поводу зачем? Вотгда придёт пора менять что-то в этом коде — тогда и отрефакторите.

Повезло и этот код никому трогать не нужно? Ну и отлично — пусть остаётся грязным. Раз его никто не трогает, то он никому и не мешает.

«у меня очень много коментариев, давай устроим парное программирование и улучшим этот код вместе»
Это обычно имеет смысл тогда, когда код совсем никуда не годится.
Вотгда придёт пора менять что-то в этом коде — тогда и отрефакторите.

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

Более того, потом, когда он с ним работать в следующий раз будет — получим отсылку к пункту статьи «Уважайте область ревью» — не моя область, я это рефакторить не собираюсь.

Плохое отношение к качеству сейчас — еще хуже отношение к качеству потом.

константам не даны имена, копи-паста много или что-нибудь подобное.

Ага, вот так сейчас: «а, хрен с ним, пока не печет». А через пол-года:
— хнык-хнык, было 50 багов, закрыл 20, теперь 100 багов.
— хнык-хнык, да, я фиксил этот баг, но оказывается, что мы его скопипастили 18 раз и каждый из участков немного менялся, потому надо пофиксить его 18 раз, а потом отдел тестирования должен будет внимательно проверить все 18 частей системы заново.
— хнык-хнык, чтобы этот модуль заработал, надо переписать его с нуля
— хнык-хнык, мы не сможем сделать это в срок, потому что программист, которому поручили работать с этим кодом сперва неделю плакал, а потом вышел в окно офиса на 7 этаже
— давай, досвидания, пущай ваши джуны разгребают этот говнокодище, который оставил ни в коем случае не я, с наплевательским отношением к качеству, а папєрєдники.

И всё это вы называете «я думаю про бизнес!»
Более того, потом, когда он с ним работать в следующий раз будет — получим отсылку к пункту статьи «Уважайте область ревью» — не моя область, я это рефакторить не собираюсь.
Читаем внимательно: «если изменение относится к изменяемому коду, хотя и не входит в него» — то оно «в области ревью».

— хнык-хнык, было 50 багов, закрыл 20, теперь 100 багов.
Это нормально. Если у вас ситуация не такова — то это просто обозначает, что ваш проект никому, по большому счёту, не нужен и новые баги заводить просто некому.

— хнык-хнык, да, я фиксил этот баг, но оказывается, что мы его скопипастили 18 раз и каждый из участков немного менялся, потому надо пофиксить его 18 раз, а потом отдел тестирования должен будет внимательно проверить все 18 частей системы заново.
А почему у вас 18 копи-пастов кода? Почему вы не провели рефакторинг после появления 4й или 5й копии? Если все 18 родились одновременно, то оценка у такого изменения «F» и заливать его не нужно было — об этом в статье тоже есть.

— хнык-хнык, чтобы этот модуль заработал, надо переписать его с нуля
Даже если вы вбухаете в разработку в 10 раз больше времени, чем нужно — вы от этого не застрахованы. Потому что, например, «этот модуль» мог реализовывать какую-то функциональность, доки на которую у вас были просто неправильными…

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

Я не так давно заменил кучу макросов на вариативный шаблоны в одном из наших модулей. Он стал чище и проще для понимания — но три года назад (когда эти макросы создавались) так его было написать нельзя, так как поддержки C++11 на одной из платформ, где мы собирались не было — и это приходилось учитывать.
А почему у вас 18 копи-пастов кода? Почему вы не провели рефакторинг после появления 4й или 5й копии? Если все 18 родились одновременно, то оценка у такого изменения «F» и заливать его не нужно было — об этом в статье тоже есть.

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


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

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

Но это же не повод писать код который уже сейчас является заведомо ужасным.
Цитирую статью:
Оценка F предназначена для кода, который либо функционально некорректен, либо настолько запутан, что вы не можете быть уверены в его корректности. Единственная причина задержать одобрение — это если код остаётся на уровне F после нескольких раундов ревью.
Если код — таки «ужасен» и либо функционально некорректен, либо настолько запутан, что вы даже не можете понять — корректен ли он, то если заливать не нужно.

Вот дальше — возможны варианты.

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


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

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

На это есть известный довод, что на 1 написание кода приходится N (обычно говорят про 10) чтений кода, и ждать рефакторинга, чтобы его исправить — означает попусту тратить будущее время коллег.
А ещё есть "теория разбитых окон", которая говорит, что если появляется несколько мест такого безобразия, то скоро весь проект станет таким.


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

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

А ещё есть «теория разбитых окон», которая говорит, что если появляется несколько мест такого безобразия, то скоро весь проект станет таким.
Теория разбитых окон — это немного о другом. Если вы что-то допустите один раз (ну, к примеру, доступ к защищённому полю через reflection), то допустите и второй, а с третьего раза — это станет «нормой жизни». Это правда. Но вряд ли все 59 замечаний в «худшем код-ревью» автора были таковы. Да и довод «в нашем проекте мы X не делаем никогда» действует даже на новичков (особенно если это явно упомянуто в style guide). А вот моменты, про которое сходу неочевидны — тут ситуация другая. И в статье — речь именно об этом.

Процитирую еще раз:


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

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

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

Я так понимаю тут речь шла не о входных данных в программу (тут вопросов нет — мало ли что пользователь пришлёт?), а о входных данных в модуль. Вот тут — уже вполне может оказаться что принцип GIGO приведёт в итоге к более надёжному коду — просто потому, что он будет проще.
В нашей фирме нет код ревью, как результат коллеги-разработчики тратят много времени на то, чтобы вникнуть в некоторые изменения кода. На понимание того, как работает измененный код порой уходит день, иногда больше.
Как-то раз от меня проскакивало предложение ввести код ревью, но оно не было воспринято всерьез и прозвучали слова о том, что это «замедляет разработку» и «у каждого свое видение».
Как можно убедить вышестоящие должности ввести код ревью и правильно аргументировать его важность? Возможно ли постепенное введение код ревью, чтобы не стопорить сразу работу на n-ное количество времени и ревьюить все, а как-то подготовить людей к такому?
Мой опыт говорит, что если в фирме недостаточный уровень качества разработки, но бизнес как-то работает и зарабатывает, почти невозможно убедить сделать какие-то улучшения. И это как бы логично, зачем тратить время, если и так работает.
Когда ведется интенсивная разработка и/или приближается дедлайн, код ревью действительно может восприниматься как пустая трата времени.
Но в какой-то момент можно столкнуться с изъянами проектирования и разработки. И, как известно, чем дольше существуют подобные изъяны, тем больше на них завязано функционала и тем сложнее потом все переделывать (частенько даже просто реализовывать новый функционал).
Бизнесу, конечно, в краткосрочной перспективе важнее выпустить работоспособный продукт. Но и о долгосрочной перспективе он думать должен.
Если в вашей фирме нет код-ревью и низкое качество кода, значит это устраивает и босса, и большинство программистов. Так же это вероятно значит, что изначально именно программисты начали разработку в таком стиле, боссу все понравилось, и зачем вдруг сейчас что-то менять, ему не очень ясно.
Раз вы работаете в этой фирме, значит вас в целом тоже все устраивает.
Если вы выросли и вас не устраивает, вы можете или попробовать стать архитектором этой системы (но нет гарантии что вам дадут применить ваше новое положение на практике), или найти новое место работы.
Возможно ли постепенное введение код ревью, чтобы не стопорить сразу работу на n-ное количество времени и ревьюить все, а как-то подготовить людей к такому?
Начните с автоматизации. clang-format, автоматический запуск тестов и «закрытие дерева», если они падают. Вот это вот всё. А к «человеческим» код-ревью подойдёте потом.

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

А когда все почувствуют что он этого они получают ощутимую прибыль (реже приходится авралы устраивать, меньше приходится работать на выходных в авральном режиме) — уже можно и о код-ревью завсети разговор. В первой части об этом упоминалось.
Как убедить: рассказать о плюсах лучшего кода. Ведь код-ревью вводится чтобы код стал лучше. Главное не зациклиться на самом коде, а вместо этого рассказать, что это даст бизнесу. Что увеличится скорость разработки (а не уменьшится), что в хороший код легче вносить изменения, что в такую фирму захотят прийти работать другие хорошие программисты, а не только говнокодеры.

Если такие аргументы не работают, значит нечего там делать.
Спасибо! Очень полезная и ёмкая статья!
У меня сложилась такая схема:
0. Реформаторы кода и линтеры запускаются до подачи кода на code review
1. Продуктовые вопросы (как это должно работать в таком-то случае? как это должно работать совместно с такой-то функцией)
2. Высокоуровневый дизайн (структуры данных, библиотеки/модули/пакеты/классы/публичные методы)
3. Качество кодирования (приватные методы, код внутри методов, тесты)

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

В примере из статьи не было нулевого пункта. Сразу были комментарии по пунктам 0, 1?, 2, 3. Это дало возможность разработчику исправить только 0 и 3, а по принципиальным для проектирования моментам ничего не сделать. Если бы были замечания только по пункту 2, то их бы не получилось так просто игнорировать.

Основная же идея о том, что можно пропускать плохой код ради обучения специалистов не понравилась. Если юниор (вообще или в проекте), то можно перед написанием кода делать верхнеуровневое проектирование (исключит пункты 1 и 2). А вот к коду внутри методов можно действительно временно снижать планку, если приходится работать с юниорами в программировании (регуляр уже должен корректно писать код на уровне методов).
Фух)) Я думал это во мне дело) А оказалось в скоупах и есть методические указания не проверять в код ревью out of scope) А то работал в одной компании и когда ревью превращалось в простыню непонятных замечаний поправить код, написанный другим (!) человеком)))) И пинался и пинался появлялись всё больше строк кода из другого скоупа и я начал думать было что я лох. Думаю, теперь этот человек поймёт, что был не прав.
Зарегистрируйтесь на Хабре , чтобы оставить комментарий

Публикации

Истории