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.

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.

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

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)

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.

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.

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.

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.

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;

}