Down with Defensive Coding (against exceptions)?

Question

I've noticed a lot of code lately (mine included) that, e.g. makes a DAL call, and then checks to see if the returned DataSet is not null, then checks to see if it has tables, then it returns Tables[0].

If there is no DataSet, I would rather see hell raised ... and a few exceptions, than have a user wandering where all there invoices are.

What are your guys thoughts on this?

Answer 1

I think it really depends on one question.

Is the underlying error condition being checked for a recoverable error?

If the error is recoverable then by all means make the checks. Perhaps there is a default value being returned in case of an uninitialized table.

If the error is not recoverable then by all means fail and fail loudly. Exceptions are a great way to fail loudly. The faster and louder you fail will enable people to find bugs faster. Finding a bug faster usually results in a much cheaper fixe.

Answer 2

I think an error should be logged but we should display a custom error message to the user. The "Yellow screen of Death"(for asp.net) is never a good feedback :)

Answer 3

Exceptions are a great way to record failure, but it's always better to fail in a controlled way than to fail anywhere due to lack of return value checking. Always fail gracefully.

You can do all those checks, and then if the dataset is null, log something, take all the appropriate actions like closing the database connection and then throw an exception (and a nice friendly message).

Answer 4

Defensive coding still allows you to raise hell when something bad happens. If anything, you have more control over the manner in which you do so. Often times multiple problems will throw the same exception, making it harder to tell exactly why something failed.

Personally, I like to run my apps in VS with the IDE set to break on all exceptions. If I'm expecting exceptions, this doesn't work so well.

Answer 5

There's not much point in raising an exception just for the sake of it, it just makes you need to add a try catch block when you could just do a null test.

When querying a DataSource an empty response can quite frequently be valid, therefore it is intuitive for the programmer to handle that response. In contrast; a "no results found" exception would not be intuitive, but a "connection timeout" error would.

You should not raise an exception when the object is in a valid state.

Regards,

Docta

Answer 6

As JaredPar correctly stated, the answer is "it depends".

• If there is a contract for the DAL and you violated it (invalid parameter, bad table name, null connection string, etc), resulting in an empty or NULL dataset, then it is reasonable for the DAL to throw an exception
• If the query that was supplied just generates no results but is a completely valid query, then it probably should not throw an exception.

When deciding whether and when to throw exceptions requires thought about a number of things including:

1. Is the condition truly exceptional?
2. Is the condition truly exceptional at the current level of abstraction?
3. Is the condition recoverable?
4. Is the condition recoverable at the current level of abstraction?

For example, depending on your abstraction levels, it may not be at all exceptional for a valid query to return an empty dataset. However, at the next level up - the class calling the DAL, maybe an empty dataset should never occur. In this case, the DAL shouldn't throw an exception. As to whether the calling class should throw an exception depends on 3 and 4. If the calling class can recover from this issue, then it shouldn't throw an exception (perhaps it should log an message however) and it should take the proper recovery steps. If it cannot recover from the issue at that level, then it should throw an exception so that a higher abstraction level can potentially gracefully handle it (upto and including graceful shutdown of the application).

Answer 7

I think this boils down to the exception vs. error returning mechanisms. To me, you should use exceptions to control 'exceptional' situations while checking results where the condition is expected (not so exceptional). That way, error checking will not get in the execution flow for unexpected situations.

This is different to 'let exceptions propagate and raise hell (or terminate your application)'. You should catch and process the exceptions at the point where it makes sense.

It is hard to draw a perfect line there. If the user makes queries without having any foresight into the results (say it asks for any invoice in a given date) whose result can be an empty set, then you should check if the set is empty. If the user performs a query that has expected results: get last 5 invoices (that must always yield 5 results!!) then an empty set is surely an unexpected result and instead of providing with a 'no results' message you should probably throw an exception that is caught where the operation was started and gets presented to the user as an application error. If the situation is unrecoverable, then don't even catch the exception, let it propagate and kill the application.

Answer 8

I am a believer in catching problems early.

If code I write needs certain types of data (ie. not just a DataSet value, specifically a non-null value, referring to a DataSet with at least one table, containing at least 1 row), then I will write defensively by explicitly checking this when the method starts.

I do this for checks that can be done reasonably cheap.

For instance, if the method needs an array of objects, that will almost be fairly small, and none of the elements can be null, I will often loop through to check once at the start of the method.

If I have an enumerator on the other hand, or an array that can potentially be large, then I'd rather postpone the actual check to inside the loop.

I've had enough debugging sessions to figure out why a specific exception occurs at some random deep code in the framework, only to figure out that an invalid value passed through N layers of my code first, and the deep down code didn't have enough clues to figure out the specific reason for why it went wrong.

Answer 9

One main concern here has been the recoverability of the failure condition. Very often recovering from that condition is expensive, and not worth the cost. In my first few iterations/releases of the application, and not just in beta, I would rather use 'free range' exceptions to help define the scope of handled exceptions.

Call me a cowboy, but cowboys are still somehow around, supported by the perfectionist, astronaut architect that I would be if not for cowboys. ;-)

Answer 10

I agree with you. In my opinion, the "defensive" habit of checking every result returned by an API for null is almost always the result of either a poorly understood API or a poorly written API.

In your example, a DAL API with method

public DataSet GetSomeData(...);

I expect the method to either succeed and return a non-null DataSet object containing a non-null collection of Tables (possibly zero-length if there are none) or else fail by throwing an exception indicating why.

Whenever I see the code you mention

DataSet result = GetSomeData(args);
if(result != null)
{
    if(result.Tables != null)
    {
        // do something with the results
    }
}

I wonder... Will this method really ever return null or null Tables? If so, what does that mean and shouldn't we have an else statement doing something about it. If not, we should clean up the code by removing the unnecessary statements.