I attend a book circle reading Code Complete 2 by Steve McConnell, where the discussion about defensive programming and asserting came up. I told the group about my assertion mechanisms with extension methods and it was all well recieved. On my way home, I started thinking about it and found a small danger about assertions.
public string Reverse(string argument)
{
argument.AssertThat(s => s != null,
"Failed assertion: Argument was null");
// ... logic ...
// return result;
}
What is wrong with this? An AssertException will be thrown if the argument passed to the method is null, where an ArgumentNullException would be much more appropriate.
public string Reverse(string argument)
{
if (argument == null)
throw new ArgumentNullException("argument");
// ... logic ...
// return result;
}
Why on earth could this be important? Because you could be catching ArgumentNullException further up in the call stack. You won't be catching the AssertException, and why is that?
- An AssertException does not tell you what is wrong and there is nothing you can do to save the day. It is the same thing as catching Exception and hope to recover from it.
- If an AssertException is thrown there is something wrong in your development code. These exceptions should never be thrown in production environment, because they are written to make development defensive and not for error handling in production.
This was a valuable lesson learned today. Now I'm going to review all my code that has to do with Assertions. Good evening!
Alive is a performance counter monitor hosted on ASP.NET. You use it to get a dashboard on how your production environment is doing.
5 Comments
Mikael Lundin said
I did not want to involve "design by contract" in my post, but focused only on the assertion, even if those are close related. DbC is where you check preconditions, postconditions and invariants and defining these for every routine is defining a contract for your methods.
I think it fits well together with TDD, but some might say that there is an issue with redundancy. Should you test that the contract holds? Should you apply tests to your assertions? or is the whole contract a way to let you skip testing the most primitive edge cases?
This is where programming turns into politics.
Alexander Schrab said
My sole point is that using an assertion to detect runtime problems that are not caused by programming mistakes is a bug in itself. You should never end up with an assertion for something that is clearly an exception of usage error. And you are probably right in that those who understands this actually cares. And those who doesn't care, well by defintion they don't care :)
Mikael Lundin said
What you're describing was the point with the whole post. :) Maybe I wasn't clear enough with what I meant. The assertion and the ArgumentNullException are used for two different scenarios and it should be carefully considered what's going on in the code.
This could also be a bit of nitpicking, since defensive programming does not come easy in any language and is forced by the programmer writing the code. What kind of programmer does distinguish between assertions and runtime null checks anyway? well - those programmers who cares.
Personally I'm looking forward to Code Contracts in C#/.NET 4.0.
Alexander Schrab said
Hmm, asserting the state of the class is called checking the invariant. Sorry, mixup. It's late. Read this Wiki-article:
http://en.wikipedia.org/wiki/Class_invariant
Alexander Schrab said
You are mixing two concepts here. Assertions are sane debug-time additions to the control flow of any big project (normally you disable them in any release build). Extensions to the language to ease writing assertions for classes (when you have assertions on the state of a class it's usually called integrity checking) and functions are welcome, but if that is not available you can simply make assertions on conditions whenever you assume something to in the code and (here comes the important point) if the assertion does not hold true it is a programming error. So this is the big difference between throwing an ArgumentNullException and an AssertException. In the case of the StringeReverse both cases are perfectly valid, but in two very different cases. If the StringReverse function is an exported library function that you expect others to use, the null argument case should be handled in some way (throwing an ArgumentNullException for instance). But if the function is an internal function only used in your module and you assume certain preconditions when using it (like it should never be called with null) the first case is the 'correct' solution.
So to summerize:
assertion when the fault is a programming bug in you module
exception when the fault is a usage error or something that is not a programming fault and is outside your control (trying to open a read only file as read write perhaps) has happened.
And another point:
When you know when and how to use assertions, add as many as you possibly can. Since you normally disable them in release builds they don't pose a performance problem and you really do want to catch a bad state as close to the source of the problem as you can.