Сгенерировать исключение или дать сбою

52

Мне интересно, есть ли плюсы и минусы против этого стиля:

private void LoadMaterial(string name)
{
    if (_Materials.ContainsKey(name))
    {
        throw new ArgumentException("The material named " + name + " has already been loaded.");
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );
}

Этот метод должен nameбыть запущен только один раз. _Materials.Add()сгенерирует исключение, если оно будет вызываться несколько раз для одного и того же name. В результате моя охрана полностью избыточна или есть некоторые менее очевидные преимущества?

Это C #, Unity, если кому-то интересно.

асинхронной
источник
3
Что произойдет, если вы дважды загрузите материал без охраны? Выдает _Materials.Addисключение?
user253751
2
На самом деле я уже упоминал в исключениях бросков: P
async
9
Примечания: 1) Рассмотрите возможность использования string.Formatконкатенации через строку для создания сообщения об исключении. 2) используйте только в том asслучае, если ожидаете, что приведение не выполнено, и проверьте результат null. Если вы ожидаете получить всегда Material, используйте приведение типа (Material)Resources.Load(...). Исключение с явным приведением легче отладить, чем исключение с нулевой ссылкой, которое возникает позже.
CodesInChaos
3
В этом конкретном случае ваши абоненты могут также найти компаньона LoadMaterialIfNotLoadedили ReloadMaterial(это имя может использовать улучшение) полезным.
jpmc26
1
С другой стороны: этот шаблон подходит для добавления элемента в список. Однако не используйте этот шаблон для заполнения всего списка, потому что вы столкнетесь с O (n ^ 2) ситуацией, когда с большими списками вы столкнетесь с проблемами производительности. Вы не заметите это при 100 записях, но при 1000 записях вы наверняка и при 10000 записей это станет серьезным узким местом.
Питер Б

Ответы:

109

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

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

Ixrec
источник
3
Согласен. Почти всегда лучше бросить исключение для нарушения предусловия.
Фрэнк Хилман
22
«Преимущество состоит в том, что ваше« пользовательское »исключение имеет сообщение об ошибке, которое имеет смысл для любого, кто вызывает эту функцию, не зная, как она реализована (что в будущем может быть вами!)». Главный совет.
асинхронно
2
«Явное лучше, чем неявное». - Zen of Python
jpmc26
4
Даже если выдаваемая ошибка _Materials.Addне является ошибкой, которую вы хотите передать вызывающей стороне, я думаю, что этот метод перемаркировки ошибки неэффективен. Для каждого успешного вызова LoadMaterial(нормальной работы) вы дважды проходите один и тот же тест на работоспособность , LoadMaterialзатем и снова _Materials.Add. Если намотано больше слоев, каждый из которых использует один и тот же стиль, вы можете даже много раз провести один и тот же тест.
Марк ван Леувен
15
... Вместо этого подумайте о том, чтобы _Materials.Addбезоговорочно погрузиться в него , затем поймать потенциальную ошибку и в обработчике выбросить другую. Дополнительная работа теперь выполняется только в ошибочных случаях, исключительном пути выполнения, где вы вообще не беспокоитесь об эффективности.
Марк ван Леувен
100

Я согласен с ответом Ixrec . Тем не менее, вы можете рассмотреть третий вариант: сделать функцию идемпотентной. Другими словами, возвращайтесь раньше, чем бросайте ArgumentException. Это часто предпочтительнее, если в противном случае вы будете вынуждены проверять, была ли она уже загружена, перед LoadMaterialкаждым вызовом . Чем меньше предварительных условий, тем меньше когнитивная нагрузка на программиста.

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

Карл Билефельдт
источник
2
С другой стороны, тихий сбой функции может привести к непредвиденному поведению, в результате чего будет сложнее обнаружить ошибки, возникающие позже.
Филипп
30
@ Филипп функция не будет молчать. То, что он идемпотентен, означает, что его многократный запуск имеет тот же эффект, что и один раз. Другими словами, если материал уже загружен, то наша спецификация («материал должен быть загружен») уже удовлетворена, поэтому нам не нужно ничего делать. Мы можем просто вернуться.
Warbo
8
Мне это нравится больше, на самом деле. Конечно, в зависимости от ограничений, заданных требованиями приложения, это может быть неправильно. Опять же, я абсолютный поклонник идемпотентных методов.
FP
6
@ Филипп, если мы думаем об этом, LoadMaterialимеет следующие ограничения: «После его вызова материал всегда загружается, и количество загружаемых материалов не уменьшается». Бросать исключение для третьего ограничения «Материал не может быть добавлен дважды» мне кажется глупым и нелогичным. Создание идемпотентной функции уменьшает зависимость кода от порядка выполнения и состояния приложения. Это похоже на двойную победу для меня.
Бенджамин Грюнбаум
7
Мне нравится эта идея, но я предлагаю одно небольшое изменение: вернуть логическое значение, отражающее, было ли что-либо загружено. Если вызывающий действительно заботится о логике приложения, он может проверить это и выдать исключение.
user949300
8

Фундаментальный вопрос, который вы должны задать: каким должен быть интерфейс для вашей функции? В частности, является ли условие _Materials.ContainsKey(name)предварительным условием функции?

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

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

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

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

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

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

Для подробного объяснения проблем с классическим защитным подходом, а также возможной реализацией проверок предварительных условий в стиле assert, обратитесь к докладу Джона Лакоса « Защитное программирование, выполненное прямо из CppCon 2014» ( слайды , видео ).

ComicSansMS
источник
4

Здесь уже есть несколько ответов, но вот ответ, который учитывает Unity3D (ответ очень специфичен для Unity3D, я сделал бы большую часть этого совсем по-другому в большинстве контекстов):

В общем, Unity3D не использует исключения традиционно. Если вы выбросите исключение в Unity3D, это не будет похоже на обычное приложение .NET, а именно, оно не остановит программу, в большинстве случаев вы можете настроить редактор на паузу. Это просто будет зарегистрировано. Это может легко перевести игру в недопустимое состояние и создать каскадный эффект, который затруднит отслеживание ошибок. Так что я бы сказал, что в случае с Unity разрешение Addбросить исключение является особенно нежелательным вариантом.

Но при рассмотрении скорости исключений в некоторых случаях не бывает преждевременной оптимизации из-за того, как Mono работает в Unity на некоторых платформах. На самом деле Unity3D на iOS поддерживает некоторые дополнительные оптимизации сценариев, и отключенные исключения * являются побочным эффектом одного из них. Это действительно то, что нужно учитывать, потому что эти оптимизации оказались очень ценными для многих пользователей, демонстрируя реалистичный пример рассмотрения вопроса об ограничении использования исключений в Unity3D. (* управляемые исключения из движка, а не ваш код)

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

Другой подход, который я рассмотрю, на самом деле не указывает на ошибку в том, что касается вызывающего, а скорее использует Debug.LogXXфункции. Таким образом, вы получаете то же поведение, что и создание необработанного исключения (из-за того, как Unity3D обрабатывает их), не рискуя поместить что-то в странное состояние. Также подумайте, действительно ли это ошибка (Попытка загрузить один и тот же материал дважды обязательно ошибка в вашем случае? Или это может быть Debug.LogWarningболее подходящим случаем ).

А что касается использования таких вещей, как Debug.LogXXфункции, а не исключения, вам все равно нужно учитывать, что происходит, когда исключение будет выдано из чего-то, что возвращает значение (например, GetMaterial). Я склоняюсь к этому, передавая ноль вместе с записью ошибки ( опять же, только в Unity ). Затем я использую нулевые проверки в моем MonoBehaviors, чтобы гарантировать, что любая зависимость, например материал, не является нулевым значением, и отключаю MonoBehavior, если это так. Примером простого поведения, которое требует нескольких зависимостей, является что-то вроде этого:

    public void Awake()
    {
        _inputParameters = GetComponent<VehicleInputParameters>();
        _rigidbody = GetComponent<Rigidbody>();
        _rigidbodyTransform = _rigidbody.transform;
        _raycastStrategySelector = GetComponent<RaycastStrategySelectionBehavior>();

        _trackParameters =
            SceneManager.InstanceOf.CurrentSceneData.GetValue<TrackParameters>();

        this.DisableIfNull(() => _rigidbody);
        this.DisableIfNull(() => _raycastStrategySelector);
        this.DisableIfNull(() => _inputParameters);
        this.DisableIfNull(() => _trackParameters);
    }

SceneData.GetValue<>похож на ваш пример в том, что он вызывает функцию в словаре, которая выдает исключение. Но вместо того, чтобы вызвать исключение, оно использует Debug.LogErrorтрассировку стека, как обычное исключение, и возвращает ноль. Следующие проверки * отключат поведение, а не позволят ему продолжать существовать в недопустимом состоянии.

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

Селали Адобор
источник
1
+1 за добавление в колоду действительно новой информации. Я не ожидал этого.
Ixrec
3

Мне нравятся два основных ответа, но я хочу предложить, чтобы ваши имена функций могли быть улучшены. Я привык к Java, поэтому YMMV, но метод «добавить» должен, IMO, не генерировать исключение, если элемент уже существует. Он должен снова добавить элемент или, если местом назначения является Set, ничего не делать. Поскольку это не поведение Materials.Add, его следует переименовать в TryPut, AddOnce, AddOrThrow или аналогичные.

Аналогично, ваш LoadMaterial должен быть переименован в LoadIfAbsent или Put или TryLoad или LoadOrThrow (в зависимости от того, используете ли вы ответ № 1 или № 2) или что-то подобное.

Следуйте соглашениям C # Unity по присвоению имен, какими бы они ни были.

Это будет особенно полезно, если у вас есть другие функции AddFoo и LoadBar, где допустимо загружать одно и то же дважды. Без четких имен разработчики будут разочарованы.

user949300
источник
Существует различие между семантикой C # Dictionary.Add () (см. Msdn.microsoft.com/en-us/library/k7z0zy8k%28v=vs.110%29.aspx ) и семантикой Java Collection.add () (см. Документы). oracle.com/javase/8/docs/api/java/util/… ). C # возвращает void и выдает исключение. Принимая во внимание, что Java возвращает bool и заменяет ранее сохраненное значение новым значением или выдает исключение, когда новый элемент не может быть добавлен.
Каспер ван ден Берг
Каспер - спасибо и интересно. Как заменить значение в словаре C #? (Эквивалентен Java put ()). Не вижу встроенного метода на этой странице.
user949300
хороший вопрос, можно сделать Dictionary.Remove () (см. msdn.microsoft.com/en-us/library/bb356469%28v=vs.110%29.aspx ), за которым следует Dictionary.Add () (см. msdn.microsoft .com / en-us / library / bb338565% 28v = vs.110% 29.aspx ), но это немного неловко.
Каспер ван ден Берг
@ user949300 dictionary [key] = значение заменяет запись, если она уже существует
Rupe
@Rupe и, по-видимому, бросает что-то, если это не так. Все кажется мне очень неловким. Большинство клиентов Наладка Dict не волнует wheteher запись была там, они просто заботятся , что после их установки кода, то есть там. Оценка один для API коллекций Java (IMHO). PHP также неуклюжий с диктовками, для C # было ошибкой следовать этому прецеденту.
user949300
3

Все ответы добавляют ценные идеи, я хотел бы объединить их:

Определите предполагаемую и ожидаемую семантику LoadMaterial()операции. По крайней мере, существуют следующие варианты:

  • Предпосылкой на nameLoadedMaterials : →

    Когда предварительное условие нарушается, то эффект LoadMaterial()не определен (как в ответ по ComicSansMS ). Это дает большую свободу в реализации и будущих изменениях LoadMaterial(). Или же,

  • Эффекты вызова LoadMaterial(name)с nameLoadedMaterials указаны; или:

    • спецификация гласит, что выдается исключение; или же
    • спецификация утверждает , что результат является тождественным (как в ответе на Карле Билефельдт ),

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

  • Бросьте пользовательское исключение (как предложено на Ixrec ) →

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

    • Для того, чтобы избежать расходов , постоянно проверяя nameLoadedMaterials вы можете следовать Марк ван Лиувен в советуют:

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

  • Пусть Dictionay.Add сгенерирует исключение →

    Код выбрасывания исключений является избыточным. - Джон Рейнор

    Хотя большинство избирателей больше согласны с Ixrec

    Дополнительной причиной для выбора этой реализации являются:

    • что вызывающие могут уже обрабатывать ArgumentExceptionисключения, и
    • чтобы избежать потери информации стека.

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

  • Сделать LoadMaterial()идемпотентные в anwser по Карлу Билефельдту и upvoted чаще всего ( в 75 раз).

    Варианты реализации для этого поведения:

    • Проверьте с Dictionary.ContainsKey ()

    • Всегда вызывайте Dictionary.Add (), перехватывая ArgumentExceptionего, когда ключ для вставки уже существует, и игнорируйте это исключение. Документируйте, что игнорирование исключения - это то, что вы намерены делать и почему.

      • Когда LoadMaterials()это (почти) всегда вызывается один раз для каждого name, это позволяет избежать затрат , постоянно проверяя nameLoadedMaterials ср Марк ван Леувен . Тем не мение,
      • когда LoadedMaterials()часто вызывается несколько раз для одного и того же name, это влечет за собой (дорогие) затраты на бросание ArgumentExceptionи разматывание стека.
    • Я думал, что существует TryAddметод -метода TryGet (), который позволил бы вам избежать дорогостоящего выброса исключений и разматывания стека неудачных вызовов Dictionary.Add.

      Но этот TryAddметод, кажется, не существует.

Каспер ван ден Берг
источник
1
+1 за самый полный ответ. Это, вероятно, выходит далеко за рамки того, что было изначально после ОП, но это полезная сводка всех опций.
Ixrec
1

Код выбрасывания исключений является избыточным.

Если вы позвоните:

_Materials.Add (name, Resources.Load (string.Format ("Materials / {0}", name)) как материал

с тем же ключом он бросит

System.ArgumentException

Сообщение будет: «Элемент с тем же ключом уже был добавлен».

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

В противном случае, я бы, вероятно, реорганизовал охрану в этом случае.

Джон Рейнор
источник
0

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

Каков ожидаемый результат (контракт) звонка:

LoadMaterial("wood");

Если вызывающая сторона может ожидать / гарантируется, что после вызова этого метода загружается материал "wood", то я не вижу причин для исключения, если этот материал уже загружен.

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

oɔɯǝɹ
источник
-2

Почему бы не изменить метод, чтобы он возвращал «true», если материал был добавлен, и «false», если это не так?

private bool LoadMaterial(string name)
{
   if (_Materials.ContainsKey(name))

    {

        return false; //already present
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );

    return true;

}
MrIveck
источник
15
Функции, которые возвращают значения ошибок, являются именно тем антишаблоном, исключения которого должны быть устаревшими.
Филипп
Это всегда так? Я думал, что это было только в случае с полу-предикатом ( en.wikipedia.org/wiki/Semipredicate_problem ), потому что в примере с OP-кодом невозможность добавить материал в систему не представляет реальной проблемы. Цель метода - убедиться, что материал находится в системе при его запуске, и это именно то, что делает этот метод. (даже если он потерпит неудачу) Возвращаемое логическое значение просто указывает, пытался ли метод уже добавить этот материал или нет. PS: я не принимаю во внимание, что может быть несколько материалов с одинаковым именем.
Мистер Айвек
1
Я думаю, что нашел решение сам: en.wikipedia.org/wiki/… Это указывает на то, что ответ Ixrec является наиболее правильным
MrIveck
@Philipp В случае, если кодер / спецификация решили, что нет ошибки при попытке дважды загрузить. В этом смысле возвращаемое значение не является значением ошибки, а является информационным.
user949300