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

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

Этот коллега — тот, кто обучал меня изначально. Он работает в компании более 10 лет, а я присоединился к ней год назад, сразу после университета. У него больше лет опыта разработчика, чем у меня лет. Несмотря на это, он по-прежнему мой ровесник, поскольку мы оба являемся разработчиками среднего уровня (уже не младшими, но и не старшими) разработчиками.

Этот конкретный запрос на изменение включает в себя старую сложную функцию. Он плохо написан в соответствии с любым стандартом кода, но работает. Мне потребовалось несколько часов, чтобы даже найти то, что мне нужно было изменить. Клиент хотел другой порядок оценки для определения начального значения поля (например, проверить client.address перед должником.address, а не наоборот).

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

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

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

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

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

Как исправить эту ситуацию?

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

Ответы (9)

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

  1. Непоследовательный отступ в ScoreHandler.cpp, строки 45–50. Нарушает правило стиля №8.
  2. getConsistentReview()в Foo.java не является потокобезопасным, как требуется, синхронизируется с неправильной блокировкой.
  3. getFoo()в Foo.java перебирает список для фильтрации результатов. Поскольку наш исходный уровень теперь равен 8, вместо этого для фильтрации следует использовать потоковый API, что гораздо более лаконично.

Похоже, у вас есть кто-то, кто (плохо) пытается наставлять вас, заставляя вас «исправлять» код так, как он хочет, не говоря вам, почему он не работает или что с ним не так. Даже не ясно, является ли причина, по которой он хочет, чтобы вы изменили его, стилистическая или функциональная.

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

Если вам нужен более мягкий подход, попробуйте написать в ответ более подробно о том, что вы пробовали, почему это не сработало, и точно укажите, что вам нужно для продолжения:

Привет х,

Ранее я пытался удалить yкод, как вы предложили, но вместо этого это приводит zк тому, что функция не проходит (эти) тесты в (этих) случаях. Мне очень неясны требования здесь - изменение, которое вы просите, стилистическое или функциональное? Если это стилистически, не могли бы вы указать конкретные рекомендации по стилю, которые нарушает код, и если это функционально, не могли бы вы предоставить мне тестовый пример, который не работает?

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

Если после этого он по-прежнему говорит: «Просто сделай то, что я пытался», вы можете просто возразить: «Как и выше, боюсь, это не сработало, и мне потребуется гораздо больше деталей, чтобы решить проблему». Если кто-то обрушится на вас за то, что вы долго тратите время, покажите им бумажный след; довольно быстро должно стать ясно, что это он медлит и отказывается играть в мяч.

Обновление: только что попробовал это, и это сработало. В итоге он подошел к моему столу и помог мне улучшить мой код. В конце концов, это не было ошибкой, но могло быть и лучше. Его решение не было полным, но не таким неверным, как я думал. Думаю, мы оба были неправы.
@ Belle-Sophie Возможно, он увидел этот пост и понял, что вы действительно застряли.
@ Belle-Sophie Приятно слышать, рада помочь.
@ Belle-Sophie: Я думаю, это хороший урок на будущее; да, проверки кода обычно проводятся асинхронно с помощью инструментов... однако всякий раз, когда вы запутались в вопросе/ответе в проверке кода, может быть лучше переключиться на общение по телефону/лицом к лицу, чтобы быстро устранить недоразумения :)
И все же, была ли какая-то польза от его изменений? Или это была просто пустая трата времени?
Другая возможность заключается в том, что рецензент увидел дополнительные варианты использования, которые не были исправлены. Это могут быть дополнительные ошибки, а могут и не быть, в зависимости от деталей отчета об ошибке и способа управления ошибками. В любом случае, подход все еще работает.
@ Belle-Sophie: пожалуйста, обновите свой вопрос, так как это в значительной степени делает недействительным всю предпосылку вашего вопроса.
OPL «Код, на который он ссылался, не имел отношения к запросу на изменение, за исключением того, что они что-то делали с этим же полем ». [выделено мной]. Так это связано !

В рабочем процессе в моей компании это было бы легко.

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

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

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

Пробовал это. Он просто бросает его обратно в меня.
Вот где вы обратитесь к своему менеджеру. Ваш руководитель не разбирается в программировании, но вы говорите, что предложения ваших коллег не работают, он говорит, что они работают, так что в этой ситуации вашему начальнику будет очевидно, что он должен выполнить эту работу.
@ Belle-Sophie Итак... он настаивает, чтобы ты внес изменения под своим именем? Это не очень крикет. Я предлагаю добавить эту деталь к вашему вопросу, поскольку она подчеркивает менталитет парня.
@rath Можете ли вы объяснить, как вы используете слово «крикет» в этом контексте? Боюсь, я совсем заблудился.
@Two-BitAlchemist - это английский термин, означающий «просто неправильный способ вести дела с честью», из игр в крикет, где от игроков всегда ожидалось честное и джентльменское поведение.
Йиш. Знаешь, что не "крикет"? Отвечать на мудачество еще большим мудаком. Пассивно-агрессивные комментарии в коде (и обзоры кода) — это огромный красный флаг, свидетельствующий о готовности человека к совместной работе. Не делай этого.
«Не исправим» обычно является близкой причиной в заявке. Зачем закрывать тикет перед его повторным назначением?
Поэтому, если вы передаете проблемы, которые вы не знаете, как делать; как ты когда-нибудь научишься их делать?

Это может не относиться к вам по техническим или организационным причинам, но общий ответ таков:

Сделать модульный тест

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

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


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

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

Это не совсем то, что вы сказали, но это работает и устраняет проблему. Я понимаю это решение в своей голове, другой способ, вероятно, тоже сработает, но мне потребовалось бы намного больше времени, чтобы понять. Итак, я думаю, мы можем пометить это как Пройдено в обзоре кода?

Мне нравится этот ответ. К сожалению, это не сработает для меня (я обновил вопрос), но это хороший ответ для людей с похожей проблемой, которую можно проверить.
@ Belle-Sophie Я думал, что твоя ситуация может быть немного другой :) Парень звучит как коллега, которого я бы не хотел иметь
Похоже, у вас есть решение, но для следующего человека в этой ситуации - хотя обычно нецелесообразно полностью тестировать массивную унаследованную функцию, почти всегда можно выделить каждую отдельную часть логики в тестируемую функцию, а затем связать их. вместе, и часто это действие вызывает искру идеи вокруг того, где ошибка.

Я согласен с berry120. Однако ваши обзорные заметки были

"это не сработает, вы изменили неправильный код" и "вы не сделали то, что я сказал"

Мое предложение:

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

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

Я считаю, что завершил внедрение запрошенных клиентом изменений в Версии X.
Чтобы учесть примечания по обзору, я предпринял следующие дополнительные шаги для Версии Y.

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

Если примечания по обзору не касаются запроса клиента:
(следующее объясняет вашу задержку и первоначальное несоблюдение примечаний)

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

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

Если в версии Y все еще есть ошибки, которые вы не можете исправить:
(вероятно, это будут примечания к обзору или добавление версии X к примечаниям к обзору — измените формулировку ниже соответствующим образом!)

Я рад продолжить работу над версией Y.
К сожалению, интеграция примечаний к обзору с версией X (рабочий клиент запросил изменения) оказывается более сложной, чем отправка только версии X, и мне потребуется больше времени.

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

вставьте имя рецензента , если у вас есть минутка, я был бы очень признателен за ваше руководство относительно мотивации (или, возможно, менее любознательной: «природы») ваших изменений кода и возникающих ошибок, которые я получаю для Версии Y.

Очевидно, используйте фактические номера версий вместо X/Y (;

РЕДАКТИРОВАТЬ:

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

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

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

Убедитесь, что вы не производите впечатление враждебного, что-то вроде:

Привет Майк. Кажется, мы продолжаем говорить друг с другом об ошибке № 123. У вас есть 30 минут в понедельник, чтобы пройтись по нему? Я попрошу Джима присоединиться к нам, так как он работал над этим в последнее время.

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

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

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

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

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

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

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

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

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

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

Какие бы инструменты ни использовала ваша компания для отслеживания проблем, обычно это инструменты для ведения учета и коммуникации; и хотя они имеют свое место, я думаю, это демонстрирует, что вербальное общение не может быть заменено.

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