Недавнее исправление ошибки требовало от меня просмотра кода, написанного другими членами команды, где я нашел это (это C #):
return (decimal)CostIn > 0 && CostOut > 0 ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 : 0;
Теперь, если есть веская причина для всех этих бросков, это все еще кажется очень трудным для подражания. В вычислении была небольшая ошибка, и мне пришлось распутать ее, чтобы решить проблему.
Я знаю стиль кодирования этого человека по обзору кода, и его подход заключается в том, что чем короче, тем лучше. И, конечно, здесь есть ценность: мы все видели излишне сложные цепочки условной логики, которые можно было бы привести в порядок несколькими удачно расположенными операторами. Но он явно более искусен, чем я, в следующих цепочках операторов, объединенных в одно утверждение.
Это, конечно, в конечном итоге вопрос стиля. Но было ли что-либо написано или исследовано для определения точки, в которой стремление к краткости кода перестает быть полезным и становится барьером для понимания?
Причиной для приведения является Entity Framework. БД должен хранить их как обнуляемые типы. Десятичный? не эквивалентен десятичному в C # и должен быть приведен.
источник
CostOut
, равноDouble.Epsilon
, и, следовательно, больше нуля. Но(decimal)CostOut
в этом случае ноль, и у нас есть ошибка деления на ноль. Первый шаг должен состоять в том, чтобы получить правильный код , который я думаю, что это не так. Получите это правильно, сделайте контрольные примеры, и затем сделайте это изящным . Элегантный код и краткий код имеют много общего, но иногда краткость не является душой элегантности.Ответы:
Чтобы ответить на ваш вопрос о существующих исследованиях
Да, была работа в этой области.
Чтобы получить представление об этом, вы должны найти способ вычислить метрику, чтобы можно было проводить сравнения на количественной основе (а не просто выполнять сравнение, основанное на остроумии и интуиции, как это делают другие ответы). Одна потенциальная метрика, которая была рассмотрена
Цикломатическая сложность ÷ исходные строки кода ( SLOC )
В вашем примере кода это соотношение очень высокое, потому что все сжато в одну строку.
Ссылка
Вот несколько ссылок, если вы заинтересованы:
McCabe, T. and A. Watson (1994), Сложность программного обеспечения (CrossTalk: журнал оборонной программной инженерии).
Watson, AH & McCabe, TJ (1996). Структурное тестирование: методология тестирования с использованием метрики цикломатической сложности (специальная публикация NIST 500-235). Получено 14 мая 2011 г. с веб-сайта McCabe Software: http://www.mccabe.com/pdf/mccabe-nist235r.pdf.
Розенберг Л., Хаммер Т., Шоу Дж. (1998). Метрики и надежность программного обеспечения (Материалы IEEE International Symposium по разработке программного обеспечения надежности). Получено 14 мая 2011 г. с веб-сайта Университета штата Пенсильвания: http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.104.4041&rep=rep1&type=pdf.
Мое мнение и решение
Лично я никогда не ценил краткость, только читабельность. Иногда краткость помогает удобочитаемости, иногда нет. Более важно то, что вы пишете действительно очевидный код (ROC) вместо кода только для записи (WOC).
Просто для забавы, вот как я бы написал это и попросил членов моей команды написать это:
Также обратите внимание, что введение рабочих переменных имеет приятный побочный эффект - запускается арифметика с фиксированной точкой вместо целочисленной арифметики, поэтому необходимость во всех этих приведениях
decimal
исключается.источник
if ((costIn < 0) || (costOut < 0)) throw new Exception("costs must not be negative");
Краткость хороша, когда она уменьшает беспорядок вокруг вещей, которые имеют значение, но когда она становится краткой , упаковывая слишком много релевантных данных слишком плотно, чтобы их можно было легко отслеживать, тогда релевантные данные сами становятся беспорядочными, и у вас возникает проблема.
В этом конкретном случае приведение
decimal
повторяется снова и снова; было бы лучше в целом переписать его примерно так:Внезапно строка, содержащая логику, становится намного короче и помещается на одной горизонтальной линии, так что вы можете видеть все без прокрутки, и смысл становится гораздо более очевидным.
источник
((decOut - decIn ) / decOut) * 100
в другую переменную.CostOut > 0
), так что вам придется расширить условное выражение вif
-условие. Не то, чтобы в этом было что-то не так, но это добавляет больше многословия, чем просто введение локального.Хотя я не могу привести какое-либо конкретное исследование по этому вопросу, я бы сказал, что все эти приведения в вашем коде нарушают принцип «Не повторяйся». Похоже, что ваш код пытается выполнить преобразование
costIn
иcostOut
в типDecimal
, а затем выполнить некоторые проверки правильности результатов таких преобразований и выполнить дополнительные операции с этими преобразованными значениями, если проверки пройдут. Фактически, ваш код выполняет одну из проверок работоспособности для неконвертированного значения, повышая вероятность того, что costOut может содержать значение, которое больше нуля, но меньше, чем половина наименьшего ненулевого значения, котороеDecimal
может представлять. Код был бы намного понятнее, если бы он определял переменные типаDecimal
для хранения преобразованных значений, а затем воздействовал на них.Кажется любопытным, что вас больше интересует соотношение
Decimal
представленийcostIn
и,costOut
чем соотношение фактических значенийcostIn
иcostOut
, если только код также не будет использовать десятичные представления для каких-либо других целей. Если код будет использовать эти представления в дальнейшем, это будет дополнительным аргументом для создания переменных для хранения этих представлений, вместо того, чтобы иметь непрерывную последовательность приведений во всем коде.источник
Decimal
округления приведения зависит от величины рассматриваемой ценности, мне трудно представить бизнес-правила, которые бы определяли, как на самом деле будут вести себя приведения.Decimal
Типа нет. Значение 1.0d / 3.0 будет иметь больше цифр справа от десятичного числа, чем может поддерживаться при использовании больших чисел. Таким образом, сложение и вычитание того же большего числа приведет к потере точности. Типы с фиксированной запятой могут терять точность при дробном умножении или делении, но не при сложении, вычитании, умножении или делении с остатком (например, 1,00 / 7 - это 0,14 остатка 0,2; 1,00 деление 0,15 - 6 остатка 0,10).Я смотрю на этот код и спрашиваю «как стоимость может быть 0 (или меньше)?». На какой особый случай это указывает? Код должен быть
Я предполагаю, что имена здесь: изменить
BothCostsAreValidProducts
иNO_PROFIT
при необходимости.источник
if (CostIn <= 0 || CostOut <= 0)
совершенно нормально.Краткость перестает быть добродетелью, когда мы забываем, что это означает цель, а не добродетель сама по себе. Нам нравится краткость, потому что она коррелирует с простотой, и нам нравится простота, потому что более простой код легче понять, легче модифицировать и содержать меньше ошибок. В конце мы хотим, чтобы код достиг этой цели:
Выполните бизнес-требования с наименьшим объемом работы
Избегайте ошибок
Позвольте нам внести изменения в будущем, которые продолжают выполнять 1 и 2
Это цели. Любой принцип или метод разработки (будь то KISS, YAGNI, TDD, SOLID, доказательства, системы типов, динамическое метапрограммирование и т. Д.) Эффективны только в той мере, в какой они помогают нам достичь этих целей.
Рассматриваемая линия, кажется, упустила из виду конечную цель. Хотя это коротко, это не просто. Он фактически содержит ненужную избыточность, повторяя одну и ту же операцию приведения несколько раз. Повторение кода увеличивает сложность и вероятность ошибок. Смешивание преобразования с фактическим расчетом также затрудняет выполнение кода.
Линия имеет три проблемы: охранники (специальный корпус 0), тип приведения и расчет. Каждая проблема довольно проста, если рассматривать ее отдельно, но поскольку все они смешаны в одном и том же выражении, становится трудно следовать.
Непонятно, почему
CostOut
не разыгрывается при первом использованииCostIn
. Может быть веская причина, но цель не ясна (по крайней мере, не без контекста), что означает, что сопровождающий будет опасаться изменять этот код, потому что могут быть некоторые скрытые предположения. И это анафема к ремонтопригодности.Поскольку
CostIn
выполняется приведение до сравнения с 0, я предполагаю, что это значение с плавающей запятой. (Если бы это был int, не было бы никакой причины бросать). Но еслиCostOut
это число с плавающей запятой, тогда код может скрывать скрытую ошибку деления на ноль, поскольку значение с плавающей запятой может быть небольшим, но не нулевым, а нулевым при приведении к десятичному числу (по крайней мере, я считаю, что это возможно).Таким образом, проблема не в краткости или ее отсутствии, проблема в повторяющейся логике и смешении проблем, приводящих к сложному в обслуживании коду.
Введение переменных для хранения приведенных значений, вероятно, увеличит размер кода, подсчитанного в количестве токенов, но уменьшит сложность, разделит проблемы и улучшит ясность, что приближает нас к цели кода, который легче понять и поддерживать.
источник
Краткость совсем не добродетель. Читаемость это добродетель.
Краткость может быть инструментом достижения добродетели или, как в вашем примере, может быть инструментом достижения чего-то совершенно противоположного. Так или иначе, это не имеет почти никакой собственной ценности. Правило о том, что код должен быть «как можно короче», можно также заменить на «настолько непристойное, насколько это возможно» - все они в равной степени бессмысленны и потенциально вредны, если они не служат большей цели.
Кроме того, код, который вы разместили, даже не следует правилу краткости. Если бы константа быть объявлено с суффиксом M, большинство ужасных
(decimal)
слепков можно было бы избежать, так как компилятор будет способствовать оставаясьint
вdecimal
. Я считаю, что человек, которого вы описываете, просто использует краткость в качестве оправдания. Скорее всего не намеренно, но все же.источник
За годы моего опыта я пришел к выводу, что предельная краткость - это время - время доминирует над всем остальным. Это включает как время выполнения - сколько времени занимает выполнение программы, так и время обслуживания - сколько времени требуется для добавления функций или исправления ошибок. (То, как вы уравновешиваете эти два, зависит от того, как часто исполняется рассматриваемый код, а не улучшается - помните, что преждевременная оптимизация по-прежнему является корнем зла .) Краткость кода предназначена для улучшения краткости обоих; более короткий код обычно выполняется быстрее, и, как правило, его легче понять и, следовательно, поддерживать. Если это тоже не сработает, то это чистый минус.
В показанном здесь случае, я думаю, что краткость текста была неверно истолкована как краткость подсчета строк за счет читабельности, что может увеличить время обслуживания. (Может также потребоваться больше времени для выполнения, в зависимости от того, как выполняется приведение, но если вышеупомянутая строка не выполняется миллионы раз, это, вероятно, не является проблемой.) В этом случае повторяющиеся десятичные приведения ухудшают читабельность, поскольку сложнее посмотрим, что является наиболее важным расчетом. Я бы написал так:
(Изменить: это тот же код, что и другой ответ, так что пошли.)
Я фанат троичного оператора
? :
, так что я бы оставил это.источник
? :
- я думаю, что приведенный выше пример достаточно компактен, особенно. по сравнению с if-then-else.:
.if-else
читает по-английски: не могу пропустить, что это значит.Как и почти все ответы выше, читабельность всегда должна быть вашей главной целью. Я также думаю, что форматирование может быть более эффективным способом достижения этого, чем создание переменных и новых строк.
Я полностью согласен с аргументом цикломатической сложности в большинстве случаев, однако ваша функция кажется небольшой и достаточно простой, чтобы лучше справляться с хорошим контрольным примером. Из любопытства, почему нужно приводить к десятичному числу?
источник
decimal
, верно?double
! =decimal
есть большая разница.Для меня это выглядит так, как будто большая проблема с читабельностью здесь заключается в полном отсутствии форматирования.
Я бы написал это так:
В зависимости от того, является ли тип
CostIn
и типомCostOut
с плавающей запятой или целым типом, некоторые из приведений также могут быть ненужными. В отличие отfloat
иdouble
, интегральные значения неявно повышаются доdecimal
.источник
Код может быть написан на скорую руку, но приведенный выше код должен быть написан в моем мире с гораздо лучшими именами переменных.
И если я правильно прочитал код, то он пытается выполнить расчет маржи.
источник
Я предполагаю, что CostIn * CostOut являются целыми числами.
Так я бы написал
M (Деньги) - это десятичное число
источник
Код написан для понимания людьми; краткость в этом случае не очень выгодна и увеличивает нагрузку на сопровождающего. Для этой краткости вам необходимо расширить это, либо сделав код более самодокументированным (лучше имена переменных), либо добавив больше комментариев, объясняющих, почему он работает таким образом.
Когда вы пишете код для решения проблемы сегодня, этот код может стать проблемой завтра, когда требования изменятся. Техническое обслуживание всегда должно приниматься во внимание, и улучшение понимания кода крайне важно.
источник
Краткость больше не является добродетелью, когда
источник
Если бы он проходил проверочные юнит-тесты, то было бы хорошо, если бы была добавлена новая спецификация, требовался новый тест или расширенная реализация, и требовалось «распутать» краткость кода, то есть когда проблема возникнет.
Очевидно, что ошибка в коде показывает, что есть другая проблема с Q / A, которая была оплошностью, поэтому факт, что была ошибка, которая не была обнаружена, является поводом для беспокойства.
При работе с нефункциональными требованиями, такими как «читаемость» кода, он должен определяться менеджером по разработке и управляться ведущим разработчиком и уважаться разработчиками для обеспечения правильной реализации.
Постарайтесь обеспечить стандартизированную реализацию кода (стандарты и соглашения), чтобы обеспечить «читабельность» и простоту «ремонтопригодности». Но если эти атрибуты качества не определены и не применяются, то в итоге вы получите код, подобный приведенному выше.
Если вам не нравится видеть этот вид кода, попробуйте договориться с командой о стандартах и соглашениях реализации, запишите его и сделайте случайные обзоры кода или выборочные проверки для подтверждения соблюдения соглашения.
источник