Re-throwing the exception of a wrapped object

Question

I use ConcurrentDictionary<TKey, TValue> to implement a ConcurrentSet<T>.

public class ConcurrentSet<T> : ISet<T>
{
    private readonly ConcurrentDictionary<T, byte> collection;
}

ConcurrentDictionary<TKey, TValue> can't contain a pair with a null key.

// summary, param, returns...
/// <exception cref="ArgumentNullException">
///     <paramref name="item" /> is null.
/// </exception>
public bool Add(T item)
{
    // This throws an argument null exception if item is null.
    return this.collection.TryAdd(item, 0);
}

So, should I,

if (item == null)
    throw new ArgumentNullException("item", "Item must not be null.");

return this.collection.TryAdd(item, 0);

Or, should I,

try
{
    return this.collection.TryAdd(item, 0);
}
catch (ArgumentNullException)
{
    throw new ArgumentNullException("item", "Item must not be null.");
}

Or, should I,

try
{
    return this.collection.TryAdd(item, 0);
}
catch (ArgumentNullException x)
{
    // I know, but I don't want to preserve the stack trace
    // back to the underlying dictionary, anyway.
    throw x;
}

Or, should I,

try
{
    return this.collection.TryAdd(item, 0);
}
catch (ArgumentNullException)
{
    // The thrown exception will have "key", instead of
    // "item" as the parameter's name, in this instance.
    throw;
}

What is the proper way to do this?

Answer 1

I would go with either this

public bool Add(T item)
{
    // This throws an argument null exception if item is null.
    return this.collection.TryAdd(item, 0);
}

or this

if (item == null)
    throw new ArgumentNullException("item", "Item must not be null.");

return this.collection.TryAdd(item, 0);

It depends on whether or not your class cares if there is a null.

If the only reason you are performing the null check is to avoid passing the null to TryAdd, then don't bother checking. TryAdd will do it's own checking and throw the exception.

If at some point you think you might use a different collection that allows null, but you still want your collection to not have nulls, then you should check yourself. This will protect you should that change happen at a future point in time.

Validation of parameters should always be the first thing a method does. There is no point in doing anything else if the parameters are invalid.

You should only catch an exception if you are going to do something with it. If you are just going to re-throw, or create a new expression that is equivalent, then don't bother catching it.

Answer 2

What you should do depends upon what you want to document your class as doing. If you wish to document that an attempt to add a null item may fail in unspecified fashion, then simply make the call directly and let any exception bubble up. If you wish to document that you will return an ArgumentNullException with ParamName equal to item and do not wish to rely upon what the behavior of ConcurrentDictionary when it receives a null key, then you should check the argument before passing it to the ConcurrentDictionary. If you wish to document that your code will throw an ArgumentNullException with ParamName equal to item, but are willing to rely upon the ConcurrentDictionary to validate its arguments and throw an ArgumentException, and if performance is critical, another possibility would be:

try
{
    return this.collection.TryAdd(item, 0);
}
catch (ArgumentNullException ex)
{
    if (ex.ParamName == "key" && item == null)
        throw new ArgumentNullException("item", "Item must not be null.");
    else
        throw;
}

This code avoids any extra cost for parameter validation in the scenario where the parameter is not null (which should 99.9999% of the time be the case) but will nonetheless ensure that it will only claim to be the source of an ArgumentNullException in the scenario where such an exception occurs for the expected reason; in the event that a bug in ConcurrentDictionary causes it to accidentally pass a null argument to a method it calls internally even when it is given a non-null item to add, the above code will ensure that the original exception stack trace is not lost. Note that another possibility might be:

    if (ex.ParamName == "key" && item == null)
        throw new ArgumentNullException("item", "Item must not be null.");
    else
        throw new UnexpectedException(ex); // Probably a custom type

The basic idea being that if an ArgumentNullException escapes from ConcurrentDictionary.Add for some reason other than item being null, such an exception should not be caught by code that might expect an ArgumentNullException from you.