Fetching detail for a given ID, which might not exist

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?

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?

StackExchange Code Review Q#64868, answer score: 11