Throw exception or let code fail

Question

I am wondering if there are any pros and cons against this style:

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
    );
}

That method should, for each name, be run only once. _Materials.Add() will throw an exception if it will be called multiple times for the same name. Is my guard, as a result, completely redundant, or there are some less obvious benefits?

That's C#, Unity, if anyone is interested.

What happens if you load a material twice without the guard? Does _Materials.Add throw an exception? — user253751, Jan 22 '15 at 23:49
Yes. I should have probably mentioned: _Materials is a dictionary. — async, Jan 23 '15 at 06:40
Side notes: 1) Consider using string.Format over string concatenation for building the exception message. 2) only use as if you expect the cast to fail and check the result for null. If you expect to always get a Material, use a cast like (Material)Resources.Load(...). A clear cast exception is easier to debug than a null reference exception that occurs later on. — CodesInChaos, Jan 23 '15 at 09:05
Personally I would use a assertion here, that still gives a exception when you try to debug this, but wont take up any resources when you build a release. Unless you load materials from a arbitrary source at runtime, then better keep the exception. — Dorus, Jan 23 '15 at 09:21
It's generally best practice to use the ArgumentException constructor that specifies a parameter name. Even though there's only one parameter in this case, it's a good habit to form. — Nick, Jan 23 '15 at 21:22
In this specific case, your callers may also find a companion LoadMaterialIfNotLoaded or ReloadMaterial (this name could use improvement) method useful. — jpmc26, Jan 24 '15 at 00:16
This might be nit picky but shouldn't it be an InvalidOperationException instead of an ArgumentException? The argument is fine it's the state (the material is already loaded) which invalidates the call. — Roman Reiner, Jan 24 '15 at 04:10
It's probably best to leave that discussion for somewhere else, but that's basically the same exception that gets thrown by Dictionary.Add() — async, Jan 24 '15 at 07:29
That's a poor reason for the exception. Effectively you are propagating an internal implementation feature, if that is the only reason. — itsbruce, Jan 24 '15 at 13:46
On another note: This pattern is ok for sometimes adding an item to a list. Do not however use this pattern for filling an entire list because you will run into a O(n^2) situation where with large lists you will run into performance issues. You won't notice it at 100 entries but at 1000 entries you certainly and at 10.000 entries it will be a major bottleneck. — Pieter B, Jan 25 '15 at 13:06
I'd probably remove the guard and do the Add in a try catch, where I throw whatever exception with a good message, and use the Adds exception as my exceptions inner. — Andy, Jan 26 '15 at 00:26
As a rule of thumb, I like to "throw custom exceptions at the shallowest point of the call stack where the origin of such exception can be distinguished". — Kroltan, Jan 26 '15 at 13:15
@Den based on the calls I'd this is Unity3D. To the poster, I think you should keep in mind that on occasion Unity3D practices do clash a bit with what general practices (and sometimes for reasons not strictly related to general game programming, but rather "quirks" specific to Unity3D as an engine). I'd say this might be a borderline case of that. — Selali Adobor, Jan 26 '15 at 17:39

score 110 · Accepted Answer · answered Jan 22 '15 at 23:40

110

The benefit is that your "custom" exception has an error message that's meaningful to anyone calling this function without knowing how it's implemented (which in the future might be you!).

Granted, in this case they'd probably be able to guess what the "standard" exception meant, but you're still making it clear that they violated your contract, rather than stumbled across some strange bug in your code.

answered Jan 22 '15 at 23:40

Ixrec

27,771

3

Agree. It is nearly always better to throw an exception for a precondition violation. – Frank Hileman Jan 22 '15 at 23:53
22

"The benefit is that your "custom" exception has an error message that's meaningful to anyone calling this function without knowing how it's implemented (which in the future might be you!)."
Top advice.
– async Jan 23 '15 at 08:40
2

"Explicit is better than implicit." -Zen of Python – jpmc26 Jan 24 '15 at 00:18
4

Even if the error thrown by _Materials.Add is not the error you want to transmit to the caller, I think this method of relabelling the error is inefficient. For every successful call of LoadMaterial (normal operation) you will have done the same sanity test twice, in LoadMaterial and then again in _Materials.Add. If more layers are wrapped, each employing the same style, you can even have many more times the same test. – Marc van Leeuwen Jan 24 '15 at 09:01
15

... Instead consider plunging into _Materials.Add unconditionally, then catch a potential error and in the handler throw a different one. The extra work is now only done in the erroneous cases, the exceptional execution path where you don't really worry about efficiency at all. – Marc van Leeuwen Jan 24 '15 at 09:02
2

Doing the extra check can perhaps add a race condition if _Materials can be modified by more than one thread. I like the suggestion that @MarcvanLeeuwen gives. – Matthew Jan 24 '15 at 16:58
1

@MarcvanLeeuwen this is exactly what I would normally do and in fact was going to suggest before I saw your comment. However I just realised that the Resources.Load call is probably way more expensive than ContainsKey, so depending on how often you expect the addition to fail vs. succeed I might go with the code as written. On a slightly unrelated note, one could opt to have a LoadMaterial and a TryLoadMaterial function. – CompuChip Jan 25 '15 at 15:10
@CompuChip: The relative expense of Contains.Key and Resources.Load is of no importance to my consideration. The call to Contains.Key will only prevent the call to Resources.Load in the erroneous case that the same material is loaded more than once; in that execution path there is an error stop, so efficiency is not important. However when all is well, both calls will be needed in the code proposed. – Marc van Leeuwen Jan 25 '15 at 15:35
@MarcvanLeeuwen Why is efficiency not important in the case where there is an error? I don't think that's necessarily true at all. If in the error case the program will display an error message to the user and stop, while in the success case it does a bunch of additional work, it might be relatively less important. But at this level you don't know that. Maybe when there's an error the caller selects a different NAME and tries again, and keeps trying different names until if finds one that's a success -- for example in a function that's trying to assign a unique name. – Jay Jan 25 '15 at 22:34
@MarcvanLeeuwen RE the potential race condition: True. I find that a more persuasive argument. – Jay Jan 25 '15 at 22:35
Leave it alone if it follows the rules and doesn't hurt any. Why all the harassments? – InformedA Feb 15 '15 at 13:23

score 99 · Answer 2 · edited Apr 12 '17 at 07:31

99

I agree with Ixrec's answer. However, you might want to consider a third alternative: making the function idempotent. In other words, return early instead of throwing an ArgumentException. This is often preferable if you would otherwise be forced to check if it has already been loaded before calling LoadMaterial every time. The fewer preconditions you have, the less cognitive load on the programmer.

Throwing an exception would be preferable if it's truly a programmer mistake, where it should be obvious and known at compile time if the material has already been loaded, without having to check at runtime before calling.

edited Apr 12 '17 at 07:31

Community

1

answered Jan 23 '15 at 03:25

Karl Bielefeldt

147,435

2

On the other hand, having a function fail silently can cause unexpected behavior resulting in harder to find bugs occuring later. – Philipp Jan 23 '15 at 10:01
30

@Philipp the function would not fail silently. Being idempotent means that running it many times has the same effect as running it once. In other words, if the material is already loaded, then our spec ("the material should be loaded") is already satisfied, so we don't need to do anything. We can just return. – Warbo Jan 23 '15 at 10:51
8

I like this more, actually. Of course, depending on the constraints given by the applications requirements, this might be wrong. Then again, I am an absolute fan of idempotent methods. – F.P Jan 23 '15 at 11:35
6

@Philipp if we think about it LoadMaterial has the following constraints: "After calling it the material is always loaded and the number of materials loaded did not decrease". Throwing an exception for a third constraint "The material cannot be added twice" seems silly and counterintuitive to me. Making the function idempotent reduces the code's reliance on execution order and application state. It seems like a double win for me. – Benjamin Gruenbaum Jan 23 '15 at 13:19
@Warbo: Being idempotent means that running it many times with the same parameters has the same effect as running it once. So if you call LoadMaterial("foo") and then LoadMaterial("bar") you should expect the code to use bar, not foo. – l0b0 Jan 23 '15 at 15:09
7

I like this idea, but suggest one minor change: return a boolean reflecting whether anything was loaded. If caller really cares about application logic, they can check that and throw an exception. – user949300 Jan 23 '15 at 16:46
Whether this is the correct strategy or not, really depends on the overall design of the system. A true precondition means that something will go wrong in the future, if that condition is not met. – Frank Hileman Jan 23 '15 at 17:30
@l0b0 yes, I left it implicit that the parameters must be the same. Thanks for clarifying for those who may haven't come across the concept before :) Of course, in this case, it doesn't matter too much, since the if (_Materials.ContainsKey(name)) line will spot our previous parameters, and those of any other loading mechanism that's been used. – Warbo Jan 24 '15 at 12:08
FWIW, I agree with the idempotent approach. From the perspective of somebody calling this method, I have to check if the map contains key N, which is redundant, because the method already does it. Or, I have to catch an exception. I guess it depends on if it's considered a mistake in your design to call this twice for the same parameters. My preference is to handle the case rather than throw an exception, which will then need to be handled anyway. – Brandon Jan 24 '15 at 17:05
If I needed to call this method and had to deal with this exception case, it would definitely annoy me. After I write my own code to check if it contains the key, I would be sad to then find that the method checks it anyway. My if statement would be for the sole purpose of avoiding the exception. And if you declared it a checked exception, I would need an if statement and a catch clause. – Brandon Jan 24 '15 at 17:07

score 8 · Answer 3 · answered Jan 23 '15 at 15:27

The fundamental question that you have to ask is: What do you want the interface for your function to be? In particular, is the condition _Materials.ContainsKey(name) a precondition of the function?

If it is not a precondition, the function must provide a well-defined result for all possible vales of name. In this case, the exception thrown if name is not part of _Materials becomes part of the function's interface. That means it needs to be part of the interface documentation and if you ever decide to change that exception in the future, that will be a breaking interface change.

The more interesting question is what happens if it is a precondition. In that case the precondition itself becomes part of the function interface, but the way that a function behaves when this condition is violated is not necessarily part of the interface.

In this case, the approach that you posted, checking for precondition violations and reporting the error, is what is known as defensive programming. Defensive programming is good in that it notifies the user early when they made a mistake and called the function with a bogus argument. It is bad in that it significantly increases the maintenance burden, as user code might depend that the function handles precondition violations in a certain way. In particular, if you find that the runtime check ever becomes a performance bottleneck in the future (unlikely for such a simple case, but quite common for more complex preconditions), you might no longer be able to remove it.

It turns out that those disadvantages can be very significant, which gave defensive programming kind of a bad reputation in certain circles. However, the initial goal is still valid: We want to users of our function to notice early when they did a mistake.

Therefore many developers nowadays propose a slightly different approach for these kinds of issues. Instead of throwing an exception, they use an assert-like mechanism for checking preconditions. That is, preconditions may get checked in debug builds to assist users in catching mistakes early on, but they are not part of the functions interface. The difference may seem subtle at first glance, but it can make a huge difference in practice.

Technically, calling a function with violated preconditions is undefined behavior. But the implementation might decide to detect those cases and notify the user right away if that happens. Exceptions are unfortunately not a good tool to implement this, as user code can react on them and thus might start relying on their presence.

For a detailed explanation of the problems with the classic defensive approach, as well as a possible implementation for assert-style precondition checks, refer to John Lakos' talk Defensive Programming Done Right from CppCon 2014 (Slides, Video).

score 4 · Answer 4 · edited Apr 12 '17 at 07:31

There are already a few answers here, but here's answer that takes Unity3D into account (the answer is very specific to Unity3D, I'd do most of this very differently in most contexts):

In general, Unity3D doesn't use exceptions traditionally. If you throw an exception in Unity3D, it's nothing like in your average .NET application, namely it won't stop the program, t most you can configure the editor to pause. It'll just get logged. This can easily put the game in an invalid state and create a cascade effect that makes errors difficult to track down. So I'd say in the case of Unity, letting Add throw an exception is an especially undesirable option.

But on the examining the speed of exceptions isn't a case of premature optimization in some cases because of how Mono works in Unity on some platforms. In fact, Unity3D on iOS supports some advanced script optimizations, and disabled exceptions* is a side effect of one of them. This is really something to consider because those optimizations haven proven very valuable for many users, showing a realistic case for considering limiting the use of exceptions in Unity3D. (*managed exceptions from the engine, not your code)

I'd say that in Unity you might want to take a more specialized approach. Ironically, a very down-voted answer at the time of writing this, shows one way I might implement something like this specifically in the context of Unity3D (elsewhere something like this really is unacceptable, and even in Unity it's fairly inelegant).

Another approach I'd consider is actually not indicating an error far as the caller is concerned, but rather using the Debug.LogXX functions. That way you get the same behavior as throwing an unhandled exception (because of how Unity3D handles them) without risking putting something in an strange state down the line. Also consider if this really is an error (Is trying to load the same material twice necessarily an error in your case? Or might this be a case where Debug.LogWarning is more applicable).

And in regards to the using things like Debug.LogXX functions instead of exceptions, you still have to consider what happens when an exception would be thrown from something that returns a value (like GetMaterial). I tend to approach this by passing null along with logging the error (again, only in Unity). I then use null checks in my MonoBehaviors to ensure any dependency like a material is not a null value, and disable the MonoBehavior if it is. An example for a simple behavior that requires a few dependencies is something like this:

    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<> is similar to your example in that it calls a function on a dictionary that throws an exception. But instead of throwing an exception it uses Debug.LogError which gives a stack trace like a normal exception would and returns null. The checks that follow* will disable the behavior instead of letting it continue to exist in an invalid state.

*the checks look like that because of a small helper I use that prints out a formatted message when it disables the game object**. Simple null checks with if work here (** the helper's checks are only compiled in Debug builds (like asserts). Using lambdas and expressions like that in Unity can take it's toll on performance)

+1 for adding some genuinely new information to the pile. I was not expecting that. — Ixrec, Jan 25 '15 at 23:40

user949300 · Answer 5 · 2015-01-24T00:37:57.403

3

I like the two main answers, but want to suggest that your function names could be improved. I'm used to Java, so YMMV, but an "add" method should, IMO, not throw an Exception if the item is already there. It should add the item again, or, if the destination is a Set, do nothing. Since that is not the behavior of Materials.Add, that should be renamed TryPut or AddOnce or AddOrThrow or similar.

Likewise, your LoadMaterial should be renamed LoadIfAbsent or Put or TryLoad or LoadOrThrow (depending on whether you use answer #1 or #2) or some such.

Follow the C# Unity naming conventions whatever they are.

This will be especially useful if you have other AddFoo and LoadBar functions where it is allowable to load the same thing twice. Without clear names the developers will get frustrated.

edited Jan 24 '15 at 00:37

answered Jan 24 '15 at 00:32

user949300

8,823

There is a difference betweeen C# Dictionary.Add() semantics (see https://msdn.microsoft.com/en-us/library/k7z0zy8k%28v=vs.110%29.aspx) and Java Collection.add() semantics (see http://docs.oracle.com/javase/8/docs/api/java/util/Collection.html#add-E-). C# returns void and throws exception. Whereas, Java returns bool and replaces the previously stored value with the new value or throws an exception when the the new element cannot be added. – Kasper van den Berg Jan 25 '15 at 11:01
Kasper - thanks, and interesting. How do you replace a value in a C# Dictionary? (Equivalent to a Java put()). Don't see a built in method on that page. – user949300 Jan 25 '15 at 18:20
good question, one could do Dictionary.Remove() (see https://msdn.microsoft.com/en-us/library/bb356469%28v=vs.110%29.aspx) followed by an Dictionary.Add() (see https://msdn.microsoft.com/en-us/library/bb338565%28v=vs.110%29.aspx), but this appears a bit awkward. – Kasper van den Berg Jan 26 '15 at 08:15
@user949300 dictionary[key] = value replaces the entry if it already exists – Rupe Jan 26 '15 at 12:52
@Rupe and presumably throws something if it doesn't. All seems very awkward to me. Most clients setting up the dict don't care wheteher an entry was there, they just care that, after their setup code, it is there. Score one for the Java Collections API (IMHO). PHP is also awkward with dicts, it was a mistake fot C# to follow that precedent. – user949300 Jan 26 '15 at 17:46
@user949300 No, it doesn't throw. It does exactly what you're saying you'd want it to. If you want to know if it's going to overwrite an existing entry you can check dictionary.ContainsKey(key). – Rupe Jan 27 '15 at 09:52

score 3 · Answer 6 · edited Apr 12 '17 at 07:31

All answers add valuable ideas, I would like to combine them:

Decide on the intended and expected semantics of the LoadMaterial() operation. At least the following options exist:

A precondition on name ∉ LoadedMaterials: →

When precondition is violated, the effect of LoadMaterial() is unspecified (as in answer by ComicSansMS). This allows to most freedom in the implementation and future changes of LoadMaterial(). Or,
effects of calling LoadMaterial(name) with name ∈ LoadedMaterials are specified; either:
- the specification states that an exception is thrown; or
- the specification states that the result is idempotent (as in answer by Karl Bielefeldt),
  - possibly returning a "has the collection changed" boolean (as proposed by User949300 and proposed (in some interpretations) by Mrlveck.

When having decided on the semantics, you must choose an implementation. The following options and considerations where proposed:

Throw a custom exception (as suggested by Ixrec) →

The benefit is that your "custom" exception has an error message that's meaningful to anyone calling this function — Ixrec and User16547
- To avoid the cost of repeatedly checking name ∉ LoadedMaterials you can follow Marc van Leeuwen's advise:
  
  ... Instead consider plunging into _Materials.Add unconditionally, then catch a potential error and in the handler throw a different one.
Let Dictionay.Add throw the exception →

The exception throwing code is redundant. — Jon Raynor

Although, most voters agree more with Ixrec

Additional reason to to choose this implementation are:
- that callers can already handle ArgumentException exceptions, and
- to avoid loosing stack information.
But if these two reasons are important you can also derive your custom exception from ArgumentException and use the original as a chained exception.
Make LoadMaterial() idempotent as anwser by Karl Bielefeldt and upvoted most often (75 times).
- possibly returning a "has the collection changed" boolean (as proposed by User949300 and proposed (in some interpretations) by Mrlveck.
The implementation options for this behaviour:
- Check with Dictionary.ContainsKey()
- Always call Dictionary.Add() catch the ArgumentException it throws when the key to insert already exists and ignore that exception. Document that the ignoring of the exception is what you intent to do and why.
  - When LoadMaterials() is (almost) always called once for each name, this avoids the cost repeatedly checking name ∉ LoadedMaterials cf. Marc van Leeuwen. However,
  - when LoadedMaterials() is often called multiple times for the same name, this incurs the (expensive) cost of throwing the ArgumentException and stack unwinding.
- I thought that there existed a TryAdd-method analogue to TryGet() which would have allowed you to avoid the expensive exception throwing and stack unwinding of failing calls to Dictionary.Add.
  
  But this TryAdd-method appears not to exist.

+1 for being the most comprehensive answer. This probably goes far beyond what the OP was originally after, but it's a useful summary of all the options. — Ixrec, Jan 25 '15 at 12:52

score 1 · Answer 7 · answered Jan 23 '15 at 20:42

The exception throwing code is redundant.

If you call:

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

with the same key it will throw a

System.ArgumentException

The message will be: "An item with the same key has already been added."

The ContainsKey check is redundant since essentially the same behavior is achieved without it. The only item that is different is the actual message in the exception. If there is true benefit from having a custom error message, then the guard code has some merit.

Otherwise, I would probably refactor out the guard in this case.

score 0 · Answer 8 · answered Jan 25 '15 at 23:56

I think this is more a question about API design than a coding convention question.

What is the expected result (the contract) of calling:

LoadMaterial("wood");

If the caller may expect / is guaranteed that after calling this method, the material "wood" is loaded, then i don't see any reason to throw an exception when that material is already loaded.

If an error occurs when loading the material, for example, the database connection could not be opened, or there is no material "wood" in the repository, then it is correct to throw an exception to notify the caller of that problem.

score -2 · Answer 9 · edited Jan 23 '15 at 16:15

-2

Why not change the method so it return 'true' it the matterial is added and 'false' if it wasn't?

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;

}

edited Jan 23 '15 at 16:15

gnat

21,213
29
113
291

answered Jan 23 '15 at 09:35

MrIveck

31

15

Functions which return error values is exactly the anti-pattern which exceptions are supposed to make obsolete. – Philipp Jan 23 '15 at 10:03
Is this alway's the case? I thought it was only the case with semipredicate (http://en.wikipedia.org/wiki/Semipredicate_problem) Because in OP code sample, failing to add a material to the system is no real problem. The method purpose it to make sure that the material is in the system when you run it, and that is exactly what this method does. (even if it fails) The returning boolean just indicates if the method already tried to add this matterial or not.
ps: I don't take into consideration that there could be multiple matterials with the same name.
– MrIveck Jan 23 '15 at 12:42
1

I think I found the solution myself: http://en.wikipedia.org/wiki/Exception_handling#Exception_handling_based_on_design_by_contract This points out that Ixrec's answer is the most correct – MrIveck Jan 23 '15 at 13:48
@Philipp In the case the coder/spec has decided that there is no error when you try to Load twice. The return value is, in that sense, not an error value but an informational one. – user949300 Jan 26 '15 at 17:54

Throw exception or let code fail

9 Answers9

Linked

Related