52

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.

  • 3
    What happens if you load a material twice without the guard? Does _Materials.Add throw an exception? – immibis Jan 22 '15 at 23:49
  • 2
    Actually I have already mentioned in throws an exception :P – async Jan 23 '15 at 7:24
  • 9
    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 9:05
  • 3
    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 0:16
  • 1
    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
109

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.

  • 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 8:40
  • 2
    "Explicit is better than implicit." -Zen of Python – jpmc26 Jan 24 '15 at 0: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 9: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 9:02
100

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.

  • 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. – Florian Peschka 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
  • 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
8

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).

4

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
    +1 for adding some genuinely new information to the pile. I was not expecting that. – Ixrec Jan 25 '15 at 23:40
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.

3

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 nameLoadedMaterials: →

    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 nameLoadedMaterials 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),

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 nameLoadedMaterials 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).

    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 nameLoadedMaterials 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
    +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
1

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.

0

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.

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

}
  • 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 (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: en.wikipedia.org/wiki/… 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

protected by gnat Jan 24 '15 at 8:07

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.