Что вы скажете в обзоре кода, когда другой человек построил слишком сложное решение? [закрыто]

37

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

В этой ситуации я спрашиваю "почему это было сделано таким образом?"

Ответ в том, что другой человек хотел сделать это таким образом.

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

Ответ - нет.

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

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

Эквивалентный сценарий:
Коллега тратит 8 часов на рефакторинг кода вручную, что можно было бы автоматически сделать в Resharper за 10 секунд. Естественно, я не доверяю рефакторингу вручную, поскольку он сомнительного качества и не полностью протестирован.
И снова я получаю ответ: «Ну, это уже сделано».

Что является подходящим ответом на это отношение?

дан
источник
22
Существует только одна вещь , чтобы сказать
47
«Вы создали слишком сложное решение»
Данте
2
Какая проблема находится в центре внимания этого вопроса: менталитет / отношение программиста, управление проектами (в частности, управление временем) или уровень квалификации?
Rwong
6
это, вероятно, принадлежит на рабочем месте - это не вопрос программирования.
GrandmasterB

Ответы:

25

Менталитет / отношение

  • Подавать пример
  • Призыв к личной беседе (один на один, вне обзора кода)
  • Поощрять простой менталитет среди членов команды

Управление командой

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

Уровень мастерства

  • Выделите некоторое время для занятий парным программированием или индивидуальных занятий для лучшего использования инструментов разработчика (рефакторинг, проверка кода)

Управление проектами (рисками)

  • Проводите проверку кода чаще, асинхронно (Примечание)
    • Заметка про "асинхронно"
      • Рецензент кода должен получать уведомления / приглашения для проверки изменений, как только они будут приняты
      • Рецензент должен иметь возможность просмотреть код перед любой встречей с разработчиком.
      • Если требуется разъяснение от разработчика, сделайте это неофициально в IM / электронной почте без отрицательного мнения
rwong
источник
69

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

Вы говорите: «Вы создали слишком сложное решение».

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

Если уже слишком поздно что-либо менять, почему вы делаете обзор кода?

Hermann Ingjaldsson
источник
Вы в основном говорите, что проверка кода работает только с хорошими, всегда разумными и рациональными символами. Реальный мир выглядит иначе ...
Филипп
3
Иногда вам приходится делать то, что вам не нравится, например, говорить кому-то, кто посвятил целый день работе над написанием сложного кода, что «это нехорошо, откатить его и начать все сначала» или что-то в этом роде. Это отстой, но вы будете благодарны, что сделали.
joshin4colours
3
Простой ответ, который точно подводит итог ситуации. Другой ваш ответ на вопрос «Это уже сделано» - объяснить, что слишком сложное решение обойдется в потерю времени на обслуживание, а его доработка сэкономит время в долгосрочной перспективе.
DJClayworth
30
+ ∞ для «Если в любом случае уже слишком поздно что-либо менять, тогда зачем вы делаете обзор кода?»
mskfisher
16

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

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

Анджей Бобак
источник
Если это действительно борьба за избавление от лишнего кода, то вы можете позволить ему сохранить его, ЕСЛИ И ТОЛЬКО ЕСЛИ он может создать полный набор модульных тестов, чтобы убедиться, что он продолжает работать. В любом случае, он должен на самом деле закончить работу.
Майкл Кохн
+1 по простому факту, что код имеет (очевидные) ошибки и, следовательно, не был проверен.
Ramhound
8

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

Это не приемлемый ответ:

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

  • Если это действительно способ сказать «я не хочу меняться», то вам нужно принять позицию, что дополнительная сложность - ПЛОХАЯ для кодовой базы, ПОТОМУ ЧТО проблем / затрат, которые будут возникать позже. И снижение вероятности будущих проблем - реальная причина, по которой вы делаете обзор кода в первую очередь.

А также ...

... решение не было полностью функциональным ...

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

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

Что является подходящим ответом на это отношение?

В конечном итоге, доведите это до сведения руководства ... если только у вас нет возможности исправить это самостоятельно. (Конечно, это не сделает вас популярным.)

Стивен С
источник
7

Вы были правы, они были неправы

  • нарушенный принцип ЯГНИ
  • сломанный принцип KISS
  • код полностью протестирован? Если нет, то это не сделано

Что является подходящим ответом на это отношение?

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

BЈовић
источник
5

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

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

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

stefan.s
источник
Я бы сказал, что 10 раз за один день это немного много. Если вы действительно хотите подтолкнуть его, 3 или 4 проверки будут в порядке, это означает, что проверка в среднем каждые 2 часа дает обычный 8-часовой день. Но 10 проверок, кажется, что нет времени, чтобы на самом деле что-то проверить, отчитаться или внести изменения, основанные на самом обзоре.
Ramhound
@Ramhound Да, 10 проверок - это крайний случай, от 3 до 4 раз гораздо более типичный. И нужно некоторое время, чтобы привыкнуть к нему ...
stefan.s
2

Вы должны сосредоточиться на коренной причине проблемы:

  1. Обучение программистов направлено на повышение сложности, предоставляемой программистам. Способность сделать это была проверена школой. Таким образом, многие программисты подумают, что если они реализуют простое решение, они не выполняют свою работу правильно.
  2. Если программист следует той же схеме, что и сотни раз, когда учился в университете, то просто так думают программисты - чем больше сложностей, тем сложнее и, следовательно, лучше.
  3. Таким образом, чтобы это исправить, вам нужно строго отделить требования вашей компании от сложности по сравнению с тем, что обычно требуется для обучения программиста. Хороший план - это правило типа «наивысший уровень сложности должен быть зарезервирован только для задач, предназначенных для улучшения ваших навыков, и его не следует использовать в рабочем коде».
  4. Для многих программистов станет неожиданностью, что им не позволено делать свои самые безумные разработки в важной среде производственного кода. Просто зарезервируйте время для программистов, чтобы сделать экспериментальные проекты, и затем держите всю сложность на той стороне забора.

(в обзоре кода уже слишком поздно его менять)

оборота тп1
источник
2

Я не знаю ничего, что работает после написания кода.

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

Есть другой подход, который работает с подрядчиками - контракты с фиксированной ценой. Чем проще решение, тем больше $$ получает программист.

Майк Данлавей
источник
1

Вы не можете исправить мир.

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

К сожалению, то, что вы испытываете в обзоре кода, слишком распространено. Я работал в нескольких организациях, где я обнаружил, что часто проверяю 100 строк кода, которые могли быть написаны за десять, и я получил тот же ответ, что и вы: «Он уже написан и протестирован» или «Мы ищем ошибки, а не редизайн ".

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

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

Кевин Клайн
источник
0

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

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

Если вы еще этого не сделали, организуйте свои мысли и сделайте это.

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

Без шансов
источник
0

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

  • Требования должны быть четко определены.
  • Вам необходимо разработать варианты использования, которые поддерживают требования.
  • Вам необходимо указать функции, необходимые для реализации сценариев использования.
  • Вам необходимо указать любые нефункциональные требования (время ответа, доступность и т. Д.)
  • Вам нужен RTM (Requirements Tracabilty Matrix), чтобы сопоставить каждую системную функцию с вариантом использования и реальным требованием.
  • Удалите любую функцию, которая не поддерживает фактическое требование.
  • Наконец, в вашем обзоре кода пометьте любой код, который непосредственно не реализует или не поддерживает определенные функции.
Джеймс Андерсон
источник
0

Скорее всего, это не слишком сложно, потому что после этого большинство людей чувствует себя плохо. Я предполагаю, что когда это происходит, уже написано много кода, не говоря об этом ни слова. (Почему это так? Потому что у этого человека достаточно полномочий, поэтому его код не нужно пересматривать в реальности?)

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

Сказать «это слишком сложно» никуда не приведет.

Филипп
источник
0

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

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

Райан Кинал
источник
0

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

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

Если вы видите проблему (чрезмерно сложную), скажите что-то более конкретное, например:

  • Разве замена части X на Y не упростит код или не облегчит его понимание?
  • Я не понимаю, что вы делаете здесь в части X, я думаю, что вы пытались сделать это. Представьте более чистый способ сделать это.
  • Как вы это проверили? Вы проверяли это? Если это слишком сложно, это обычно приводит к пустым взглядам. Запросы на тестирование часто заставляют человека самостоятельно упростить свой код, когда он не может понять, как тестировать исходный код.
  • Кажется, здесь есть ошибка, изменение кода на это решит проблему.

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

Замочить
источник
Код имеет очевидные ошибки. Тот факт, что автор также считает, что само решение является неправильным, подчеркивает тот факт, что есть ошибки. Если в вашем коде есть ошибки, и я не говорю о неочевидных ошибках, которые вы не можете обнаружить без полного регрессионного теста, существует проблема с указанным кодом.
Ramhound
@Ramhound: если есть ошибки, укажите на конкретные ошибки. Если исправление ошибок не является частью процесса проверки, то какой смысл проводить проверку? Как я уже сказал, слишком сложный не ошибка. Это, конечно, недолго, но если единственный человек, который считает, что это слишком сложно, это ОП, и никто другой не делает этого, ну да ладно. Работайте усердно, станьте лидером и указывайте качество, соответствующее вашим стандартам. Я могу сочувствовать ОП, я прошел через те же проблемы, теперь, когда у меня есть полномочия направлять людей на внесение желаемых изменений, я обнаружил, что другие вещи оказываются более приоритетными.
Данк
0

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

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

  • Делать самое простое, что могло бы сработать?
  • Вам это не понадобится (вы решаете проблемы, которых не было в спецификации)
  • Напишите тест перед написанием кода (Помогает сфокусировать ваш код)
  • Не повторяйся

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

Билл К
источник
0

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

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

Это плохая привычка для любого разработчика. Если он / она работал над спецификацией проекта, то нет подходящего варианта без веской причины - нет, нет.

temptar
источник
0

Одним словом: проворный

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

Для нормальной разработки на основе проектов я бы рекомендовал использовать подход Scrum . Для сред непрерывной разработки / интеграции, особенно если у вас много разработчиков, работающих над одними и теми же или смежными проектами, рассмотрите возможность включения элементов Kanban . Другой эффективный подход заключается в использовании парного программирования , определенной практики экстремального программирования .

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

Адам
источник
-1

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

Vatine
источник
-2

Вы должны кодировать после удаления / отката их код: «Ой, ваш код пропал. Пожалуйста, переписайте его. Как вы уже написали, однажды вам потребуется менее двадцати минут, чтобы предоставить ТОЛЬКО код, требуемый спецификацией.

«Мой следующий обзор через 20 минут.

"Добрый день."

НЕ принимайте никаких аргументов!

Готово, ИМХО

Крис

cneeds
источник
Я рад, что мой босс не работает таким образом.
@Jon: Когда люди отвечают непрофессионально, как в «хорошо, что это уже сделано», как сказал бы мой шестилетний ребенок, тогда вы должны обращаться с ними как с детьми.
собирается
2
Не могу сказать, что согласен. Какие результаты вы ожидаете получить от своих людей, если вы «относитесь к ним как к детям»? Есть и другие подходы, которые, ИМХО, более конструктивны.
Я не защищаю отношение к профессионалам как к детям. В приведенном примере мы имеем упрямого, наглого человека, который пишет глючный код с ненулевой функциональностью и возвращает детские ответы на законные вопросы. Дэн просит лучшего способа справиться с этим. Не самый популярный способ.
собирается
К счастью, у меня нет «детей» в моей команде, поэтому нет необходимости относиться к ним как к чему-либо, кроме профессионалов. Они не добавляют незаданных функций (напрасно тратят мое время и деньги), они пишут довольно солидный код и, когда их просят вернуться или пересмотреть что-то, делают это и учатся на опыте.
собирается