Fetching detail for a given ID, which might not exist

Question

I'm actually working on a piece of code which use the following code:

public Detail GetDetail(int id)
{
    if (!_detail.ContainsKey(id))
    {
        GetDetailForNewObjects(id);
    }

    return _detail[id].Data;
}

The last line throws a NullReferenceException. Since it should not happen that this method is called in the way it returns an exception they propose not to catch it and rethrow a more specific and descriptive one.

I don't agree. The software readability and the maintainability of the cose falls dramatically when we get a NullReferenceException.

How would you write that piece of code?

Answer 1

In this case, I would suggest that this exception should be classified as a 'Boneheaded' exception as defined by Eric Lippert because there is no reason why the code should throw the exception at all. Exceptions are expensive, as is handling them so I would make every effort to have code that only has them in TRULY exceptional circumstances.

It seems to me like the reason the _detail[id] throws a NullReferenceException is because the GetDetailForNewObjects method had a problem which could be in many forms, but I'll outline two below.

---

This problem could be one of

• Your DB Query (assuming that is what is happening) couldn't find any records.

Solution: return a null object. Don't try to access something you know has a reasonable possibility of being null, instead check again for null and return null if no object exists after the call. Let the calling code figure out what to do with a null (maybe search results are null, this is not an exception).

• Your DB had a connection issue / timeout

Solution: Again, THIS is the exception that should be passed up. We shouldn't handle a DB exception (since there it typically nothing that can be done about it), just to throw a null reference exception. This behavior would make debugging more difficult since it hides the actual DB connection issue. Bubble the DB exception upwards so the code / developer can respond to that, instead of an ambiguous NullReferenceException because ultimately, we need to know WHY it was null since clearly we weren't expecting it to be.

In both cases, we as developers know how to handle the situation (given the assumptions I laid out).

I would write the above code something like this:

public Detail GetDetail(int id) {
    if(id < 0) {
        throw new ArgumentException("id", "Argument should have a positive value.");
    } // end if
    Detail returnValue = null; // value store that is only assigned if the call succeeds
    if (!_detail.ContainsKey(id)) {
        GetDetailForNewObjects(id); // assuming this adds the key to the dictionary.
        if(_detail.ContainsKey(id)) { //Cheaper and safer than handling an exception
            returnValue = _detail[id].Data;
        } // end if
    } // end if

    return returnValue;
} // end function GetDetail

Eric Lippert has a great description of when / how to handle different types of exceptions.

Further reading on Stack Exchange:

• How to avoid throwing vexing exceptions?
• Is catching 'expected' exceptions that bad?

Answer 2

I would leave this code as-is.

I feel in this case we should propagate a NullReferenceException as that does actually accurately describe the error that has occurred. You would gain little from wrapping this exception in another exception. The only exception (heh) in my opinion would be if you wanted to wrap this exception in a domain-specific exception (new MyLibraryException(ref)) but this is debatable even then. By using a null reference exception you clearly signal to the programmer that the line on which it failed had a null reference exception (in this case, it is obvious that _detail[id] is null - in the case of _detail[id] being out of bounds, we would throw an ArrayOutOfBoundsException).

In other words, there's little need for you to 'wrap' managed exceptions. Most competent programmers will understand them, rather than your domain specific ones.