Это вторая часть статьи о том, как правильно общаться и избежать ошибок в процессе код-ревью. Здесь мы поговорим о том, как довести ревью до конца и избежать неприятных конфликтов.
Основы изложены в первой части, так что рекомендую начать с неё. Но если не терпится, вот её краткое содержание: хороший рецензент не только ищет баги, но и обеспечивает добросовестную обратную связь, чтобы помочь коллеге повысить свой уровень.
Моё худшее код-ревью
Худшее код-ревью в моей жизни было для бывшей коллеги, назовём её Мэллори. Она начала работать в компании за несколько лет до меня, но только недавно перешла в мой отдел.
Ревью
Когда Мэллори прислала для ревью первый список изменений, код был довольно сырой. Она никогда раньше не писала на Python и никогда не разрабатывала систему поверх неуклюжей легаси-системы, которую мне приходилось поддерживать.
Я добросовестно задокументировал все проблемы, которые сумел найти, всего 59 штук. Если верить прочитанной литературе по код-ревью, я проделал отличную работу: нашёл ТАК много ошибок. Мне действительно было чем гордиться.
Через несколько дней Мэллори прислала новый список изменений и ответ на мои заметки. Она исправила самые простые ошибки: опечатки, названия переменных и т.д. Но отказалась решать проблемы высокого уровня, такие как неопределённое поведение кода для искажённых входных данных или то, что структуры потока управления в одной из её функций находились на шестом уровне вложенности. Вместо этого она печально пояснила, что исправление этих проблем не стоит времени разработки.
Рассерженный и раздосадованный, я выслал новые замечания. Мой тон был профессиональным, но переходил в область пассивно-агрессивного: «Можешь объяснить, зачем нам неопределённое поведение для искажённых входных данных?» Как несложно догадаться, Мэллори проявляла всё большее упрямство.
Цикл ожесточения
Был вторник, прошла уже неделя. Мы с Мэллори по-прежнему обменивались письмами насчёт того же код-ревью. Я отправил ей свои последние замечания вечером предыдущего дня. Я специально дождался, пока она уйдёт с работы, потому что не хотел находиться с ней в одной комнате, когда она получит письмо.
Всё утро я чувствовал физическую тяжесть в желудке, потому что боялся следующего раунда ревью. Когда вернулся с обеда, то увидел, что Мэллори нет на рабочем месте, но от неё пришли новые изменения. Я догадался, что она тоже не хочет присутствовать в комнате, когда я их получу.
Сердце всё сильнее стучало в груди по мере увеличения моего возмущения с каждым её ответом. Я немедленно начал барабанить по клавиатуре опровержения, указывая на то, что она не приняла никакие из моих предлагаемых изменений, и в то же время не предоставила никаких оправданий, чтобы можно было одобрить её код.
Этот ритуал повторялся каждый день в течение трёх недель. Код практически не изменился.
Вмешательство
К счастью, наш самый опытный коллега Боб разорвал этот круг. Он вернулся из длительного отпуска и обнаружил, что мы ожесточённо швыряем друг другу заметки по код-ревью. Боб немедленно оценил ситуацию как тупиковую. Он попросил перенести ревью на него, и мы оба согласились.
Боб начал своё ревью с того, что попросил Мэллори составить новые списки изменений, выделив две маленькие библиотеки, о которых мы вообще не спорили, каждая на 30-50 строк. Когда Мэллори сделала это, Боб немедленно одобрил изменения.
Потом он вернулся к основному списку изменений, который сократился примерно до 200 строк кода. Он сделал несколько небольших замечаний, которые Мэллори исправила. Затем одобрил список изменений.
Всё ревью Боба завершилось за два дня.
Коммуникация имеет значение
Вы могли уже догадаться, что конфликт на самом деле возник не из-за кода. Там действительно имелись проблемы, но их явно мог решить коллега, который обладал навыками эффективной коммуникации.
Это был неприятный опыт, но я рад вспоминать о нём. Он заставил меня пересмотреть свой подход к код-ревью и определить направления для улучшения.
Ниже я поделюсь некоторыми техниками, которые помогут вам избежать таких неприятных ситуаций. Позже я вернусь к Мэллори и объясню, почему мой изначальный подход имел обратный эффект и почему подход Боба оказался абсолютно блестящим.
Техники
9. Стремитесь повысить уровень качества кода только на одну-две ступени
10. Ограничьте фидбек по повторяющимся примерам
11. Уважайте область ревью
12. Ищите возможность разбить большие ревью
13. Искренне хвалите
14. Утверждайте ревью, когда остались тривиальные правки
15. Заранее избегайте тупиковых ситуаций
Стремитесь повысить уровень качества кода только на одну-две ступени
Хотя в теории у вашего коллеги может быть желание исправить абсолютно все недочёты в коде, но его терпение не безгранично. Если вы станете тянуть с одобрением раунд за раундом, у него быстро появится раздражение, потому что у вас возникают всё новые и новые блестящие идеи, как улучшить код.
Я про себя ставлю коду оценку, как в школе, от A до F [от шестёрки до кола в американской системе образования — прим. пер]. Если получаю список изменений, который соответствует оценке D (3), то пытаюсь поднять автора до оценки C или B-. Не идеально, но достаточно хорошо.
В теории возможно повысить уровень качества с D до A+, но это может потребовать до восьми раундов ревью. К концу такой работы автор вас возненавидит и никогда больше не захочет присылать вам код.
Вы можете подумать: «Если я одобрю код уровня C, не приведёт ли это к тому, что вся кодовая база будет уровня C?» К счастью, такого не произойдёт. Я пришёл к выводу, что когда помогаю коллеге подняться с уровня D до C, следующий список изменений от него уже начинается с уровня C. Через несколько месяцев он начинает присылать код, который начинается с B, и переходит на уровень A к последним код-ревью.
Оценка F предназначена для кода, который либо функционально некорректен, либо настолько запутан, что вы не можете быть уверены в его корректности. Единственная причина задержать одобрение — это если код остаётся на уровне F после нескольких раундов ревью. См. главу о тупиках ниже.
Ограничьте фидбек по повторяющимся примерам
Если вы заметили, что у автора несколько раз повторяются однотипные ошибки, не нужно помечать каждый случай. Не стоит тратить время на написание одного и того же 25 раз, и автор тоже не захочет читать 25 повторяющихся замечаний.
Нормально будет назвать два или три отдельных случая ошибок этого типа. Для всего остального просто попросите автора исправить все ошибки этого типа, а не перечисляйте каждый конкретный случай.
Уважайте область ревью
Существует пример неправильных действий, когда рецензент обнаруживает что-то рядом с кодом из списка изменений — и просит автора исправить. Когда тот удовлетворяет просьбу, рецензент обычно находит, что код стал лучше, но он теперь непоследователен, так что нужно произвести ещё несколько незначительных изменений. А потом ещё несколько. Это продолжается и продолжается, пока конкретный лаконичный список изменений не разрастётся, вобрав в себя кучу посторонних вещей.
Если у ваших дверей появится голодный мышонок, вы захотите дать ему печенье. И если дадите, он попросит стакан молока. Он захочет посмотреть в зеркало, чтобы проверить, что у него не остались усы от молока, а затем попросит ножницы, чтобы постричься…
— Лора Джоффе Нумеров, «Если дать мышонку печенье»
Практическое правило: если изменение не из области ревью, то оно не подлежит разбору.
Вот пример:
Даже если вы всю ночь не можете заснуть и мучаетесь из-за магического числа и нелепого названия переменной в коде, это не подлежит разбору. Даже если автор — тот же самый человек, который написал соседние строчки кода, это изменение по-прежнему не подлежит разбору. Если это вопиющий случай, зарегистрируйте баг или предложите собственное исправление, но не заставляйте автора разбираться с этим изменением в процессе код-ревью.
Исключением будет только тот случай, если изменение относится к изменяемому коду, хотя и не входит в него, например:
В этом случае обратите внимание, что автору нужно переименовать функцию с
ValidateAndSerialize на просто Serialize. Это не влияет на сигнатуру функции, но всё равно та становится некорректной с таким названием.Я слегка нарушаю это правило, если в коде немного исправлений и я замечаю какую-то простую правку, пусть и не подлежащую обязательному разбору. В таких случаях я делаю примечание, что автор может проигнорировать замечание, если хочет.
Ищите возможность разбить большие ревью
Если вы получаете список изменений из более чем ~400 строк кода, попросите автора разбить его на более мелкие части. Чем больше строк выходит за пределы этого лимита, тем труднее подготовить ответ. Лично я отказываюсь рассматривать списки изменений более чем на 1000 строк.
Автор может ворчать по поводу разбиения списка изменений, потому что это нудная задача. Облегчите его бремя, предложив логичные границы разделения. Самый простой случай — когда список изменений затрагивает несколько отдельных файлов. Здесь список изменений можно просто разделить между файлами. В более трудных случаях определите функции или классы на самом низком уровне абстракции. Попросите автора перенести их в отдельный список изменений, а после его одобрения вернитесь к остальному коду.
Если код низкого качества, решительно запросите разделение списка изменений. Сложность анализа плохого кода экспоненциально растёт с размером. Вам гораздо проще проводить аудит пары небрежных списков изменений по 300 строк, чем одной большой мерзости на 600 строк.
Искренне хвалите
Большинство рецензентов фокусируются на ошибках в коде, но код-ревью — это великолепная возможность поощрить правильное поведение.
Например, вы рассматриваете код от автора, у которого проблемы с написанием документации — и вдруг натыкаетесь на чёткий, лаконичный комментарий к функции. Скажите автору о его успехе. Он будет быстрее прогрессировать, если вы будете сообщать о правильных действиях, а не просто ждать того случая, когда он облажается — чтобы сообщить ему об этом.
Не нужно иметь какую-то конкретную цель, чтобы похвалить человека. Каждый раз, когда я вижу в списке изменений что-нибудь, что меня радует, я говорю автору об этом:
- «Не знал об это API. Действительно полезная вещь!»
- «Элегантное решение. Никогда о таком не думал»
- Разбить эту функцию было отличной идеей. Так намного проще»
Если автор — начинающий разработчик или недавно присоединился к команде, то может нервничать или занять оборонительную позицию во время ревью. Искренние комплименты частично снимают это напряжение — вы демонстрируете, что являетесь коллегой, который готов помочь, а не враждебным охранником кода с синдромом вахтёра.
Утверждайте ревью, когда остались тривиальные правки
У некоторых рецензентов есть неправильная установка, что они должны тянуть с одобрением ревью до тех пор, пока не увидят исправления каждой ошибки. Это добавляет несколько ненужных раундов ревью, которые впустую тратят время и автора, и рецензента.
Одобряйте код, если верно любое из следующих утверждений:
- У вас больше нет замечаний.
- У вас остались только тривиальные замечания.
- Например, переименование переменной, исправление опечатки.
- Оставшиеся изменения необязательны к исправлению.
- Явно укажите их как необязательные, чтобы у коллеги не возникло впечатление, что от них зависит ваше одобрение.
Я видел, как рецензенты тянут с одобрением, потому что автор не поставил точку в конце комментария. Пожалуйста, не нужно так делать. Так вы указываете автору, что он неспособен поставить простой знак пунктуации без вашего надзора.
Существует некоторая опасность в том, чтобы одобрять код, когда остались нерешённые вопросы. По моей оценке, примерно в 5% случаев автор или неправильно интерпретирует замечания последнего раунда, или полностью упустит их. Чтобы избежать таких ситуаций, я просто проверяю правки, внесённые автором после одобрения. В редких случаях недопонимания я или сообщаю автору, или создаю собственный список изменения с исправлением. Небольшое количество работы в 5% случаев лучше, чем ненужные усилия и задержка в 95% остальных случаев.
Заранее избегайте тупиковых ситуаций
Самый худший результат код-ревью — это возникновение тупиковой ситуации: вы отказываетесь ставить подпись без дополнительных изменений, а автор отказывается их делать.
Вот некоторые признаки того, что вы зашли в тупик:
- Тон обсуждения становится напряжённым или враждебным.
- Объём важных замечаний на каждом раунде не уменьшается.
- Вы сталкиваетесь с противодействием необычно большому количеству своих замечаний.
Поговорите с человеком
Поговорите с человеком в видеочате. Из-за текстового общения легко забыть, что с той стороны живой человек. Слишком легко представить своего коллегу как некое порождение упрямства или некомпетентности. Живая беседа разрушает это неверное впечатление и для вас, и для автора.
Предложите дизайн-ревью
Сварливое код-ревью может указывать на то, что какие-то проблемы не предусмотрели заранее в процессе разработки. Может вы спорите о вещах, которые стоило обсуждать во время дизайн-ревью? Оно вообще было?
Если причина несогласия лежит в высокоуровневом выборе архитектуры проекта, то решать проблему должен более широкий круг людей, а не просто два человека, которые оказались втянуты в это обсуждение кода. Поговорите с автором о том, чтобы расширить обсуждение на всех остальных участников группы разработки в виде дизайн-ревью.
Уступка или эскалация
Чем дольше вы с коллегой топчетесь в тупике, тем больше вреда наносите своим отношениям. Если альтернативы вас не устраивают, то остаётся только два варианта: уступить или обострить.
Оцените, во что выльется одобрение изменений. Вы не можете создавать качественные программы, если постоянно одобряете низкокачественный код, но вы также не сможете добиться высокого качества, если настолько ожесточённо воюете с коллегой, что больше не можете вместе работать. Насколько велик вред, если вы в реальности одобрите список изменений? Этот код способен повредить критически важные данные? Или это фоновый процесс, где в самом худшем случае грозит сбой задачи и необходимость отладки? Если второй вариант более точно описывает ситуацию, то подумайте о том, чтобы уступить и сохранить возможность дальнейшего сотрудничества с коллегой в нормальных условиях.
Если уступить не вариант, поговорите с автором насчёт обсуждения с менеджером группы или техническим руководителем. Предложите назначить другого рецензента. Если эскалация конфликта вам невыгодна, согласитесь с решением и двигайтесь дальше. Продолжение спора приведёт к возникновению плохой ситуации, где вы выставите себя в непрофессиональном свете.
Восстановление после тупика
Сумбурные аргументы в процессе обсуждения не столько относятся к коду, сколько к отношениям между людьми. Если вы попали в тупик или близки к нему, такая ситуация может повториться, пока не будет решён основополагающий конфликт.
- Обсудите ситуацию с менеджером.
- Если конфликт возник в вашей команде, менеджер должен знать о нём. Может быть, с этим автором просто сложно работать. Возможно, вы влияете на ситуацию неким образом, какой сами не понимаете. Хороший менеджер поможет вам обоим решить эти проблемы.
- Отдохните друг от друга.
- Если возможно, постарайтесь не проводить код-ревью друг у друга несколько недель, пока страсти не утихнут.
- Изучите способы разрешения конфликтов.
- Мне показалась полезной книга «Важные разговоры». Её советы могут показаться очевидными, но есть огромная ценность в анализе своего поведения в конфликтной ситуации со стороны, когда вы не находитесь в пылу спора.
Моё худшее код-ревью: работа над ошибками
Помните код-ревью с Мэллори? Почему моё стало трёхнедельным мучением с пассивно-агрессивной жижей, а Боб пронёсся ветерком за пару дней?
В чём я ошибся
Это было первое ревью для Мэллори в нашей группе. Я не подумал, что она может чувствовать предвзятое отношение или занять оборонительную позицию. Мне следовало начать только с высокоуровневых пометок, чтобы не хоронить её под грудой замечаний.
Мне следовало больше постараться, чтобы показать: моя задача не препятствовать её работе, а скорее помочь. Я бы мог предоставить примеры кода или отметить положительные моменты в её списке изменений.
Я позволил своему эго повлиять на ревью. Я потратил целый год, восстанавливая эту старую систему до работоспособного состояния. Внезапно появляется новый человек, который с ней возится, и она не хочет всерьёз принимать мои замечания? Я воспринял это как оскорбление, но такое отношение стало контрпродуктивным. Мне следовало сохранить объективное отношение, которое я пытаюсь принести во все свои ревью.
В конце концов, я слишком долго позволил сохраняться тупиковой ситуации. После нескольких раундов следовало понять, что дальнейший прогресс отсутствует. Я должен был радикально изменить ситуацию, например, встретиться лично для разрешения более глубокого конфликта или перенести обсуждение на уровень менеджера.
Что Боб сделал правильно
То, что Боб первым делом разбил ревью на части, оказалось очень эффективным. Вспомните, что ревью застопорилось на три долгие недели страданий. И тут внезапно он одобрил два фрагмента кода. Это придало позитивных эмоций и Мэллори, и Бобу, потому что дело сдвинулось с мёртвой точки, наметился прогресс. В остальном коде по-прежнему остались проблемы, но список изменений уменьшился и стал проще в работе.
Боб не пытался довести код до совершенства. Вероятно, он нашёл те же проблемы, о которых кричал я, но он учёл, что Мэллори недавно начала работать в нашей команде. Благодаря проявленной в данный момент гибкости он поможет Мэллори улучшить качество кода в долгосрочной перспективе.
Вывод
После публикации первой части статьи некоторые читатели высказали критику относительно стиля общения, который я рекомендовал. Некоторые назвали его покровительственным. Другие беспокоились, что он не слишком прямой и возникает риск недопонимания.
Такие отзывы разумны и их можно было ожидать. Один считает, что краткие комментарии выглядят резкими и грубыми. Другой может назвать их лаконичными и эффективными.
Осуществляя аудит кода, вы принимаете много решений: на чём сконцентрироваться, как оформить отзыв, когда одобрять результат. Не обязательно принимать те варианты, которые я предлагаю. Просто имейте в виду, что есть варианты.
Никто не даст вам рецепт идеального ревью. Наиболее эффективные техники в каждом конкретном случае зависят от личности автора, ваших взаимоотношений и корпоративной культуры. Оттачивайте свой подход, критически размышляя о результатах своего ревью. Когда возникает напряжённость, возьмите паузу и оцените происходящее. Уделяйте внимание качеству своих замечаний и предложений. Если вы не можете довести код до своих стандартов качества, подумайте, какие помехи есть в процессе ревью — и как их устранить.
Удачи, и пусть ваши код-ревью будут человеческими.
Дополнительная литература
Д-р Карл Вигерс — единственный из встреченных мной авторов, кто уделил должное внимание социальным факторам в код-ревью. Он хорошо подытожил свои тезисы в статье «Очеловечивание экспертиз». Написанная в 2002 году, она сохраняет свою актуальность, демонстрируя долгосрочную ценность эффективного общения.
Комментарии (146)
ivan2kh
13.11.2017 15:13+3Коротко обрисую ситуацию: В команду пришла новая сотрудница, которая фигачит говнокод и слушает только начальника (напомню, автор находится на одном уровне с сотрудницей). При этом автор гораздо более компетентен технически чем сотрудница и, я так полагаю, чем менеджер. Напомню, сотрудница в компании уже нескольно лет и выработала поведение при ревью. Вместо того, чтобы жадно впитывать новые знания, она вредит продукту, команде и компании.
Вывод: не теряйте время, ищите людей таких же целеустремленых и желающих учиться, как вы.YaMishar
13.11.2017 15:47Однако автор не влияет на подбор персонала, потому я предложу другой вывод: налаживайте отношения с коллегами, какими бы тупыми говнокодерами они вам ни казались. Более того, может оказаться, что ваше мнение о них изменится в будущем.
myxo
13.11.2017 15:54И будет у вас идеальная компания с идеальными разработчиками, живущими где-то в параллельном мире.
ivan2kh
13.11.2017 17:24Я же говорю про людей которые хотят учиться. Почему бы не делать компанию только из таких людей.
zvulon
13.11.2017 19:42потому что такие люди стоят на порядок дороже
senya_z
14.11.2017 09:56их еще и на порядок меньше
Ivan22
14.11.2017 10:05+1и еще я с ужасом представляю себе команду из 7-8 человек, все из которых хотят учится.
igorp1024
14.11.2017 12:35Либо я не понял сарказма, либо я не понял вообще. :)
Почему с ужасом? Они будут тратить на обучение слишком много времени вместо того, чтобы работать?
Neikist
14.11.2017 13:06Ну, на работе ведь работают, так что какая разница сколько они свободного времени тратить будут. Я например себе иногда позволяю пол часика выделить в рабочее время на интересные вещи, но вообще все самообразование, в т.ч. и по неинтересным но нужным работодателю темам только в свободное время, я же свое рабочее время продаю. Иногда конечно нужно что изучить «вот прям щас», но это уже по производственной необходимости, если сроки поедут в случае изучения в свободное время.
quantum
14.11.2017 14:11Плохо, что вы рабочее время продаете, а не вклад :)
У 1сников распространен термин «жопочасы» для этого :)Neikist
14.11.2017 14:31А я кем работаю? Если надрываться и решать задачи быстрее и лучше — никто не оценит, только больше навалят при той же оплате. Ну и естественно что отработано в месяц должно быть не меньше чем 8*количество дней рабочих. Именно отработано, а не на обучение. На какой результат в таких условиях работать можно?
Ivan22
14.11.2017 18:03потому что команды из них не выйдет…
igorp1024
14.11.2017 18:11Ну, у программеров учёба — неотъемлемая часть профессиональной деятельности. Хоть ты джуниор, хоть синьор… Надоело учиться — конец профессионализму.
Или я опять неверно понял?Ivan22
15.11.2017 10:09просто для хорошей команды именно как команды, важны совсем другие качества, совершенно перпендикулярные, такие как например: «умение находить компромис», «не зашкаливающее самомнение», «умение работать на общий результат» ну и т.п.
khim
13.11.2017 23:55Потому что если вы вывалите на человека 100500 замечаний, то любой, сколько угодно «желающий учиться» человек «встанет в позу». Особенно если вы будете наставивать на каких-то мелочах (или чем-то, что кажется мелочью «с другой стороны»).
DummyBear
14.11.2017 09:23Например, потому что «хочет учиться» это не неотъемлемое свойство человека, а скорее одно из его состояний.
newartix
13.11.2017 16:43Что характерно, в статье и в вашем комментарии упоминается пол разработчика. У меня тоже был опят столкновения с подобным. Есть подозрение, что женщины склонны к подобному поведению, воспринимать замечания как личные оскорбления. Или во мне заговорил сексист?
Ivan22
14.11.2017 10:08я долго руководил командой из 10 разработчиков из которых 6 были девушки. Мой ответ — нет. Но есть исключения, когда две девушки могут друг друга нелюбить, скажем так, но это все таки больше исключение чем правило.
MacIn
13.11.2017 18:32При этом автор гораздо более компетентен технически чем сотрудница и, я так полагаю, чем менеджер.
Нет. Автор думает, что более компетентен, но несмотря на это, не может доказать свою точку зрения новичку. Либо он не настолько компетентен, либо не умеет общаться (именно на это упор в статье), либо изменения того не стоят и спорны.mayorovp
14.11.2017 09:02Либо выбранный формат код-ревью не дает возможности полноценно высказать свою точку зрения.
senya_z
14.11.2017 09:34никто не запрещает ему в дополнение к выбранному формату код-ревью подойти к новичку и обговорить-обсудить спорные моменты. возможно, объяснив свою точку зрения, он продвинется дальше чем у него вышло, выписывая повести в код-ревью.
mayorovp
14.11.2017 09:36Но он почему-то этого не сделал. Возможно, он верующий в инструмент для код-ревью…
senya_z
14.11.2017 09:44скорее, недальновидный сотрудник, не способный сделать шаг назад и рассмотреть картину целиком, замкнувшийся на факте того, что новичок не хочет выполнять разумные (с его точки зрения) требования изменить код. и вместо того, чтобы решить проблему при помощи диалога и продолжить работу, он три недели провел, злясь каждый день, пререкаясь в код-ревью с новичком. возможно, технически этот человек отличный специалист, но soft skills у него не на высоте, что отражается на продуктивности команды.
Antervis
13.11.2017 21:30Вывод: не теряйте время, ищите людей таких же целеустремленых и желающих учиться, как вы.
абстрактные Мэллори/Вася/Петя/Айрат могут быть отличными профессионалами своего дела и некомпетентными чуть вне его. Задача ревью в первую очередь — не допустить говнокод в продакшн, во вторую — научить сотрудника. Если научить не получается (иногда это на самом деле и не нужно), то зачастую лучше и проще сделать как надо самому.
search
13.11.2017 22:40Ну такое. Сейчас работаем вдвоем с человеком, желающим учиться и вообще таким же как я целеустремлённым. Код ревью иногда в ад превращается. Оба рогом упираемся и пишем друг другу поэмы в комментариях. Так что команда, состоящая из золотых специалистов — не всегда гарантия безстрессовой работы. Где есть два человека, там конфликт неизбежен.
Но в последнее время заметил, что код ревью всегда проходит проще, когда ревьювер (в данном случае я сам) искренне задаёт себе вопрос "как я могу помочь этому человеку?". Обычно сразу после этого вопроса приходит соломоново решение. Главное вспомнить о нём вовремя :)
TheShock
14.11.2017 08:06+1Где есть два человека, там конфликт неизбежен.
У нас сейчас команда побольше и ревью просто прекрасные. Более того, мы притерлись к стилю друг друга, выработали хорошие практики и, в итоге, ревью значительно сократились. При этом с самого начала у нас не было ни одного конфликта на этой почве.search
14.11.2017 10:52Ах как вам повезло что я у вас не работаю!
Но, если серьёзно, то я бы лучше не хвастался хорошими отношениями с людьми, а то можно беду накликать.TheShock
14.11.2017 10:56Я не суеверный. Наши отношения в наших руках)
Просто я хотел сказать, что где два человека — конфликт вероятен, но не неизбежен)search
14.11.2017 11:13Как сказал классик, "На большом отрезке времени шансы каждого из нас на выживание близки нулю." Эту же фразу можно переиначить и об отношениях с людьми.
Ну и тут дело не в суеверности, а в том, что хорошие отношения — это очень большая работа. Причем, работа всех участников предприятия. То что у вас в команде гладкие отношения с коллегами, говорит о том, что все участники упорно трудятся над этими самыми отношениями. А когда хвастаешься тем что хорошо работаешь, то обычно сразу после этого позволяешь себе слегка расслабиться. Сорри, что я вообще начал этот разговор.
PqDn
14.11.2017 13:46Не всегда так, иногда компании нужны не стахановцы, а те, кто за свои 8 рабочих часов будет делать, что от них требуют. Да и не по карману многим целеустремленные…
Вывод: Только воспитывая можно добиться таких результатов
lpwaterhouse
13.11.2017 15:56+1Дичь какая-то. Сказали переделывать — переделывай. Нравится, не нравится, считаете что ваш код лучше — вооружитесь от начальниками списком правок и в случае чего тыкайте туда пальцем, мол сказали делать так. А тут какое-то раздувание проблемы на ровном месте. Программисты похоже стали забывать, что отсутствие «вербальной» субординации не отменяет фактическую и начальник остается начальником, а значит требования его нужно выполнять. Как максимум обрисовать ему свое видение ситуации, но не настаивать на своем решении как единственно верном.
sbnur
13.11.2017 15:57Любой код ревью портит отношения между коллегами по разработке
Проще взаимоотношения между разработчиком и тестером — они антагонисты по природе — это воспринимается, как данное, и не создает психологических трений.
Также любой код, как известно (мне, по крайней мере), может получить приставку г.quantum
13.11.2017 16:06>Любой код ревью портит отношения между коллегами по разработке
У нас в команде не портит. Замечания все воспринимают как должное
>Также любой код, как известно (мне, по крайней мере), может получить приставку г.
Одна из прелестей ревью — через некоторое время уровень и ожидания от кода в команде синхронизируются и люди уже не отдают на ревью г-кодMacIn
13.11.2017 18:33+1У нас тоже не портит. Потому что любое код-ревью — это помощь со стороны (а-ля две пары глаз лучше, чем одна), а не менторство «я знаю лучше, что и как, делай, как говорят».
senya_z
13.11.2017 22:17В случае сервисов, где баг может послужить причиной алерта и соответственно звонка посреди ночи с последующим дебагингом, за любой косяк, найденный в код-ревью, еще и благодарят от души.
Petrenuk
14.11.2017 02:30Я вообще обожаю посылать свой код на ревью. Жадно хватаюсь за каждое предложенное улучшение, даже если это code-style или небольшой трюк, который позволяет написать код короче и чище. Не говоря уже о серьезных недочётах, мне же потом и поддерживать.
sbnur
14.11.2017 04:56Сделал такой маленький эксперимент — высказываю свое мнение (подчеркивая это) и получаю кучу минусов — это и есть естественная реакция на любой код ревью
Petrenuk
14.11.2017 05:02Что-то с вами не так. Люди намекают вам, что ваше мнение не незыблемо, возможно стоит его пересмотреть.
sbnur
14.11.2017 07:23А у вас зыблемо?
TheShock
14.11.2017 08:12+1Любой код ревью портит отношения между коллегами по разработке
Знаете, а у нас код-ревью даже что-то сродни тим-билдинга и отношения наоборот укрепляются. Так что вы ошибаетесь.
Проще взаимоотношения между разработчиком и тестером — они антагонисты по природе
Тут вы, кстати, тоже ошибаетесь. Они как танк и хилер в играх. Когда я работал в Варгейминге мы с тестерами были одной командой и реально вместе работали над тем, чтобы сделать результат наиболее качественным. То есть я не воспринимал их как: «вот подлецы, баг нашли!», а как «класс, они нашли баг и игроки его уже не увидят».
А из-за вашего категоричного заявления и невежества, которое вы проецируете на всех — вас и минусуют.
alix_ginger
14.11.2017 12:06+1Если в команде код ревью вызывает психологические трения, к чьему-то коду придираются, добавляя приставку «г-», а тестеры — антагонисты программистов, то проблема не в код ревью.
Джуниор, делающий замечания по коду сеньора — хороший джуниор. Сеньор, прислушивающийся к ним (и научивший джуниора так делать) — хороший сеньор. И наоборот это тоже верно.
dmitry_dvm
13.11.2017 16:24С удовольствием поработал бы в конторе, где ревьювили бы мой код. Халявное обучение же.
yizraor
13.11.2017 19:29И я, и я, и я, того же мнения :)
В командной разработке бывал, но с код-ревью не сталкивался, и вообще взаимодействия с более опытными коллегами не хватает...
kvasvik
13.11.2017 16:43Я специально дождался, пока она уйдёт с работы, потому что не хотел находиться с ней в одной комнате, когда она получит письмо.
То есть, они сидели в одной комнате, но вместо того чтобы выяснить отношения по человечески, 3 недели перекидывались письмами? Я бы уволил обоих за имитацию деятельности вместо работы.quantum
13.11.2017 18:56Угу, у нас есть негласное правило — начался срач в ревью — садимся в переговорку и обсуждаем. Пару раз приходилось привлекать арбитра :)
silveruser
14.11.2017 09:22Всецело поддерживаю, единственный нормальный (без выпячивания XCD) комментарий в этом топике! Два бездельника тратят деньги компании на вопрос, который выеденного яйца не стоит… kvasvik, в нашем с Вами идеальном мире такого бы не могло случиться!
Но, увы: уж сколько я за свою профессиональную деятельность в software development наблюдал таких… «работничков»… По средневзвешенной оценке, порядка 50% в «молодых стартапах», и до 80-85% в «старых, солидных компаниях» (там, где врастают корнями в свое офисное кресло, и десятилетиями колупают с умным видом протухший говнокод). Как правило, такие «работнички» вместо работы очень любят постить во всевозможные блоги, а также обожают всяческие митинги — «типа профессиональная этика» (а, может, просто не любят/умеют/хотят) не дает им сидеть в рабочее время в социальных сетях/играть в игры/ваять homebrew «нетленку»/«халтурить», вот потому они и так рады бессмысленным митингам и «конфликтам», высосанным из пальца.
100% no hire! Но, увы, в идеальной вселенной… В реальной я сам лично наблюдал, как человек просто-напросто «подставил» компанию, принеся реальный убыток, мотивируя лишь тем… что у сына школьная игра в бейсбол на уровне районных школ (т.е. даже не county, и не штата), которую он не может пропустить! И, самое странное, что его не уволили тут же — не знаю даже, почему (возможно, испугались судебных издержек — сложно сказать).TheShock
14.11.2017 10:53Не захотел овертаймить в субботу и пошел на игру сына? Мужик!
silveruser
14.11.2017 14:01Нет. Отказался билдить релиз (который ему по рабочим обязанностям положено делать, как релиз менеджеру), «послал» CTO и ушел в два часа дня. И это на фоне showstopper-а и просьбы от клиента с многомилионным контрактом выдать багфикс ASAP.
P.S. Понятное дело, что сделали и без него (вообще, по хорошему, его должность была практически чистая синекура), но сам факт…
MockBeard
13.11.2017 17:28дичь какая-то. чувак занимается тем, что старается не обидеть коллегу, т.к. она просто не хочет делать то, что ей рекомендует более опытный коллега.
HappyGroundhog
13.11.2017 22:54Видимо менеджеры-технари смотрят на статью несколько иначе. Еще раз вводные из статьи
Она никогда раньше не писала на Python и никогда не разрабатывала систему поверх неуклюжей легаси-системы, которую мне приходилось поддерживать.
Девушка написала свое первый код на Питоне, и не хочет выполнять замечания более опытного коллеги который, судя по всему, повел себя как упрямый баран и вместо помощи выкатил ей список на 100 листов замечаний ожидая от неё кода синьора да еще и в легаси, которое он сам долго причесывал?! Кто из них двух разумнее? Если бы ваш первый код зеревьювил бы по самое «не балуйся» гуру из Гугла? Вряд ли бы вы сейчас программировали, т.к. редко кто готов чувствовать себя полным ничтожеством.
bak
14.11.2017 00:30Оба хороши. И ревьюер мог снизить планку, и коллега мог спокойно поправить все замечания, разобравшись с тем что хочет ревьюер.
netch80
14.11.2017 07:42+1и вместо помощи выкатил ей список на 100 листов замечаний
А замечания это разве не помощь? (Разумеется, я предполагаю замечания вида "тут желательно сделать отдельную функцию" или "не соответствие стилю XYZ" при наличии руководства по стилю, а не "код говно, переделывай").
Есть совершенно конкретный список, что не устраивает, и чего хотелось бы видеть вместо. Что ещё может быть "помощью" на ревью?
Если бы ваш первый код зеревьювил бы по самое «не балуйся» гуру из Гугла? Вряд ли бы вы сейчас программировали, т.к. редко кто готов чувствовать себя полным ничтожеством.
Ревью от гуру из Гугла имеет смысл для кода для гугловского продукта, но тогда это не может быть чей-то первый код (не считая тривиальнейших правок в две строчки).
И точно так же гуру из Гугла может быть ревьюером для кода своего ребёнка, смягчая требования до подъёмных соответственно возрасту и опыту, но этот код не будет предназначен для гугловского продукта.
Девушка написала свое первый код на Питоне
Я не девушка, но попадал в аналогичные ситуации. Или код забирался так, что его правил кто-то другой и уже коммитил под собой, или требовал выполнения таки описанных правил по стилю и функциональности. Если не впадать в психологические позы обструктивизма, проблем не было.
netch80
14.11.2017 07:55Необходимое уточнение: всё это уже за пределами основной идеи статьи. Вы пошли на безусловное оправдание Мэллори, я возражаю против этой позиции, но полезное в статье не об этом, а о том, как уже имея участников с крайними позициями — умудриться примирить их.
Поэтому данная сублиния обсуждения не суть важна.
mayorovp
14.11.2017 09:13Если бы ваш первый код зеревьювил бы по самое «не балуйся» гуру из Гугла?
… то некоторых моментов в том коде я бы сейчас не стыдился :-) А ваши предположения о том что я бы сейчас не программировал не имеют под собой основания.
MockBeard
14.11.2017 11:12ок, невнимательно просмотрел статью, был неправ. с начинающими нужен совершенно другой подход, почти как с детьми )
MacIn
14.11.2017 17:20Девушка написала свое первый код на Питоне, и не хочет выполнять замечания более опытного коллеги
Вполне возможно, что его замечания не питоно-специфичны, так что ее малый опыт на данном ЯП и не имеет значения.
Cheater
13.11.2017 21:01Автор странный. Коллега первая нарушила правила нормального взаимодействия, проигнорировав фидбек. Кнопочка review rejected и до свидания чинить свои ~100 ошибок. в следующий раз научится, что каждый коммент ревьюверов надо или выполнять, или открывать по нему обсуждение, если не согласна.
avallac
13.11.2017 21:33О, вот и роботы подтянулись, упомянутые в первой статье. Автор пытается получить результат, а не с помощью силы почесать свое ЧСВ. В одиночку большой проект не вытянуть, а значит надо уметь кооперироваться, договариваться и иные непонятные для роботов слова.
lpwaterhouse
13.11.2017 22:44Не надо договариваться с менее опытным сотрудником, если он не понимает как надо сделать и при этом еще и качает права. Ему надо популярно объяснить, что так дела не делаются, отправить переделывать, а в случае повторения выгонять к чертовой матери — все равно не работает.
avallac
13.11.2017 23:03А потом на проекте все зависит от одного человека — выгоревшая суперзвезда, у которой глаз дергается. Договариваться надо со всеми участниками, а иначе это верный способ убить проект. Вообще, Я бы рекомендовал свое ЧСВ как-то иначе выражать. Спортом там заняться, где результаты формализованы, или кибер-спортом. Там, да, можно точно сказать, что ты круче и бить пяткой в грудь.
side2k
14.11.2017 00:05lpwaterhouse правильно говорит.
В коде тоже довольно легко формализовать оценку отдельно взятого фрагмента.
Например, если я в ревью придираюсь к тому, соответствует ли код стандартам PEP-8 — то это означает, что у нас заранее было оговорено, что мы придерживаемся PEP-8.
Если то, как предлагаю сделать я — быстрее/безопаснее/надёжнее/читабельнее, и я могу это доказать — значит так и надо делать. Если мне, при ревью моего кода, аргументированно такое доказывают — я не упираюсь рогом и не трачу время ни ревьювера, ни своё, ни кого-либо ещё из команды. Это то, что я считаю профессионализмом. Формулируя чуть короче — профессионализм, это когда стараешься делать своё дело лучше и быстрее, избавляясь от того, что тебя сдерживает.
А заниматься выстраиванием отношений в команде вместо работы над продуктом — это хипстерская херня. Да, коммуникация важна. Да отношения с коллегами надо поддерживать.
Но всё это не должно быть самоцелью, мы здесь работать собрались, а не играть клуб анонимных алкоголиков.
Сейчас в среде разработчиков принято называть «суперзвёздами» или «рок-звёздами» тех, кто кроме своего эго ничего не видят, не умеют делегировать, и, в результате всего этого — замедляют работу над продуктом, а не ускоряют. И, в итоге, сгорают сами.
Я считаю, это неправильно.
Те, кто выгорел — это не суперзвёзды. Настоящие суперзвёзды, как Линус Торвальдс, год за годом локомотивом пробивают пробивают дорогу не только себе, но и другим.bak
14.11.2017 00:39В данной ситуации конечно же коллегу, отказавшуюся вносить правки профессионалом назвать нельзя. Но и к ревьюеру это тоже относится. Можно было снизить планку по требованиям, поговорить с коллегой и убедить внести правки, на худой конец заапрувить а затем отрефакторить. Но нет, вместо этого три недели потраченного впустую времени и незакрытая задача.
side2k
14.11.2017 00:48>можно было снизить планку по требованиям,
Т.е. сознательно ухудшить продукт, просто чтобы подыграть чувствам коллеги?
> и убедить внести правки
Вот здесь — да. Но я сам неоднократно сталкивался с тем, что менее опытные коллеги упираются рогом просто потому, что не понимают сути моих претензий.
Я в таких случаях рогом в ответ не упираюсь. Не нравится моя оценка — либо докажи мне, что я неправ, либо мёрджи без моего подтверждения, а потом сам объясняйся с руководством, почему ты так сделал. Я больше на это своё время тратить не буду. Sapienti sat.khim
14.11.2017 03:46+1Я в таких случаях рогом в ответ не упираюсь
Уммм.
Не нравится моя оценка — либо докажи мне, что я неправ, либо мёрджи без моего подтверждения, а потом сам объясняйся с руководством, почему ты так сделал. Я больше на это своё время тратить не буду. Sapienti sat.
Вообще-то такое поведение и называется «упереться рогом» и является лучшим способом добиться того, что работа будет выполняться «на от$бись».
TheShock
14.11.2017 08:04+2Можно было снизить планку по требованиям
Объясните, пожалуйста, зачем снижать планку по требованиям? Если советы в код-ревью дельные, то их стоит учесть и исправить, но:
— если это слишком сложно для новичка — он может попросить о помощи
— если их аргументированость под сомнением — обсудить, аргументировать, пригласить других членов команды
— если времени слишком много на исправление — обсудить с командой. Возможно, найти компромиссный вариант. Прям сейчас это время текущей задачи. И качественный код включен в это время. Если выбился за эстимации — это не означает, что необходимо жертвовать качеством, а означает, что необходимо улучшить эстимации. Потом, увы, времени на исправление не будет — будут в работе другие задачи. В крайнем случае серьезного дедлайна, конечно, качеством можно пожертвовать, но такие ситуации бывают редко.
Пока это выглядит так: новенькой девочке было то ли лень, то ли просто обидки, а потому — она устроила саботаж на работе, убила 15-30 человеко-дней, испортила атмосферу в команде и… получила положительное подкрепление — работу по объемному фидбеку она так и не сделала.
Инициативный человек, который оставлял хорошие фидбеки — крайне демотивирован, чувствует свою вину за желание качества кода, скорее всего будет далее писать код и фидбеки значительно хуже, с коллегой отношения натянуты.
Команда видит, что если ты бахнул пивка в обеденный перерыв и тебе влом делать правки по ревью, то ты надуваешь губки, устраиваешь недельный саботаж и к тебе прибегает лид и начинает тебя облизывать, в то время как твой «обидчик», который посмел критиковать твой код — наказан.
Короче решение из статьи — как писать в штаны на морозе. Сейчас тепло, конфликт вроде замяли. В средне-срочной перспективе — одни минусы.
Должен заметить, что такое решение прекрасно подходит для низкосортных галер, где нанимают всех подряд, 3-6 месяцев из них выжимают все сроки с графиком 80 часов в неделю, а потом проект — толкают, команду — выбрасывают, поддержку отдают на аутсорс в Индию. Краткосрочный выигрыш от решения конфликта — крайне важен, а среднесрочные риски могут не успеть сработать.MacIn
14.11.2017 17:24Так а кто решает, дельные они или нет?
Мне вообще непонятен кодревью в стиле «вот это и это неверно, переделай, или не получишь некий аппрув». Это как вообще? Код-ревьювер — твой партнер, а не учитель и не ментор. Он не имеет права требовать каких-то изменений. Он твой… дублер, твоя страховка на случай, если ты что-то упустил. Да, он может дать рекомендации по поводу того, что можно было бы сделать лучше (типа именований, что субъективно), но требовать это выполнять — … новости с другой планеты какие-то.netch80
14.11.2017 19:01Код-ревьювер — твой партнер, а не учитель и не ментор. Он не имеет права требовать каких-то изменений.
В командной работе такие принципы часто не применяются, и какие-то коллеги не просто имеют право требовать изменения — они обязаны это делать, если видят несоответствие задаче, ТЗ, стилю и так далее. И по итогу этого всего они обязаны поставить свою оценку и отвечают за неё — если пропущен откровенно плохой коммит, по разбору пострадают и они.
Типовой стиль Gerrit, например, настроен на это — для допуска ревью к коммиту кто-то должен поставить +2 на "Review". В принципе, можно поставить самому себе :), но будет заметно.
Польза от такого подхода достаточно очевидна — новые правки будут хотя бы известны команде и, если она приняла их — то они соответствуют (хотя бы частично) ожиданиям и требованиям. Недостатки тоже понятны: размывание ответственности, задержки на сбор оценок — но при нормальной дисциплине работы они не мешают.
новости с другой планеты какие-то.
Ну я на этой планете несколько лет работал. И уверен, даже среди читателей этой статьи таких найдётся немало. И не скажу, что это было как-то ужасно — наоборот, (повторюсь) польза была видна всем.
MacIn
14.11.2017 19:09В командной работе такие принципы часто не применяются, и какие-то коллеги не просто имеют право требовать изменения — они обязаны это делать, если видят несоответствие задаче, ТЗ, стилю и так далее.
Не обязаны. Отметить несоответствие и выдать рекомендацию — да. Требовать — нет.netch80
14.11.2017 19:16Вынужден повториться: я работал именно с такой организацией.
Видишь просто несоответствие стиля, недочёты, не влияющие на работу, но которые могут вылезти потом => ставь -1.
Видишь, что сломается в принципе => ставь -2 (срабатывает запрет, пока отметка не будет снята).
Если не поставил, пропустил коммит => принимаешь долю ответственности.
Если именно у вас так не делают, это не значит, что не делают нигде.
И я считаю, что так в очень многих случаях правильно — когда работающие над некоторым участком кода обязаны участвовать в ревью любого изменения этого участка. Иначе слишком легко оторваться от знания, что там происходит.
Да, не в 100% случаев. Точную цифру бы не дал. Но полагаю, что на большинство случаев.
khim
14.11.2017 17:33Потом, увы, времени на исправление не будет — будут в работе другие задачи.
Серьёзно? То есть вы вот так — берёте и сходу пишите идеальный код?
Вы уникум, вас «в палату мер и весов надо». У меня так не получается. Код, который я написал год назад я сейчас, скорее всего, напишу по другому. Потому что задачи изменились (и я про это знаю), я изменился (я же за год что-то выучил?), инструменты изменились (год назад мне пришлось «делать маленький велосипед», а сейчас — достаточно три строчки ниписать, ага).
А раз код всё равно нужно улучшать и менять — то так ли важно когда это делать? Я не говорю про патологические случаи типа 50 копий одной функции для разных типов данных вместо одой шаблонной. Я говорю про вещи, которые можно сделать так — или иначе. А по закону тривиальности именно такие вещи будут вызывать больше всего споров на code review.
Назвать переменный destination и source или dest и src? Если Вася делает так, а Петя — иначе, то код действительно выглядит слегка неряшливо, но… настолько ли это серьёзная проблема, чтобы тратить на наё часы и дни?
Если хочется какого-то одного стандарты — ну выдилите вы пару дней, чтобы подобные механические вещи сделать. Если все переменные паре сотен функций называется dest и src, то даже Вася, скорее всего, не будет спорить.
Короче решение из статьи — как писать в штаны на морозе. Сейчас тепло, конфликт вроде замяли. В средне-срочной перспективе — одни минусы.
Ну дык. Хочу только напомнить, что фраза про «писанье в штаны на морозе» — была сказана Ансси Ваньоки в отношении Андроида. Типа: в краткосрочной перспективе — это даст производителям телефонов краткосрочное облегчение, но в долгосрочной перспективе — у них будут проблемы. И он, в общем-то, был прав: у производителей, выбравших Андроид (который был как раз разработан с подходом, описанным в статье — «стремитесь повысить уровень качества кода только на одну-две ступени») куча проблем… а вот гордая Нокия, выбравшая другой подход — телефоны перестала производить вообще.mayorovp
14.11.2017 18:42Цитирую пост:
Через несколько дней Мэллори прислала новый список изменений и ответ на мои заметки. Она исправила самые простые ошибки: опечатки, названия переменных и т.д. Но отказалась решать проблемы высокого уровня, такие как неопределённое поведение кода для искажённых входных данных или то, что структуры потока управления в одной из её функций находились на шестом уровне вложенности. Вместо этого она печально пояснила, что исправление этих проблем не стоит времени разработки.
Как видно, все тривиальные вещи были без вопросов исправлены, а откладывались именно признанные серьезными.
avallac
14.11.2017 00:39Самые ужасные проекты (в первую очередь с точки зрения бизнеса), за которыми мне удавалось наблюдать, почему-то велись адептами «идеального» кода, серебряных пуль, патернов и принципов. Бывает, что адепты создают и качественный продукт, но вот ни разу не наблюдал, чтобы люди умеющие договариваться создавали такой адЪ. В этих проектах все было замечательно: пяток языков программирования, столько же разношерстных баз, адовое нарушение этих самых принципов (с горой оправданий, что мы все по уму, но нам бизнес все испортил своими желаниями), а главное заправлено это все зависимостью от единого разработчика. Как следствие безумные расходы на поддержку монстра. Но формально все хорошо, стандартов придерживаемся, код ревьюим (ну и что, что разработчики разбегаются).
Смею предположить, что на заводском конвейере «хипстерская херня» Вас не достанет. А в IT, все же напомню, единственный ресурс это люди. Кодовая база моментально устаревает и теряет актуальность и без человеческого ресурса просто умирает.side2k
14.11.2017 01:00Самые ужасные проекты (в первую очередь с точки зрения бизнеса), за которыми мне удавалось наблюдать, почему-то велись адептами «идеального» кода, серебряных пуль, патернов и принципов
Вот вы взяли и навесили мне ярлык. А я разве где-то говорил про идеальный код? Я говорю о том, что во главе угла должна стоять задача, а не умащивание нежных чувств отдельных членов команды. Если человек пишет говнокод — значит он должен его исправить. Если я вижу, как его исправить с уровня F (в системе координат автора статьи) до уровня A, то почему мы должны остановиться на D? Нет, если переход на A требует неадекватных затрат времени, то мы можем остановиться и на B. Но почему D? Просто потому что автор кода — эмоциональная девушка, не желающая уступать?
но вот ни разу не наблюдал, чтобы люди умеющие договариваться создавли такой адЪ
Во-первых, если вы чего-то не наблюдали лично — то этого не существует?
Во-вторых — вы так говорите, словно я ратую за то, что договариваться ни в коем случае нельзя. Я лишь говорю о том, что работа не должна превращаться в бесконечные переговоры. Если два разработчика упёрлись рогом на код ревью — они оба неправы, какова бы ни была их квалификация.avallac
14.11.2017 01:25Среди мужского населения желания уступать не сильно больше, особенно когда это подается под очень странным соусом «я тут самый умный». Попробуйте посмотреть на вопрос в долгосрочной перспективе. Не в ключе, что сейчас таску решим, дыру заткнем, а дальше хоть трава не расти, а именно в динамике. Не важно же кто прав или не прав, проблема в том, что два человека в команде не смогли договориться и тратят свои сили не туда. И как Вы лично считаете, что произойдет с видением ситуации и мотивацией у каждой из стороны конфликта в случае силового решения?
Вот моя версия. Задавленная сторона скорее всего получит минус в мораль. При прохождении критической отметки либо уволится, либо будет выполнять таски формально для галочки. Вторая сторона в итоге получает положительное подкрепление к агрессивной модели поведения, что в итоге приводит к росту ЧСВ со всеми вытекающими. А это все разворачивается на глазах других участников команды, что морали им не прибавит. Считаем, что конфликт задушен в зародыше, но по факту полечили симптомы, получив осложнения.
side2k
14.11.2017 02:58Не важно же кто прав или не прав, проблема в том, что два человека в команде не смогли договориться и тратят свои сили не туда
Или я как-то непонятно изъясняюсь, или вы странно меня читаете.
Я как раз предлагаю тратить ресурсы не вникуда, а на полезную деятельность.
И зачем вы продолжаете приписывать мне то, чего я не говорил. Где я предлагаю «силовое решение»?
Вот у нас с вами происходит то же самое, что у персонажей заметки.
Так что давайте я вам на живом примере и поясню, что я имею ввиду, поставив в этом споре точку. Вот она:.khim
14.11.2017 03:50+1И зачем вы продолжаете приписывать мне то, чего я не говорил. Где я предлагаю «силовое решение»?
Вот тут:Я в таких случаях рогом в ответ не упираюсь. Не нравится моя оценка — либо докажи мне, что я неправ, либо мёрджи без моего подтверждения, а потом сам объясняйся с руководством, почему ты так сделал. Я больше на это своё время тратить не буду. Sapienti sat.
В армии это называется «дедовщина». Ну и что, что зубной щёткой драить сортир долго и мезко? Я «дед», ты — «салага», иди, драй.
Так ваше поведение выглядит со стороны новичка.TheShock
14.11.2017 07:49+1В армии это называется «дедовщина». Ну и что, что зубной щёткой драить сортир долго и мезко? Я «дед», ты — «салага», иди, драй.
Нет, на дедовщину это совершенно непохоже. И мы не в армии.MacIn
14.11.2017 17:26+1Похоже до степени неразличимости.
mayorovp
14.11.2017 18:44Драить сортир зубной щеткой — это бессмысленное действие, придуманное специально для издевательства, которое "дед" сам уже давно не делает.
Писать проверку входных данных — это принцип защитного программирования, которому "дед" сам тоже следует.
MacIn
14.11.2017 19:07+1Вы хватаетесть за описание конкретного действия, тогда как это могло быть что-то иное, вполне осмысленное, схожесть не в этом.
mayorovp
14.11.2017 19:13Тем не менее, ключевая разница между дедовщиной и обучением в команде — именно в этом.
"Дед" заставляет делать то, чего он уже не делает сам. Наставник заставляет делать то что он делает сам.
khim
14.11.2017 19:24«Дед» заставляет делать то, чего он уже не делает сам. Наставник заставляет делать то что он делает сам.
Хорошо если так. Мой самый ужасный конфликт по поводу код-ревью случился тогда, когда я получил с десяток самых разнообразных замечаний. Когда я на каждое из них нашёл CL, залитый «наставником» в последний месяц и их нарушающий — то история начала развиваться почти как в статье. С нажимом «делай так, как я говорю», и «я так сделал, потому что нужно было сделать срочно, но ты пока делаешь не критичную по времени часть и дожен делать правильно» и прочее.
Дедовщина в чистом виде. Как Macin говорит: до степени неразличимости.mayorovp
14.11.2017 19:28В вашем случае — возможно, но мы же обсуждаем как надо делать код-ревью?
Так вот, есть случае когда лично я бы на код-ревью "уперся рогом" и не стал бы допускать такой код. И у самого меня подобных коммитов, которые я бы сам не пропустил — нет.
khim
14.11.2017 19:39И у самого меня подобных коммитов, которые я бы сам не пропустил — нет.
Если у вас нет, но вообще в проекте они есть, то это тоже вариант дедовщины, пусть и более мягкий: ты должен драить туалет зубной щёткой, потому что я сам так делаю, а то, что другим «дедам» можно пользоваться шваброй — так то тебя, «салага», не касается.
А вот если есть общее решение для всего проекта (особенно если оно зафиксировано официально) — то это повод выставить оценку F и не пускать такой код дальше, да, разумеется.
side2k
15.11.2017 14:55Если для вас требовать от нового члена команды соответствовать стандартам, в этой команде принятой — это дедовщина, то тут и обсуждать нечего, мы в разных плоскостях работаем.
netch80
14.11.2017 08:05А вот тут +100. Новичку часто очень сложно поверить в объективность некоторых правил, происхождение которых он не понимает. Особенно когда на это намешиваются вопросы стиля с местными тараканами, которые всегда есть и которые в основном потому, что так "исторически сложилось". Отделить после этого реально-объективные требования от традиций часто не может даже гуру-старожил, а новички или ломают себя психологически, просто привыкая к странному, или не врабатываются вообще.
side2k
16.11.2017 02:44Вы представьте на секунду, что вам аппендицит удаляет стажёр. Под надзором опытного хирурга.
Всё еще считаете, что «неопытному специалисту» можно что-то прощать?
Я могу согласиться с тем, что в ревью, которое более опытный разработчик делает на код менее опытного, он должен стараться нести конструктив, поясняя суть своих замечаний, предлагая варианты решений и т.п.
Но снижать планку требований? Да с каких это вдруг таких заслуг? Не можешь тянуть лямку такого проекта — работай над проектами попроще.
Это я говорю, как человек, поработавший более, чем с десятком «младших» коллег, но и неоднократно потрудившийся «ведомым».
На свой собственный код, кстати, я давно привык делать ревью ещё до пуша, в gitk-е. Последний у меня запускается автоматически скриптом при ежедневном начале работы над проектом.
Вы поинтересуйтесь, ради расширения кругозора, как оно в других профессиях происходит. Попробуйте, например, рулём покрутить, раскатывая по городу вместе с инструктором по вождению, сварщику с десятилетним стажем порассказывать, что нужно делать, чтобы электрод не залипал, или токарю шестого разряда разъяснить, какой резец выбрать.
Начинающие водители грузовиков, например, очень часто игнорируют советы старших товарищей снимать обручальное кольцо перед работой. А потом ломают пальцы или сдирают кожу. Иногда вместе с мясом.
Потому что опыт — он в любой профессии опыт. Он не является неким абсолютным мерилом всего, но всё равно крайне важен при оценке авторитетности мнения.
lpwaterhouse
14.11.2017 01:00Я не с той стороны баррикад, о которой вы подумали. Я на месте этой женщины. Но в отличие от нее я прекрасно понимаю, что более опытный человек не просто так советует мне делать так, а не иначе. И уж тем более он это делает не для того, чтобы потешить свое ЧСВ, а потому, что и ему самому за результат работы отвечать. Да, я конечно могу выразить свою точку зрения, объяснить что я сделал так потому-то и потому-то, но настаивать и вот так вот упираться я не вижу смысла. И вот такой вот расклад я считаю правильным порядком вещей, а не пытаться три недели восстанавливать свое оскорбленное чувство прекрасного.
side2k
14.11.2017 01:06Я говорю о том же самом — надо думать и работать, а не разводить демагогию в комментах к пулл-реквестам.
khim
14.11.2017 03:26Но в отличие от нее я прекрасно понимаю, что более опытный человек не просто так советует мне делать так, а не иначе.
Значит вы ещё просто не доросли до понимания того, что «более опытный человек» — тоже не Бог. Не далее как неделю назад я, к примеру, выпилил тот код, который меня пытался заставить создать «более опытный коллега», что я отказался делать категорически.
В конце-концов точно также, как в статье — привлекли третьего, в качестве «третейского судьи», было принято решение, что «более опытный товарищ» всё сделает сам, чем он, разумеется был страшно недоволен… но я прекрасно понимал, что делал: об этот код потом буквально вся команда «спотыкались» два года и, когда «опытный товарищ» свалил радостно изжила. А что было бы, если бы сдался? А очень просто: все шишки падали бы не на «опытного товарища», а на меня. Он же, как вы выразились, «не просто так советует» — а что в результате грабли получились… ну так это я накосячил, без опыта-то…TheShock
14.11.2017 07:30+2Обычно в таких случаях необходима аргументация с каждой стороны и решение команды (иногда, в конфликтных ситуациях демократия — лучший выбор, ведь потом ВСЕЙ команде поддерживать этот).
А детский сад из разряда «ты старая пердящая звезда, а я тут яркая молодая звездочка, потому нибуду делать, как ты сказал, сам так делай» — это крайне неконструктивно.
Если команда не может договориться — это или саботаж, или детский сад.MacIn
14.11.2017 17:30+1А делать всей команде нечего, как в третейского судью играть каждый день по куче вопросов.
khim
14.11.2017 17:38Обычно в таких случаях необходима аргументация с каждой стороны и решение команды
Аргументация — это хорошо, если у вас есть агрументы — то тут можно о чём-то говорить. К сожалению в большинстве «спорных» случаев агрументация сводится к ссылкам на высказывания «авторитетов» — а так можно спорить до бесконечности.
Больше всего меня убивает аргументация в стиле «а что если мы наш паром, перевозящий почту через Ла-Манш превратим в ракету по доставке писем на Марс?» — вот когда встанет задача по доставке писем на Марс, тогда и будем думать, а пока у нас в багтрекере такой задачи нет и приоритета у неё тоже нет… забудьте. Всего учесть невозможно и лучше простой код хорошо решающий задачи, которые у нас уже есть, чем сложный, который плохо решает и задачи, которые у нас уже есть и что-то, что есть только в больном воображении ревьюера…Neikist
14.11.2017 17:43Проблема в случае девушки из статьи как раз не в том что она сделала слишком просто, когда автор хотел сложно, гибко и т.п. Позволю себе процитировать: «неопределённое поведение кода для искажённых входных данных или то, что структуры потока управления в одной из её функций находились на шестом уровне вложенности» — это никак не желание автором чрезмерной гибкости и усложнения архитектуры. Скорее наоборот, автор просто хотел четкости, ясности и простоты в коде.
khim
14.11.2017 18:11Ну, «обработка искажённых данных» — это вообще «высший пилотаж». В долгосрочной перспективе от этого скорее вред, но если у вас данные уже есть — то без того, чтобы их обрабатывать вам не обойтись.
mayorovp
14.11.2017 18:51Проблема авторитетов решается очень просто. Начальник тыкает пальцем в небо и выбирает один из вариантов. Более демократичные команды выбирают вариант общим голосованием.
Дальше уже в ревью ссылаются не на авторитетов — а на однажды принятое на проекте решение.
khim
14.11.2017 19:11Дальше уже в ревью ссылаются не на авторитетов — а на однажды принятое на проекте решение.
Это если есть документ, где это решение «принято». А если его нет — то у разных людей будут разные трактовки. Вплоть до полностью противоположных.mayorovp
14.11.2017 19:20В первом случае начальник всегда может тыкнуть пальцем в ту же сторону повторно. Во втором случае — да, нужно обязательно принятые решения где-то документировать.
khim
14.11.2017 19:34И, тем не менее, часто это не делается. Высшую степерь опофигения, которую я наблюдал была «у нас так принято, но в style guide мы это не вписываем, потому что он и так большой».
Пришлось то ли на один уровнь, то ли на два подняться, чтобы принцип «если какое-либо решение недостаточно важно для того, чтобы быть внесённым в style guide, то оно недостаточно важно для того, чтобы отказывать принятии CL'я» было официально зафиксировано.
Кстати и style guide был изменён. Раньше говорилось, что можно писатьint* x;илиint *x;, но неint*x;илиint * x;как в Goggle Style Guide, теперь говорится, что в каждом файле должен использоваться только один вариант из двух. Это — меня устраивает, мне, в общем-то, пофигу куда пробел ставить, главное, чтобы в code review на это время не тратить…
mayorovp
14.11.2017 09:17Если бы автор и правда пытался получить результат — он бы не переписывался 3 недели с непонятным результатом.
netch80
14.11.2017 07:50Если бы было только две альтернативы — не принимать неадекватный код, соблюдя чистоту проекта, или принимать, подложив свинью на будущее — да, такая постановка имеет смысл.
Но автор рассказал про случай третьего выхода из вроде бы дилеммы. И это должно быть основным мотивом обсуждения статьи.
johnfound
13.11.2017 21:14такие как неопределённое поведение кода для искажённых входных данных
Правильно ли понял, что в конце, неопределенное поведение так и не было исправлено?
fillpackart
13.11.2017 22:05Уже не первый раз такое встречаю. Некий разработчик берёт, и пишет о том, как надо вести свою жизнедеятельность исходя из посыла… как угодить бизнесу. Постойте, но ведь мы стали разработчиками не ради бизнеса. Какая мне к чёрту разница, что правильно для бизнеса, если ради этого бизнеса я должен мириться с плохим кодом? Почему для многих успех бизнеса стал важней, чем код?
yizraor
13.11.2017 22:10Наверно, я чего-то не понимаю…
Но музыку заказывает тот кто платит, т.е. работодатель — а ему свой бизнес важнее твоего кода.
А хочешь, чтобы твой код был важнее чьего-то бизнеса: ну, можно покодить бесплатно для себя, да и опенсорс-проектов навалом )fillpackart
13.11.2017 23:35Очевидно, да. Вопрос только в том, зачем конкретно мне думать о том, чего хочет работодатель, и ещё и других этому учить? Если я писатель, должен ли я писать книгу так, чтоб она нравилась издательству? Я так не думаю. Имеет смысл написать так первую книгу. Получить свой контракт. А вот потом писать так, как хочу я, ведь какой смысл быть писателем, если ты пишешь строго так, как хочет издатель?
khim
14.11.2017 00:22+1Если я писатель, должен ли я писать книгу так, чтоб она нравилась издательству?
Да, разумеется. Вы Пушкина знаете? Почитайте «разговор книготорговца с поэтом». Там всё очень поэтичненько описано.
А вот потом писать так, как хочу я, ведь какой смысл быть писателем, если ты пишешь строго так, как хочет издатель?
Ну вы же хотите, чтобы вашу книгу прочитали? Значит вы заключите контракт. А если потом захотите не оказаться на улице из-за выплаты неустойки — то вам придётся его выполнять. А если уж совсе плохо будет — издатель может и кому-нибудь другому поручить за вас продолжение написать.
Как раз первую книгу — вы можете писать как угодно. А уже последующие — будьте добры придерживаться заранее оговоренных форматов и сроков. Или готовьтесь к серьёзному разговору с издательством…MockBeard
14.11.2017 11:15чудный стих, как-то прошел мимо меня «Не продается вдохновенье, Но можно рукопись продать» :)
gtbear
13.11.2017 22:38+1Сам код никому не нужен, нужно решение задач бизнеса. Если разработчик этого не хочет признавать, то наверное ему место только в опенсорс/собственных проектах. Хороший код нужен бизнесу только тогда, когда его написание и поддержка обходится дешевле говнокода, а это наступает далеко не сразу.
avallac
13.11.2017 23:05О, господа тролли подтянулись. А слабо, завтра подойти к своему гендиру и сказать это в лицо?)
fillpackart
14.11.2017 00:36+1О, господа тролли подтянулись.
О, вот и роботы подтянулись, упомянутые в первой статье.
У вас есть подходящий ярлычок для каждого, кто посмел думать иначе?avallac
14.11.2017 00:47Да думайте, на здоровье иначе, я за свободу самовыражения, но только не надо лицемерить. Мне вот не будет стыдно, если мое начальство ознакомиться с моими комментариями.
Lord_Ahriman
14.11.2017 12:41Вам никто не мешает работать над open source проектами для души с таким отношением. Работодатель же, платя вам зарплату, покупает ваши услуги. Разумеется, эти услуги должны удовлетворять работодателя, ваш код, написанный в рамках этого договора, должен отвечать целям бизнеса, собственно, за что вам и платят. Не думаю, что где-то платят просто за абстрактный красивый и правильный код. Платят за то, что работает с минимальными затратами средств и времени и соответствует целям работодателя.
PS: я сталкивался вот с таким отношением а-ля «Я художник, я так вижу» от одного программиста в одном проекте. Итогом стал срыв сроков и прочие не менее неприятные вещи.
vitvad
14.11.2017 02:17в продолжение к статье возник вопрос:
— дядя Боб смержил код к которому есть достаточно притензий, мы все такие на белом коне, ворвались и выровняли ситуацию с назревающим конфликтом. Но как выровнять ситуацию с кодом? В рамках каких задач заставить этого человека отрефакторить только что смерженый код?
— писать //TODO в коде? Нет ничего более постоянного чем временное — тудушки будут только множиться в коде и контекст задачи уже потеряется к моменту рефакторинга
— оставить как есть? Тогда зачем код ревью вобще проводить
мне кажется в подобной ситуации есть только один вариант решения:
«у меня очень много коментариев, давай устроим парное программирование и улучшим этот код вместе»khim
14.11.2017 03:40Но как выровнять ситуацию с кодом? В рамках каких задач заставить этого человека отрефакторить только что смерженый код?
А зачем его заставлять рефакторить только что смерженный код? Пусть рефакторинг тот код, который он (или она) править будет!
Ведь речь не идёт о том, что код настолько ужасен, что непонятно — будет он работать или нет. А просто с ним что-то «не совсем так»: константам не даны имена, копи-паста много или что-нибудь подобное.
Ну и? Убиваться-то поэтому поводу зачем? Вотгда придёт пора менять что-то в этом коде — тогда и отрефакторите.
Повезло и этот код никому трогать не нужно? Ну и отлично — пусть остаётся грязным. Раз его никто не трогает, то он никому и не мешает.
«у меня очень много коментариев, давай устроим парное программирование и улучшим этот код вместе»
Это обычно имеет смысл тогда, когда код совсем никуда не годится.TheShock
14.11.2017 07:45+2Вотгда придёт пора менять что-то в этом коде — тогда и отрефакторите.
Так вот — прям сейчас меняем. Вот пул-реквест открыт, еще даже до тестеров не дошло. Мы помним контекст, причину решений, у нас даже есть комментарии, что улучшить! В следующий раз всё это необходимо будет нарабатывать с нуля, то есть вы предлагаете просто выкинуть на ветер ресурсы, деньги на которые бизнес уже потратил. И все зачем? Потому что у кого-то корона мешает признать, что его код имеет недостатки?
Более того, потом, когда он с ним работать в следующий раз будет — получим отсылку к пункту статьи «Уважайте область ревью» — не моя область, я это рефакторить не собираюсь.
Плохое отношение к качеству сейчас — еще хуже отношение к качеству потом.
константам не даны имена, копи-паста много или что-нибудь подобное.
Ага, вот так сейчас: «а, хрен с ним, пока не печет». А через пол-года:
— хнык-хнык, было 50 багов, закрыл 20, теперь 100 багов.
— хнык-хнык, да, я фиксил этот баг, но оказывается, что мы его скопипастили 18 раз и каждый из участков немного менялся, потому надо пофиксить его 18 раз, а потом отдел тестирования должен будет внимательно проверить все 18 частей системы заново.
— хнык-хнык, чтобы этот модуль заработал, надо переписать его с нуля
— хнык-хнык, мы не сможем сделать это в срок, потому что программист, которому поручили работать с этим кодом сперва неделю плакал, а потом вышел в окно офиса на 7 этаже
— давай, досвидания, пущай ваши джуны разгребают этот говнокодище, который оставил ни в коем случае не я, с наплевательским отношением к качеству, а папєрєдники.
И всё это вы называете «я думаю про бизнес!»khim
14.11.2017 16:58Более того, потом, когда он с ним работать в следующий раз будет — получим отсылку к пункту статьи «Уважайте область ревью» — не моя область, я это рефакторить не собираюсь.
Читаем внимательно: «если изменение относится к изменяемому коду, хотя и не входит в него» — то оно «в области ревью».
— хнык-хнык, было 50 багов, закрыл 20, теперь 100 багов.
Это нормально. Если у вас ситуация не такова — то это просто обозначает, что ваш проект никому, по большому счёту, не нужен и новые баги заводить просто некому.
— хнык-хнык, да, я фиксил этот баг, но оказывается, что мы его скопипастили 18 раз и каждый из участков немного менялся, потому надо пофиксить его 18 раз, а потом отдел тестирования должен будет внимательно проверить все 18 частей системы заново.
А почему у вас 18 копи-пастов кода? Почему вы не провели рефакторинг после появления 4й или 5й копии? Если все 18 родились одновременно, то оценка у такого изменения «F» и заливать его не нужно было — об этом в статье тоже есть.
— хнык-хнык, чтобы этот модуль заработал, надо переписать его с нуля
Даже если вы вбухаете в разработку в 10 раз больше времени, чем нужно — вы от этого не застрахованы. Потому что, например, «этот модуль» мог реализовывать какую-то функциональность, доки на которую у вас были просто неправильными…
Вы исходите из неправильно посыла: что вы можете взять — и написать правильный, красивый код. А это — невозможно. Сегодня — он красивый, а завтра — уже не очень, послезавтра — вообще безобразный. Потому что вы меняетесь, требования к проекту меняются, инструменты меняются, в конце-концов.
Я не так давно заменил кучу макросов на вариативный шаблоны в одном из наших модулей. Он стал чище и проще для понимания — но три года назад (когда эти макросы создавались) так его было написать нельзя, так как поддержки C++11 на одной из платформ, где мы собирались не было — и это приходилось учитывать.mayorovp
14.11.2017 18:56А почему у вас 18 копи-пастов кода? Почему вы не провели рефакторинг после появления 4й или 5й копии? Если все 18 родились одновременно, то оценка у такого изменения «F» и заливать его не нужно было — об этом в статье тоже есть.
Потому что заказчик проснулся и вывалил кучу новых требований одновременно добавив финансирования. Пришлось часть задач отдавать индусам, а они любят наследование в терминах УОП.
Вы исходите из неправильно посыла: что вы можете взять — и написать правильный, красивый код. А это — невозможно. Сегодня — он красивый, а завтра — уже не очень, послезавтра — вообще безобразный. Потому что вы меняетесь, требования к проекту меняются, инструменты меняются, в конце-концов.
Но это же не повод писать код который уже сейчас является заведомо ужасным.
khim
14.11.2017 19:13Но это же не повод писать код который уже сейчас является заведомо ужасным.
Цитирую статью:Оценка F предназначена для кода, который либо функционально некорректен, либо настолько запутан, что вы не можете быть уверены в его корректности. Единственная причина задержать одобрение — это если код остаётся на уровне F после нескольких раундов ревью.
Если код — таки «ужасен» и либо функционально некорректен, либо настолько запутан, что вы даже не можете понять — корректен ли он, то если заливать не нужно.
Вот дальше — возможны варианты.mayorovp
14.11.2017 19:24Судя по описанию ситуации, код остался именно на этом уровне. Но автор поста, видимо, считает именование переменных более значимым чем корректность работы алгоритма.
Как и ожидалось от человека способного допустить трехнедельный простой по задаче на пустом месте.
khim
14.11.2017 19:36Судя по описанию ситуации, код остался именно на этом уровне.
С чего вы взяли? Код принимает данные от другого модуля не валидируя их повторно. Само по себе это не криминал — главное понимать, насколько высока вероятность того, что другой модуль вызоват ваш с неправильными данными.
netch80
14.11.2017 08:13+2Повезло и этот код никому трогать не нужно? Ну и отлично — пусть остаётся грязным. Раз его никто не трогает, то он никому и не мешает.
На это есть известный довод, что на 1 написание кода приходится N (обычно говорят про 10) чтений кода, и ждать рефакторинга, чтобы его исправить — означает попусту тратить будущее время коллег.
А ещё есть "теория разбитых окон", которая говорит, что если появляется несколько мест такого безобразия, то скоро весь проект станет таким.
В соседнем проекте я такое наблюдаю — народ уже давно плюнул на идею что-то исправить и пишет такое, что волосы дыбом лезут.
khim
14.11.2017 17:06В соседнем проекте я такое наблюдаю — народ уже давно плюнул на идею что-то исправить и пишет такое, что волосы дыбом лезут.
Если народ «давно плюнул на идею что-то исправить» — то это значит, что никакого рефакторинга и вычистки кода при добавлении новых вещей не было, извините.
А ещё есть «теория разбитых окон», которая говорит, что если появляется несколько мест такого безобразия, то скоро весь проект станет таким.
Теория разбитых окон — это немного о другом. Если вы что-то допустите один раз (ну, к примеру, доступ к защищённому полю через reflection), то допустите и второй, а с третьего раза — это станет «нормой жизни». Это правда. Но вряд ли все 59 замечаний в «худшем код-ревью» автора были таковы. Да и довод «в нашем проекте мы X не делаем никогда» действует даже на новичков (особенно если это явно упомянуто в style guide). А вот моменты, про которое сходу неочевидны — тут ситуация другая. И в статье — речь именно об этом.mayorovp
14.11.2017 18:59Процитирую еще раз:
Через несколько дней Мэллори прислала новый список изменений и ответ на мои заметки. Она исправила самые простые ошибки: опечатки, названия переменных и т.д. Но отказалась решать проблемы высокого уровня, такие как неопределённое поведение кода для искажённых входных данных или то, что структуры потока управления в одной из её функций находились на шестом уровне вложенности. Вместо этого она печально пояснила, что исправление этих проблем не стоит времени разработки.
Входные данные нужно проверять на корректность пока они еще входные а не хранимые. Не знаю какие там доводы приводились протагонистом, но они явно не были услышаны.
khim
14.11.2017 19:19Входные данные нужно проверять на корректность пока они еще входные а не хранимые.
Зависит от того, насколько высока вероятность того, что они некорректны.
Я так понимаю тут речь шла не о входных данных в программу (тут вопросов нет — мало ли что пользователь пришлёт?), а о входных данных в модуль. Вот тут — уже вполне может оказаться что принцип GIGO приведёт в итоге к более надёжному коду — просто потому, что он будет проще.
n7nexus
14.11.2017 14:34В нашей фирме нет код ревью, как результат коллеги-разработчики тратят много времени на то, чтобы вникнуть в некоторые изменения кода. На понимание того, как работает измененный код порой уходит день, иногда больше.
Как-то раз от меня проскакивало предложение ввести код ревью, но оно не было воспринято всерьез и прозвучали слова о том, что это «замедляет разработку» и «у каждого свое видение».
Как можно убедить вышестоящие должности ввести код ревью и правильно аргументировать его важность? Возможно ли постепенное введение код ревью, чтобы не стопорить сразу работу на n-ное количество времени и ревьюить все, а как-то подготовить людей к такому?5mbytes
14.11.2017 15:33Мой опыт говорит, что если в фирме недостаточный уровень качества разработки, но бизнес как-то работает и зарабатывает, почти невозможно убедить сделать какие-то улучшения. И это как бы логично, зачем тратить время, если и так работает.
n7nexus
14.11.2017 16:14Когда ведется интенсивная разработка и/или приближается дедлайн, код ревью действительно может восприниматься как пустая трата времени.
Но в какой-то момент можно столкнуться с изъянами проектирования и разработки. И, как известно, чем дольше существуют подобные изъяны, тем больше на них завязано функционала и тем сложнее потом все переделывать (частенько даже просто реализовывать новый функционал).
Бизнесу, конечно, в краткосрочной перспективе важнее выпустить работоспособный продукт. Но и о долгосрочной перспективе он думать должен.5mbytes
14.11.2017 17:26Если в вашей фирме нет код-ревью и низкое качество кода, значит это устраивает и босса, и большинство программистов. Так же это вероятно значит, что изначально именно программисты начали разработку в таком стиле, боссу все понравилось, и зачем вдруг сейчас что-то менять, ему не очень ясно.
Раз вы работаете в этой фирме, значит вас в целом тоже все устраивает.
Если вы выросли и вас не устраивает, вы можете или попробовать стать архитектором этой системы (но нет гарантии что вам дадут применить ваше новое положение на практике), или найти новое место работы.
khim
14.11.2017 17:13Возможно ли постепенное введение код ревью, чтобы не стопорить сразу работу на n-ное количество времени и ревьюить все, а как-то подготовить людей к такому?
Начните с автоматизации. clang-format, автоматический запуск тестов и «закрытие дерева», если они падают. Вот это вот всё. А к «человеческим» код-ревью подойдёте потом.
Людям психологически гораздо комфортнее, когда их код «заворачивает» «бездушная мащина» по формальным признакам, чем когда это делает человек.
А когда все почувствуют что он этого они получают ощутимую прибыль (реже приходится авралы устраивать, меньше приходится работать на выходных в авральном режиме) — уже можно и о код-ревью завсети разговор. В первой части об этом упоминалось.
5mbytes
14.11.2017 17:33Как убедить: рассказать о плюсах лучшего кода. Ведь код-ревью вводится чтобы код стал лучше. Главное не зациклиться на самом коде, а вместо этого рассказать, что это даст бизнесу. Что увеличится скорость разработки (а не уменьшится), что в хороший код легче вносить изменения, что в такую фирму захотят прийти работать другие хорошие программисты, а не только говнокодеры.
Если такие аргументы не работают, значит нечего там делать.
hippoage
15.11.2017 18:39+1У меня сложилась такая схема:
0. Реформаторы кода и линтеры запускаются до подачи кода на code review
1. Продуктовые вопросы (как это должно работать в таком-то случае? как это должно работать совместно с такой-то функцией)
2. Высокоуровневый дизайн (структуры данных, библиотеки/модули/пакеты/классы/публичные методы)
3. Качество кодирования (приватные методы, код внутри методов, тесты)
Если есть вопрос по определенному пункту, то ниже не спускаемся, т.к. может оказаться, что соответствующий код будет переделан.
В примере из статьи не было нулевого пункта. Сразу были комментарии по пунктам 0, 1?, 2, 3. Это дало возможность разработчику исправить только 0 и 3, а по принципиальным для проектирования моментам ничего не сделать. Если бы были замечания только по пункту 2, то их бы не получилось так просто игнорировать.
Основная же идея о том, что можно пропускать плохой код ради обучения специалистов не понравилась. Если юниор (вообще или в проекте), то можно перед написанием кода делать верхнеуровневое проектирование (исключит пункты 1 и 2). А вот к коду внутри методов можно действительно временно снижать планку, если приходится работать с юниорами в программировании (регуляр уже должен корректно писать код на уровне методов).
MooNDeaR
Если коротко — не будь упрямым мудаком) Думаю этого достаточно, чтобы описать всю статью.
Poehali
Упрямый мудак — слишком абстрактное понятие, а здесь как раз уточняется, как определить свою вхожесть в него.
danfe
Вовсе нет. Я сам часто руководствуюсь правилом «не будь м*ком», но применительно к code review автор делает очень верные выводы. Их, конечно, теоретически можно вывести, что называется, из общих соображений, но лично мне это понимание пришло лишь с многолетним опытом.