Should I throw my own ArgumentOutOfRangeException or let one bubble up from below?

Question

I have a class that wraps List<>

I have GetValue by index method:

    public RenderedImageInfo GetValue(int index)
    {
        list[index].LastRetrieved = DateTime.Now;
        return list[index];
    }

If the user requests an index that is out of range, this will throw an ArgumentOutOfRangeException .

Should I just let this happen or check for it and throw my own? i.e.

    public RenderedImageInfo GetValue(int index)
    {
        if (index >= list.Count)
        {
            throw new ArgumentOutOfRangeException("index");
        }
        list[index].LastRetrieved = DateTime.Now;
        return list[index];
    }

In the first scenario, the user would have an exception from the internal list, which breaks my OOP goal of the user not needing to know about the underlying objects.

But in the second scenario, I feel as though I am adding redundant code.

Edit:
And now that I think of it, what about a 3rd scenario, where I catch the internal exception, modify it, and rethrow it?

Answer 1

It's fine to let the exception bubble up in this case.

edit

I've been asked to update my answer to include some of the responses...

Arguably, catching the exception and rethrowing it as an inner is more robust, in that it insulates you from changes and creates a more consistent interface. Having said that, the benefit can be very small -- bordering on none -- in an environment where the developer has access to all the code. Moreover, whatever small benefit we obtain is not free. The additional error-handling code adds complexity, and must itself be tested to ensure that it works. It also adds to the risk of breaking the method during maintenance, in that the basic functionality of the method could be obscured by all of the bullet-proofing.

So, is it worth it? It depends. It comes down to cost/benefit analysis, which can only be done for a specific context. For a public API, such as AlphaFS, the benefits may well exceed the costs. For internal use on code that is not designed for significant reuse, it probably isn't. That's the context of my original answer, which I still stand by. Also, as Hightechrider pointed out, there may be other technologies, such as coding contracts, which yield much the same benefit but at a lower cost, changing the equation in favor of robustness.

As always, I am open to reasoned disagreement. However, I suggest that we all try to avoid one-size-fits-all thinking.

edit

Just to make it clear that I'm not crazy when I say that error-handling code is error-prone, take a look at the original question and tell me if you can spot the bug. I offer a +1 if you do.

Answer 2

You should throw your own, for a couple of reasons:

1. You can explicitly set the appropriate parameter name in the constructor. This way, the exception has the appropriate parameter information for the Argument that is out of range.
2. (Minor) The internal list's exception will have an invalid stack trace as far as your user is concerned. By constructing a new exception, you can get the stack trace showing your method as being the one that's inappropriate, which will be easier for your user's debugging.

As for catching and modifying the internal exception - I wouldn't recommend this, necessarily. If the exception is one where the extra information would potentially be useful, you should use the InnerException of your new exception to propogate this information up.

In this example (ArgumentOutOfRangeException), the fact that there is an internal list should be kept as an implementation detail, and there's no reason for your user to see that information.

Answer 3

If you're on .NET 2.0 or above, you can use the inner exception, like so:

public RenderedImageInfo GetValue(int index)
{
    try {
        list[index].LastRetrieved = DateTime.Now;
        return list[index];
    } catch (ArgumentOutOfRangeException ex) {
        throw new ArgumentOutOfRangeException("index", ex);
    }
}

That way, your user will primarily see your exception, but it is possible to dig deeper if you (or they) want to.

Edit: I see now that InnerException has been supported since .NET 1.0, it's just that constructor that is new. I see no way to actually set the InnerException for an ArgumentOutOfRangeException in .NET before 2.0. Did they just forget it, or is the code I wrote above against the intended use?

Answer 4

...breaks m[y] OOP goal of the user not needing to know about the underlying objects.

Throwing the same exception does nothing for encapsulation. Either your (implied/documented - since we don't have checked exceptions or DbC) contract states that you'll throw an ArgumentOutOfRange exception in this case or not. How that exception is generated, and what it's stack trace looks like is irrelevant to a caller.

If you move your internal implementation to something that doesn't throw an ArgumentOutOfRange exception, then you'll need to throw one yourself to fulfill your contract (or do a breaking change to the contract).

Stack traces (and the param name) is for the guy debugging - not for programmatic access. Unless there's some security concern, there's no worry to letting them "leak".

BTW, the advice (which you may be thinking of) of wrapping exceptions comes from a more abstract scenario. Consider an IAuthenticationService that throws if a user can't Login,

If there's both an LdapAuthenticationService and a DatabaseAuthenticationService implementation, then you'd have to catch both LdapDirectoryException and SqlException to determine a failed login. When a 3rd implementation is done, you'd need to add it's specific exception types as well. By all implementers wrapping their specific exception in a FailedAuthenticationException, you only have the single type to worry about.

It'd still be a good idea to include the original exception in InnerException though, since it aids in debugging.

I hope you can see the difference between this scenario and yours - in your case, you're just throwing the same exception type so there's no difference.

All that being said, if it was for a library - then I'd check for it and throw. But not for OOP purity - but because it makes the contract explicit and less likely to be changed inadvertently. If I was using [T|B]DD, then I'd just write a test for it and let the List throw instead - the test makes the contract explicit.