Как отказаться от проверки кода, которая, по вашему мнению, не нужна?

89

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

Фиксатор, который старше меня, настаивает на том, что его исправление необходимо, но для меня это не более, чем софистика С ++. Частью нашего процесса развертывания является проверка кода, и я, как второй по величине инженер в небольшой компании, должен рассмотреть изменения.

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

Джеймс
источник
113
Кажется очевидным для меня. В обзоре вы указываете, что код - это интеллектуальная мастурбация C ++, целью которой является решение проблемы, которой не существует. И затем, как часть процесса проверки, вы отклоняете код.
user16764
86
Не используйте слова «интеллектуальная мастурбация», когда отвергаете ее. Если вы используете эту формулировку, вы будете противодействовать ему, и он не увидит, что вы говорите, как потенциально правильную техническую критику, он воспримет это как личную атаку. Это вполне может быть интеллектуальная мастурбация, но вы можете быть абсолютно уверены, что он так не видит.
Майкл Шоу
15
Джеймс - Я убрал эмоции из твоего вопроса, чтобы позволить сообществу сосредоточиться на твоем основном вопросе. Как указывают другие, использование загруженной терминологии в вашем обзоре только усугубит ситуацию, которая явно деликатна.
8
C и C ++ полны вещей, которые, кажется, работают, но на самом деле неопределенное поведение. UB - это настоящий недостаток, даже если вы не можете написать тест, который показывает, что он не работает должным образом. Например, многие компиляторы используют UB в качестве возможности оптимизации, предполагая, что случай, который не определен, никогда не произойдет. Например, если вы использовали size > size+1для проверки переполнения целого числа со знаком, компилятор может просто заменить это выражение false, поскольку переполнение целых чисел со знаком не определено. См. Blog.llvm.org/2011/05/…
CodesInChaos
5
@MichaelShaw "он увидит это как личную атаку". что формулировка вопроса звучит для меня как личная атака на более старшего разработчика, с которым у ОП есть разногласие, которое он теперь может "выиграть", потому что он был поставлен во власть.
jwenting

Ответы:

274

Запросите тестовый пример, который не проходит без изменения, которое успешно выполняется с изменением.

Если он не может произвести его, вы используете это как оправдание.

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

Craig
источник
245
Или, может быть, если он сможет это сделать, вы пересматриваете свои предположения и считаете, что он все-таки был прав. Предположительно, цель упражнения - улучшить код, а не выиграть дискуссию.
Калеб
99
Тот факт, что вы не можете написать тест, не означает, что он не сломан. Неопределенное поведение, которое обычно работает должным образом (C и C ++ полны этого), условия гонки, потенциальное переупорядочение из-за слабой модели памяти ...
CodesInChaos
29
Кроме того, как насчет стандартного рефакторинга? Как вы пишете провальный тест для части работы по рефакторингу?
Майк Веллер
62
«Попросите его доказать, почему это необходимо ...». Может быть, попросите его научить вас, почему это необходимо.
Майк Шеррилл 'Cat Recall'
8
@RhysW: Неправильное предположение. Существующий код с условием гонки также не может быть протестирован в этом отношении. Таким образом, новый код теоретически лучше и эмпирически эквивалентен. Ваше предположение остается верным, только если старый код эмпирически лучше.
MSalters
152

Рецензенты должны быть объективными.

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

Рассмотрим командный подход.

Если вы не можете удалить себя, возможно, вы можете попросить нескольких других инженеров проверить код одновременно. Либо они согласятся с вами, что код должен быть отклонен, либо нет. Если они согласятся с вами, то это больше не будет только вы против фиксатора, и вы сможете более убедительно доказать, что команда объективно рассмотрела исправление и решила не принимать его. С другой стороны, если они решат принять исправление, то это тоже будет командным решением. Само собой разумеется, что вы должны участвовать с максимально открытым умом, и что вы не должны пытаться влиять на мнения других членов команды чем-либо, кроме рационального обсуждения. Важно: если позже будет плохой результат, не бросайте команду под автобус, говоря: «Ну, я всегда говорил, что это плохой код, но другие члены команды меня обогнали ».

Отклонения являются естественной частью процесса проверки кода.

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

Ответственность сокращается в обе стороны.

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

Делать заметки.

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

Калеб
источник
24
+1 это намного полезнее, чем принятый ответ
Симон Бергот
2
Определенно. Принятый ответ специфичен для одного случая, явно не такой общий и продуманный, как этот.
Флориан Маргэйн
40

Запишите ваши причины отказа и защиты от возможных встречных аргументов. Затем разумно обсудите это со специалистом по ремонту и при необходимости перейдите к руководству.

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

У вас возникнет недоброжелательность с исправителем, поэтому убедитесь, что проблема того стоит (т. Е. Причиняет ли исправление отсутствие исправления?)

Также, если фиксатор каким-либо образом связан с владельцами, забудьте этот ответ.

Дэн Пичельман
источник
9
Я бы проголосовал дважды за последнюю часть вашего ответа в одиночку. Очень солидный ответ.
31

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

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

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

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

Темный рыцарь
источник
8
Замечательно, но я не понимаю, как пример внизу связан (я не утверждаю, что он не связан, конечно, просто укажу на мое непонимание отношений).
Угорен
8
@ugoren Ха-ха, мне нравится, что ты там сделал!
TheDarkKnight
1
@ugoren отношение "если вы не можете видеть всю картину, не
формируйте
2
Мне всегда нравится скромный подход, я бы отверг это изменение на основании того, что я не понимаю, и поэтому я не квалифицирован, чтобы дать ему все в порядке.
PhilDin
2
@PhilDin Смиренно, а потом все говорят, что ты не подходишь для этой работы. Если он в состоянии пересмотреть код, я думаю, что люди, которые ставят его в такое положение, ожидают, что он достаточно квалифицирован, чтобы сделать это.
Энди
9

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

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

Однако в тот или иной момент вы смените платформу (скажем, вы хотите стать мобильным, поэтому вы портируете на ARM или хотите ускорить работу и использовать GPU). В этот момент все может начать ломаться, и вам нужно будет его отладить. Это может быть так же тривиально, как обновить компилятор до более новой версии (и вам может понадобиться / понадобиться).

Проблемный код был:

int d[16];

int SATD (void)
{
   int satd = 0, dd, k;
   for (dd=d[k=0]; k<16; dd=d[++k]) {
     satd += (dd < 0 ? -dd : dd);
   }
   return satd;
 }

Во время последней итерации d[++k] => d[++15] => d[16]осуществляется доступ. Поскольку обычно следующим элементом была легальная память (с точки зрения подкачки, а не модели памяти C), даже тривиальные компиляторы не создавали исполняемый файл со странным поведением. Единственный способ написания тестового примера - это найти платформу с точно такой же моделью памяти, как у C (возможно, VM).

Однако gcc 4.8 prerelase видел, что d[++k]выполняется в каждом цикле. Таким k < 16образом, в противном случае доступ был бы незаконным, и легальность программы, передаваемой компилятору, является частью контракта. Таким образом, условие цикла всегда выполняется с учетом допущений, поэтому это бесконечный цикл. Это может звучать странно, но это была совершенно легальная оптимизация - испускание system("dd if=/dev/zero of=/dev/sda"); system("format c:")также будет правильной заменой цикла. Есть более тонкие способы показать поведение. Например, во время лекции о транзакционной памяти я помню, что докладчик несколько раз пытался получить неправильное значение, когда 2 потока увеличивали одно и то же значение.

Однако C / C ++, в отличие от некоторых языков, стандартизирован, поэтому такой спор может быть сделан со ссылкой на объективный источник:

  • Если вы думаете, что это не проблема, попробуйте доказать это, используя стандарт C / C ++ (то есть, что это приводит к определенному значению и поведению)
  • Если он думает, что это проблема, попросите его доказать это, используя стандарт C / C ++ (то есть, что это приводит к неопределенному значению или поведению)

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

Мацей Печотка
источник
4

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

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

djechlin
источник
1
«... но, возможно, есть причина, по которой он или она сделал это ...» - это большое предположение, которое вы делаете. И вам также не хватает того, что в Вопросе говорится, что (по мнению ФП) проблем не существует. Конечно, ответственность должна быть на человеке, который предлагает «исправить», чтобы продемонстрировать, что есть реальная проблема, которая должна быть решена. Бессмысленно предлагать «более простое решение» проблемы, которой не существует.
Стивен С.
4
@StephenC да, и я придерживаюсь мнения, что ОП должна дать преимущество сомнения в этом случае. Или это будет действительно конфронтационный обзор.
Джечлин
3

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

Конечно, вокруг этого может быть чувствительная внутренняя политическая обстановка. Но это путь, которым я бы пошел (восхитительно).

SnakeDoc
источник