×Закрыть

Негативный опыт с code review

Ребята, я готовлю доклад на конференцию о том, как не надо проводить код-ревью и как негативные моменты этого процесса влияют на атмосферу внутри команды.

Ответьте, пожалуйста, на вопросы небольшого опроса, который я подготовил для этой темы — это не займет больше 3 минут: goo.gl/Uaax25

PS: у кого есть твитер, я буду признателен за ретвит ссылки на этот опрос: twitter.com/...status/530735105212710912...
Mehr anzeigen

LinkedIn
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter

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

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

Что за херня, переписать заново
Посмотри на форумы, как народ в массе своей реагирует на подобные посты. Процентов 60% возмущается — они не адекватные?
Посмотри на форумы, как народ в массе своей реагирует на подобные посты.

О каких именно форумах речь? Прошу парочку реальных примеров для уточнения контекста.

Неважно, посмотри на этот. А так любой.
Вот я тебе напишу:

«Что за херня, переписать заново»
Как среагируешь?
Неважно, посмотри на этот. А так любой.

Не смог найти примера. Может, потому, что не хожу по явным помойкам. Но на явных помойках вряд ли будет существенный код:)

Как среагируешь?

Попрошу подробного обоснования. До получения такого обоснования — проигнорирую.

Причем тут код. Ты можешь быть миллиен раз прав, но фраза

«Что за херня, переписать заново»
очень негативно воспринимается подавляюшим большинсвом народа.
очень негативно воспринимается подавляюшим большинсвом народа.

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

А должны быть исключительно культурные, а не посередине.

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

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

Вот видишь уже

Знаете, мне не нравится такой стиль дискуссии с Вашей стороны.
Хотя я писал пока исключительно культурно (в инете я всегда использую «ты» — принято в нем так) и даже намеков на оскорбление не давал. Ты просто придумал про стиль, основываясь всего-лишь на том, что кто-то посмел уточнить твою точку зрения.
И твоя реакция даже на эту переписку показывает насколько проблематично применения «кодревью».
Ты просто придумал про стиль, основываясь всего-лишь на том, что кто-то посмел уточнить твою точку зрения.

Нет, это говорит о Вашей, а не о моей реакции.

А какая у меня реакция??? Я всего-лишь уточнял конкретный момент:

Конечно, в зависимости от местности это может быть от «У меня есть некоторые подозрения, что это место не до конца проработано...» до «Что за херня, переписать заново», но сколь-нибудь адекватный автор не воспримет это как оскорбление.

То, как именно Вы это «уточняли».

Попрошу подробного обоснования. До получения такого обоснования — проигнорирую.

Ты проигнорируеш, а пять других — обидятся.

Да вот уже

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

Тебе же сказали — «в зависимости от местности». Можно даже уточнить — в зависимости от того, «как тут принято».

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

Точно также существуют ранимые личности, которым такая фраза будет прямым уязвлением самомнения. И это тоже стоит учитывать.

«в зависимости от местности»
А что есть в местность в мире, где нормой является:
Что за херня, переписать заново
?
Или просто есть какой-то небольшой процент людей, который привык к такому общению и оно ему нравиться?
И за примером далеко ходить не надо: Valentin Nechayev , который эту фразу здесь предложил обиделся уже на то, что кто посмел уточнить его точку зрения, по сути.

А я разве говорю, что все люди толстокожие? В конце концов, ты знаешь свою команду, и знаешь кому что можно говорить. И да, довести человека до увольнения/нервного срыва вполне можно и исключительно вежливо.

Ключевая фраза — «в рамках принятой корректности». Остальное-то указано лишь как пример. Зачем придираться к словам?

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

А гнуть матом на незнакомого человека, не только некорректно, но и вредно для здоровья :)

P.S. Нет, правда, у тебя комментарии пишут в стиле «Уважаемый сударь, ваша реализация чудесна! Однако, мне показалось, что количество условий в этом методе слегка чрезмерно. Возможно, для продуктивной работы в дальнейшем, вы соблаговолите изменить этот код?»

то все тебе известно
Неизвестно, это в высшей степени глупая самонадеянность.
"Уважаемый сударь
Да почти в таком виде, только помимо грубости
для продуктивной работы в дальнейшем
желательно четко описать, что именно ты поправил бы и почему.
Да почти в таком виде
Забавно, но я бы такое воспринял как насмешку.
желательно четко описать, что именно ты поправил бы и почему
Т.е. ты предлагаешь унижать разраба тем, что он не в состоянии придумать сам хорошее решение?

Нет. Ты просто предлагаешь свое видение.

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

И кстати. «Сударь» в Украине в настоящей время употребимо?

Ты просто предлагаешь свое видение.
И тем самым намекаешь, что ты лучше его.
Кстати, шутки-шутками, но уперание рогами из-за «моя идея лучше, потому что она моя» — я реально видел.

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

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

Хотя один раз был случай, когда не выдержал и нагрубил в стиле «Шо за херня» — чел riff wav разбирал арифметическими сдвигами, вместо того, чтобы прочитать тот же MSDN. Да, я был неправ, но тут мы все с эмоциями и можем нагрубить.

чтобы бить на эмоции — иногда и это надо, но очень очень редко.

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

Вообще-то «это херня» можно сказать не грубя. Плохо. Не подходит. Надо переделать.

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

P.S. И да, не важно какими словами это сказано, но «Это херня, все переделай» — в любом случае крайне плохая рецензия.

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

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

Такое согласование может сильно затянуться :)

Демократия здесь не работает, есть смысл обсудить на самом высоком уровне техлид и несколько senior-ов и выдать уже готовую процедуру code-review, которой все обязаны следовать, в идеале внедрить в сам билд проекта. Малейшая слабина в принятии решения и все рушится. Т.е. правила формально есть, но фактически на них забивают, при этом осадок у всех остался.

Причем тут демократия (в смысле анархии). 1-3 сеньора подготавливают документ, рассылают остальным, те пишут свои замечания, затем большое совещание и утверждение (вплоть до того, что голосованием).
А иначе, половина будет вредить или передерется. Климату в коллективе кранты, все всех будут посылать подальше на вопросы.
В итоге придется сменить половину программистов (новым уже можно просто выложить местные требования).

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

Вы пишете на собственном опыте?

Да. Правда с те времена кодревью еще не существовало. Но правила написания кода уже пытались спускать сверху.

на самом высоком уровне техлид и несколько senior-ов и выдать уже готовую процедуру code-review, которой все обязаны следовать, в идеале внедрить в сам билд проекта.
Может в идеале они сами на самом высоком уровне начинают делать код-ревью согласно принятой процедуре и приучают этим остальных, впоследствии расширяя круг тех, кто делает ревью?..

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

Поделись результатами и анализом, когда соберешь данные. Если об этом еще не просили.

Можно будет послушать на XP Days меньше, чем через месяц: xpdays.com.ua/...ew-antipatterns

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

Код-ревью — однозначно полезная и нужная вещь. Но ее нужно уметь правильно применять. Мне кажется обосновано применение 1-2 опытных ревьювера и на фичи или на джунов.

Сталкивался с ситуацией (не в EPAM), когда вся команда из 5-7-9 человек должна была ревьювить любой чих, даже изменение в проперте или на JSP-ке + сюда добавлялся напряг добавить на каждый коммит +4 теста. Понятное дело что ценность инструмента резко снизилась со всеми выходящими последствиями.

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

Кратко — не лучше. Тем же миддлам нужна передача опыта, а джунам и подавно. К тому же важно чтобы senior-ы ревьювили код друг друга и были общие командные подходы, а не огораживание. Иначе получится как здесь:
«Сидоров предлагает крупноблочную архитектуру. Петрович настаивает, что все надо строить по старинке, из кирпича, не по-ламерски.»......."Просто 12 этажей Сидорова на 4 метра выше и на 5 метров шире, чем 12 этажей Петровича. Выяснилось, что они строили из разных панелей."

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

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

Во-первых senior — более редкий, капризный и ограниченный ресурс. Вы попробуйте за полгода набрать хотя бы 20-30 senior-ов при той же зарплатной политике, если не верите, спросите у рекрутеров.
Во-вторых это глупо стрелять из пушки по воробьям. В армии например есть и обычные и 12,7 пулеметы и гранатометы и 40/100/120/152мм и корабельная 330мм артиллерия, каждая под свою задачу. Для многих задач senior-ы не нужны, достаточно миддлов или джунов.

Тем же миддлам нужна передача опыта
"обмен опытом" звучит красивей)

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

От вам і негативний досвід code review, коли замість того щоб оцінити ваш внесок,
вам зробили такі прості зауваження:

що за форма з країнами — «америка, німеччина, і два „інших“»??
А каким боком Software Testing Engineer к code review?
Reviewed. Try to fix ASAP.

А каким боком Software Testing Engineer к code review?

Тестировщики также пишут код и участвуют в ревь кода, написанного другими тестировщиками

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

а что можно сделать ревью не на поровшись на проблемы если у вас работают неадекватные упыри?

А зачем вы нанимаете на работу неадекватных упырей?;))) — может в этом проблема?..

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

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

мне это не надо доказывать, докажите менеджерам решившим что самый упоротый аспергер это лучший кандидат делать всем ревью ))))

менеджерам решившим что самый упоротый аспергер это лучший кандидат делать всем ревью ))))
Цитата
А зачем вы нанимаете на работу неадекватных упырей?;))) — может в этом проблема?..
касается менеджеров в первую очередь. Вот зачем их таких нанимали?)
Аспергеры бывают, это не самое страшное. Иногда им лучше просто не мешать — и они сами наваяют вполне.
кое кто говорит в гугле например дико много политики на этом замешано. и ревью оч популярный тул для выяснения отношений
 — совершенно не соответствует моему опыту, механизм code review в Google работает очень хорошо

я слышал и людей очень не довольных
видно кому как с командой свезло

Возможно. Мне свезло с командой трижды.

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

Сергей, ну вы умный, придумайте что-нибудь чтобы сохранить всем адекватность)

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

разве адекватная девушка станет интересоваться высказываниями Саши Грей?;)

А можна з цього місця детальніше? Про які таки висловлювання йде мова? :) Щось накшталт cs317327.userapi.com/...hVyaIHWXZmg.jpg ?

Не ображайте дівчину, насправді там було щось про фемінизм
www.youtube.com/...h?v=42EIKQP4Mto

Кстати комент выше — прямая иллюстрация насчет неадекватности.
Благодарю.

Вы кажется не заметили, что я отделяю человека от определенного периода ее работы. Я не интересуюсь Сашей Грей. Но вот то, что делает Марина Аня, достойно некоторого внимания. Ее же образ в порно мне неинтересен абсолютно (так же как и ее видео — в котором ее слов практически и нет, благо ее рот занят), но это вопрос вкуса.
Теперь насчет adult video. Адекватные люди вполне интересуются таковым — вопрос в том, что многим они перестает быть интересным. Разве что изредка сходить парой в хороший кинотеатр на хорошее кино. Жовтень к примеру очень хороший был — и Киев (насколько могу судить по отрывочным впесатлениям). Подростки, интересующиеся таким видео, вполне понятны. Но взрослые люди ... разное бывает.
Адекватность же вполне базовая вещь. Она вполне может быть у каждого и ее можно развивать.
Вопрос в том что во многих профессиях полно разных факторов. И где-то прибыль компании важнее чем состояние и целостное развитие работников.

Тю, теж мені, проблема... Замініть пєчєньки ноотропами.

Мне хватает чтения Доу и настольных игр;)

шо-то мне шепчет в ухо, что при наличие неадекватных упырей кодревью не будет ни единственной, ни самой серьезной проблемой

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

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

Разные у людей проблемы, я так понял, что ТС читает for beginners.

Ну не знаю, мне приходилось сталкиваться с тем, что ревью может вызывать неприятные чувства у людей. Это может быть связано совсем не с их профессионализмом, или его отсутствием. У человека может быть плохое настроение, черная полоса, а вы и не в курсе. Всяко может быть. При общении в команде оценка чего угодно может привести к тому, что человек вместо рационального взгляда на критику/замечания может проявить (или ощутить) эмоциональную реакцию.

Наверное в этом случае проблема не с ревью?..

Естественно не с ревью. А разве опрос касается непосредственно проблем и техник codereview? Мне казалось тут как раз хотят выяснить человеческий фактор.

Опрос непонятно чего касается. И мало что выясняет конкретного с человеческим фактором.

по опросу понятно, к чему вы клоните. Но это детский сад какой-то а не код ревью, если выходит «да что он ко мне прицепился со своими замечаниями»

Это обычная реакция человека, когда его код ревьювят в первый раз;)

Мне сложно понять суть некоторых вопросов, не ясно как понимать «Do you feel that the code reviews you do for other people ...» (Кажется ли вам что проводимое вами ревью кода других людей ...?). Я ожидал бы более четкого вопроса. Например «Ваши ревью ...?» (Do your reviews ...? Are your reviews ...?)

Have you had your code reviewed by another person? => вопрос частично дублирует «Which role have you had in code review activities?» с первого шага :)

Да, спасибо, за обратную связь.

Некоторые вопросы действительно длинные, и их надо переформулировать.

видаляйте одразу :)))
що за форма з країнами — «америка, німеччина, і два „інших“»??
україну в студію :))

There is an “other” item there ))))

в таких випадках пропоную опитувальник постити на other сайтах, а не на доу :)))

А він і запощений на Google Docs )

Подписаться на комментарии