Yes, it’s an error, but it’s *my* error!

This morning I learned of a problem with a .Net optimization component developed by my team. We checked the server logs and we found that an optimization failed with the following (fictionalized) call stack:

System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.
   at System.ThrowHelper.ThrowKeyNotFoundException()
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at MyLibrary.Optimization.CreatePlanningHorizon(Int32 planId)
   at System.Linq.Enumerable.WhereSelectListIterator`2.MoveNext()
   at System.Linq.Enumerable.Sum(IEnumerable`1 source)
   at MyLibrary.Optimization.FindPlanningHorizons(BusinessObject data)
   at MyLibrary.Optimization.Initialize(BusinessObject data)
   at MyLibrary.Optimization.CreateModel(ScenarioData info)
   at Optimization.OptimizationThread.Optimize() in c:\Program Files\MyLibrary\OptimizationThread.cs:line 42

LINQ! Dictionaries! Call stacks! Help!

It turns out the situation wasn’t that complicated – it turns out the data that was being passed to the optimization library was invalid. A “plan” entity must have a range of “time period” entities associated with it. The CreatePlanningHorizon method examines the time periods associated with a plan to create a planning horizon. In this case, we were passed a plan with no time periods, which is invalid. (Again, I have fictionalized the scenario: I have renamed the entities from our actual application. The point is that we were passed invalid data according to the business rules of our application.)

It is totally cool to throw an exception in this case.  The .Net Design Guidelines for exceptions explain why – and this advice applies for other environments as well, for example Java. Don’t try to set an error code, or swallow the error and soldier on. This page states it well:

If a member cannot successfully do what it is designed to do, that should be considered an execution failure and an exception should be thrown.

So our problem is not that we are throwing an exception, it’s that the exception is confusing. The message does not tell me what the problem is, or what to do about it. The right thing to do here is to explicitly check the condition that should hold, and throw your own exception. You don’t need to define your own custom exception type for this. You can throw an existing exception type with a decent message. For example, at the time the planning horizon data is read from the database, throw a System.IO.InvalidDataException with the message: “The planning horizon with ID=42 is invalid because it has no associated time periods.” (Or something like that.) The point is that the message should indicate that there is a problem with the input data, so that the appropriate member of the development team can address the issue. The user of the application should never see this message – it’s for internal use.

Failing in the manner of your own choosing is preferable to failing in an unpredictable fashion!

About these ads

One comment

  1. I believe this is part of the principle of Fail fast: As soon as a programming variable is not without it’s expected range or an exception occurs, fail fast with a decent error message. Don’t ignore it and continue only the fail late with an bad error message (ussually NPE).

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s