gwerren.com

Programming notes and tools...

Fail Early In Code

Tue, 09 Jan 2018 ExceptionsC#

When writing code we should always consider error cases, this is a good thing. When attempting to handle an error case however we need to ensure that we are not simply delaying the error rather than solving the problem.

Let's have an example:

private static Person GetOldestPerson(IList<Person> people)
{
    if (people.Count == 0)
        return null;

    var oldestPerson = people[0];
    for (int i = 1; i < people.Count; ++i)
    {
        if (people[i].Age > oldestPerson.Age)
            oldestPerson = people[i];
    }

    return oldestPerson;
}

In this code there is a check at the beginning of the method for there being no people in which case null is returned. This seems like a reasonable check and a reasonable default however we have not considered how this code is used. Let's have a simple usage example:

var oldestPerson = GetOldestPerson(people); 
return oldestPerson.Age;

In this example the caller of GetOldestPerson is assuming that a person will always be returned, if null is returned then a null reference exception will be thrown on the following line.

Now imagine you get a nice error report with the exception details in it - the exception will not tell you where the real source of the error is (no people). In this example you could work that out pretty quickly by inspecting the code. Imagine however that you have a lot of complex code between the line calling the GetOldestPerson method and where it is used, the error could even happen several methods away just to make life more complicated. Now all of a sudden what should have been a 30 second inspection (oh - you have no people) can turn into several hours of trawling through code trying to trace back the error, especially if you don't have access to the same copy of the system which is producing the error.

You could argue that the calling code should immediately check the result from GetOldestPerson and throw a useful error if it is null. In an ideal world this is great but in most real code doing this in thousands of places is impractical and unnecessary. If this method is only called by your code and you don't expect or handle the no-people case in the callers then it would be better to let the GetOldestPerson method fail if there are no people. You could either keep the check but throw an exception with a useful message if there are no people or simply let the people[0] call throw an exception. Either of these will mean your exceptions help you find the source of the problem faster although the nice exception message may even allow your users to solve the problem for themselves.