Comments 147
Вывод: не теряйте время, ищите людей таких же целеустремленых и желающих учиться, как вы.
Либо я не понял сарказма, либо я не понял вообще. :)
Почему с ужасом? Они будут тратить на обучение слишком много времени вместо того, чтобы работать?
У 1сников распространен термин «жопочасы» для этого :)
Ну, у программеров учёба — неотъемлемая часть профессиональной деятельности. Хоть ты джуниор, хоть синьор… Надоело учиться — конец профессионализму.
Или я опять неверно понял?
При этом автор гораздо более компетентен технически чем сотрудница и, я так полагаю, чем менеджер.
Нет. Автор думает, что более компетентен, но несмотря на это, не может доказать свою точку зрения новичку. Либо он не настолько компетентен, либо не умеет общаться (именно на это упор в статье), либо изменения того не стоят и спорны.
Вывод: не теряйте время, ищите людей таких же целеустремленых и желающих учиться, как вы.
абстрактные Мэллори/Вася/Петя/Айрат могут быть отличными профессионалами своего дела и некомпетентными чуть вне его. Задача ревью в первую очередь — не допустить говнокод в продакшн, во вторую — научить сотрудника. Если научить не получается (иногда это на самом деле и не нужно), то зачастую лучше и проще сделать как надо самому.
Ну такое. Сейчас работаем вдвоем с человеком, желающим учиться и вообще таким же как я целеустремлённым. Код ревью иногда в ад превращается. Оба рогом упираемся и пишем друг другу поэмы в комментариях. Так что команда, состоящая из золотых специалистов — не всегда гарантия безстрессовой работы. Где есть два человека, там конфликт неизбежен.
Но в последнее время заметил, что код ревью всегда проходит проще, когда ревьювер (в данном случае я сам) искренне задаёт себе вопрос "как я могу помочь этому человеку?". Обычно сразу после этого вопроса приходит соломоново решение. Главное вспомнить о нём вовремя :)
Где есть два человека, там конфликт неизбежен.
У нас сейчас команда побольше и ревью просто прекрасные. Более того, мы притерлись к стилю друг друга, выработали хорошие практики и, в итоге, ревью значительно сократились. При этом с самого начала у нас не было ни одного конфликта на этой почве.
Ах как вам повезло что я у вас не работаю!
Но, если серьёзно, то я бы лучше не хвастался хорошими отношениями с людьми, а то можно беду накликать.
Просто я хотел сказать, что где два человека — конфликт вероятен, но не неизбежен)
Как сказал классик, "На большом отрезке времени шансы каждого из нас на выживание близки нулю." Эту же фразу можно переиначить и об отношениях с людьми.
Ну и тут дело не в суеверности, а в том, что хорошие отношения — это очень большая работа. Причем, работа всех участников предприятия. То что у вас в команде гладкие отношения с коллегами, говорит о том, что все участники упорно трудятся над этими самыми отношениями. А когда хвастаешься тем что хорошо работаешь, то обычно сразу после этого позволяешь себе слегка расслабиться. Сорри, что я вообще начал этот разговор.
Вывод: Только воспитывая можно добиться таких результатов
Любой код ревью портит отношения между коллегами по разработке
Проще взаимоотношения между разработчиком и тестером — они антагонисты по природе — это воспринимается, как данное, и не создает психологических трений.
Также любой код, как известно (мне, по крайней мере), может получить приставку г.
У нас в команде не портит. Замечания все воспринимают как должное
>Также любой код, как известно (мне, по крайней мере), может получить приставку г.
Одна из прелестей ревью — через некоторое время уровень и ожидания от кода в команде синхронизируются и люди уже не отдают на ревью г-код
Сделал такой маленький эксперимент — высказываю свое мнение (подчеркивая это) и получаю кучу минусов — это и есть естественная реакция на любой код ревью
А у вас зыблемо?
Любой код ревью портит отношения между коллегами по разработке
Знаете, а у нас код-ревью даже что-то сродни тим-билдинга и отношения наоборот укрепляются. Так что вы ошибаетесь.
Проще взаимоотношения между разработчиком и тестером — они антагонисты по природе
Тут вы, кстати, тоже ошибаетесь. Они как танк и хилер в играх. Когда я работал в Варгейминге мы с тестерами были одной командой и реально вместе работали над тем, чтобы сделать результат наиболее качественным. То есть я не воспринимал их как: «вот подлецы, баг нашли!», а как «класс, они нашли баг и игроки его уже не увидят».
А из-за вашего категоричного заявления и невежества, которое вы проецируете на всех — вас и минусуют.
Джуниор, делающий замечания по коду сеньора — хороший джуниор. Сеньор, прислушивающийся к ним (и научивший джуниора так делать) — хороший сеньор. И наоборот это тоже верно.
Я специально дождался, пока она уйдёт с работы, потому что не хотел находиться с ней в одной комнате, когда она получит письмо.
То есть, они сидели в одной комнате, но вместо того чтобы выяснить отношения по человечески, 3 недели перекидывались письмами? Я бы уволил обоих за имитацию деятельности вместо работы.
Но, увы: уж сколько я за свою профессиональную деятельность в software development наблюдал таких… «работничков»… По средневзвешенной оценке, порядка 50% в «молодых стартапах», и до 80-85% в «старых, солидных компаниях» (там, где врастают корнями в свое офисное кресло, и десятилетиями колупают с умным видом протухший говнокод). Как правило, такие «работнички» вместо работы очень любят постить во всевозможные блоги, а также обожают всяческие митинги — «типа профессиональная этика» (а, может, просто не любят/умеют/хотят) не дает им сидеть в рабочее время в социальных сетях/играть в игры/ваять homebrew «нетленку»/«халтурить», вот потому они и так рады бессмысленным митингам и «конфликтам», высосанным из пальца.
100% no hire! Но, увы, в идеальной вселенной… В реальной я сам лично наблюдал, как человек просто-напросто «подставил» компанию, принеся реальный убыток, мотивируя лишь тем… что у сына школьная игра в бейсбол на уровне районных школ (т.е. даже не county, и не штата), которую он не может пропустить! И, самое странное, что его не уволили тут же — не знаю даже, почему (возможно, испугались судебных издержек — сложно сказать).
P.S. Понятное дело, что сделали и без него (вообще, по хорошему, его должность была практически чистая синекура), но сам факт…
Она никогда раньше не писала на Python и никогда не разрабатывала систему поверх неуклюжей легаси-системы, которую мне приходилось поддерживать.
Девушка написала свое первый код на Питоне, и не хочет выполнять замечания более опытного коллеги который, судя по всему, повел себя как упрямый баран и вместо помощи выкатил ей список на 100 листов замечаний ожидая от неё кода синьора да еще и в легаси, которое он сам долго причесывал?! Кто из них двух разумнее? Если бы ваш первый код зеревьювил бы по самое «не балуйся» гуру из Гугла? Вряд ли бы вы сейчас программировали, т.к. редко кто готов чувствовать себя полным ничтожеством.
и вместо помощи выкатил ей список на 100 листов замечаний
А замечания это разве не помощь? (Разумеется, я предполагаю замечания вида "тут желательно сделать отдельную функцию" или "не соответствие стилю XYZ" при наличии руководства по стилю, а не "код говно, переделывай").
Есть совершенно конкретный список, что не устраивает, и чего хотелось бы видеть вместо. Что ещё может быть "помощью" на ревью?
Если бы ваш первый код зеревьювил бы по самое «не балуйся» гуру из Гугла? Вряд ли бы вы сейчас программировали, т.к. редко кто готов чувствовать себя полным ничтожеством.
Ревью от гуру из Гугла имеет смысл для кода для гугловского продукта, но тогда это не может быть чей-то первый код (не считая тривиальнейших правок в две строчки).
И точно так же гуру из Гугла может быть ревьюером для кода своего ребёнка, смягчая требования до подъёмных соответственно возрасту и опыту, но этот код не будет предназначен для гугловского продукта.
Девушка написала свое первый код на Питоне
Я не девушка, но попадал в аналогичные ситуации. Или код забирался так, что его правил кто-то другой и уже коммитил под собой, или требовал выполнения таки описанных правил по стилю и функциональности. Если не впадать в психологические позы обструктивизма, проблем не было.
Необходимое уточнение: всё это уже за пределами основной идеи статьи. Вы пошли на безусловное оправдание Мэллори, я возражаю против этой позиции, но полезное в статье не об этом, а о том, как уже имея участников с крайними позициями — умудриться примирить их.
Поэтому данная сублиния обсуждения не суть важна.
Если бы ваш первый код зеревьювил бы по самое «не балуйся» гуру из Гугла?
… то некоторых моментов в том коде я бы сейчас не стыдился :-) А ваши предположения о том что я бы сейчас не программировал не имеют под собой основания.
Девушка написала свое первый код на Питоне, и не хочет выполнять замечания более опытного коллеги
Вполне возможно, что его замечания не питоно-специфичны, так что ее малый опыт на данном ЯП и не имеет значения.
В коде тоже довольно легко формализовать оценку отдельно взятого фрагмента.
Например, если я в ревью придираюсь к тому, соответствует ли код стандартам 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, то оно недостаточно важно для того, чтобы отказывать принятии CL'я» было официально зафиксировано.
Кстати и style guide был изменён. Раньше говорилось, что можно писать
int* x;
или int *x;
, но не int*x;
или int * x;
как в Goggle Style Guide, теперь говорится, что в каждом файле должен использоваться только один вариант из двух. Это — меня устраивает, мне, в общем-то, пофигу куда пробел ставить, главное, чтобы в code review на это время не тратить…Если бы автор и правда пытался получить результат — он бы не переписывался 3 недели с непонятным результатом.
Если бы было только две альтернативы — не принимать неадекватный код, соблюдя чистоту проекта, или принимать, подложив свинью на будущее — да, такая постановка имеет смысл.
Но автор рассказал про случай третьего выхода из вроде бы дилеммы. И это должно быть основным мотивом обсуждения статьи.
такие как неопределённое поведение кода для искажённых входных данных
Правильно ли понял, что в конце, неопределенное поведение так и не было исправлено?
Наверно, я чего-то не понимаю…
Но музыку заказывает тот кто платит, т.е. работодатель — а ему свой бизнес важнее твоего кода.
А хочешь, чтобы твой код был важнее чьего-то бизнеса: ну, можно покодить бесплатно для себя, да и опенсорс-проектов навалом )
Если я писатель, должен ли я писать книгу так, чтоб она нравилась издательству?Да, разумеется. Вы Пушкина знаете? Почитайте «разговор книготорговца с поэтом». Там всё очень поэтичненько описано.
А вот потом писать так, как хочу я, ведь какой смысл быть писателем, если ты пишешь строго так, как хочет издатель?Ну вы же хотите, чтобы вашу книгу прочитали? Значит вы заключите контракт. А если потом захотите не оказаться на улице из-за выплаты неустойки — то вам придётся его выполнять. А если уж совсе плохо будет — издатель может и кому-нибудь другому поручить за вас продолжение написать.
Как раз первую книгу — вы можете писать как угодно. А уже последующие — будьте добры придерживаться заранее оговоренных форматов и сроков. Или готовьтесь к серьёзному разговору с издательством…
О, господа тролли подтянулись.
О, вот и роботы подтянулись, упомянутые в первой статье.
У вас есть подходящий ярлычок для каждого, кто посмел думать иначе?
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). А вот к коду внутри методов можно действительно временно снижать планку, если приходится работать с юниорами в программировании (регуляр уже должен корректно писать код на уровне методов).
Вспомнился Zed A. Shaw и его сайт: http://programming-motherfucker.com/
Code review по-человечески (часть 2)