# 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?

• Is it possible that you actually need a line: _detail.Add(id, GetDetailForNewObjects(id)) Without knowing what the GetDetail... method does, it's hard to say. Oct 6, 2014 at 19:20
• Assert is handy when we know a thing will never happen, but I'm not sure I'd use it here. Jul 24, 2015 at 9:33

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

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


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