NOthingyoumissed

George Mauer is on the Net

Error Handling and the Message Repackaging Anti-Pattern

I currently have an interesting responsibility at work.  I am functioning as the manager and single-point-of-contact for a team in India working on imporving the codebase for one of our more important ASP.Net sites.  I know what you’re thinkin.  Did I say interesting?  I meant infuriating.  At the very least I should get a good “Lessons Learned” post out of this.

Overall the experience hasn’t been too bad though.  I’ve been managing to maintain a decent rapport with their project manager and team lead, deadlines have been missed, but no more than I secretly expected, and overall they seem like fairly competent guys.  Their work isn’t innovative, Agile, testable, or well factored, but its decent enough.

Except for the exceptions.

Browsing one of their recent commits to our SVN lately (ok, so technically speaking they couldn’t figure out Subversion so they ftp-ed and I did the commit for them) I was shocked to discover the following error-handling anti-pattern repeated 41 times!

try {
  // Do Stuff
}
catch(Exception ex) {
  throw new Exception(ex.Message);
}

Do I get to name it?  Let’s call it the message repackaging anti-pattern.  And in case you’re not already reeling from this, the following is a mildly edited-down version of the letter I sent (Notice the eschewed profanity.  I believe a not insignificant achievement.)


Dear Team,

While looking through some of your committed code I noticed quite a few (a quick search shows 41) places where you have code of the following form.
try {
 // ...
}
catch(Exception ex) {
 throw new Exception(ex.Message);
}
Let me be absolutely clear: This is the worst possible way of handling exceptions!
Let’s go over why:
  1. Not doing anything inside the exception. If nothing happens within the catch block then what purpose does it serve? I have seen people prepare their code like this in anticipation of going back and adding logging at some later date. Although this is not necessarily the best logging solution, it is acceptable when you actually implement the code! However, preparing your method like this in anticipation of some future date does nothing but make the code difficult to read. It’s not like it is that hard to go back later and add a try…catch block; if you’re using a refactoring tool like CodeRush it is literally four keystrokes. 
  2. Catching the base Exception. Not every exception can have something be done about it. The canonical example is an OutOfMemoryException or one implying the database might be down. Is there anything to do in this case other than drop the user to an error page? I would even go so far as to say that most exceptions fall in this category – you just can’t do anything about them.  I strongly encourage you to read Stop Catching Exceptions for a more in depth discussion.
  3. Treating the message as if it were the most important thing. Exceptions usually carry a heck of a lot more information than just their message. They carry their type and a StackTrace and a list of any inner exceptions. Many specializations of the Exception base have even more information specific to that type of error. Out of all that information, the message is arguably the least important – for debugging I would take a StackTrace over a message any day. But by creating an exception off of the message you are effectively stripping out all this useful stuff! If for some reason, you must create a new exception, at least use the constructor overload that includes the original error as an InnerException: new Exception(“I don’t know why I’m creating this”, ex);
  4. Why create a new Exception object at all? You already have a perfectly valid exception object. It is a simple matter to do:
    try {
     // ...
    }
    catch(Exception ex) {
     // Log or otherwise handle the exception
     throw; // Same as throw ex;
    }
So in conclusion
  • Don’t catch an exception if you aren’t going to do anything with it.
  • Try to catch exceptions at the appropriate level and for the appropriate task. (logging inside private methods is probably unnecessary)
  • Do not create a new exception, throw the old one.
Please take this as healthy constructive criticism. I hope that you agree with me on these points and that we can take care of this issue properly from now on.

 


A bit harsh perhaps, but I feel like for all the reasons above the immensity of this mistake cannot be overstated.  If you are doing this in your code STOP NOW and apologize to the maintenance gods!  You might even need to sacrifice a goat to appease them.  It’s that bad.

In any case, the recent commit has only 38 instances of this error.  So at least we’re getting better!

Advertisements

January 16, 2009 Posted by | Programming | , , , | 2 Comments

Re: What is Unit Testing?

In a post last month Zachariah Young posts that 

… unit tests … verify that the business logic is correct.  I believe that for it to be unit testing it has to use a testing framework like nunit or junit.

I say thar be dragons!  Tying your understanding of unit testing to a specific tool allows you to neatly sidestep thinking about whatThar be dragons  testing actually does.  Additionally, you deny credit to an excellent tool like NUnit by pigeonholing its purpose.

Consider for a minute what NUnit actually is.  Yes their own website claims that they are specifically a “unit testing framework for all” but what does it actually do?  In my opinion it is a framework for setting up an application’s state, executing code, and then asserting that the resulting state is as you expected. That’s pretty broad.  Way broader than the confines of mere unit testing.  

Take the excellent WatiN or (no longer maintained) NUnitASP tools.  Both can be run using NUnit but neither is used to build a true unit test.  WatiN for example remote controls your browser to investigate how your page will react to certain stiumuli.  What unit does WatiN test?  Your Page_Load method?  Your page renderer?  Whether the viewstate persists that you had entered some text halfway up the page before submitting and the text is not cleared when your page returns with a validation error?  The localhost routing mechanism on your PC?  Or does it test the integration of all of these?

I made the same exact mistake myself when I decided to ‘see what this TDD thing’ is all about last April.  I ultimately had to delete my entire testing project after I came to the realization that each test bit off far too large a chunk making it utterly impossible to maintain once true refactoring started.  I nearly cried that day.

But out of the ashes I was reborn with what I hope (but don’t really expect) to be the final zen-like understanding of this unit testing thing.  The first aha moment was when I finally caved in and decided to learn Rhino Mocks and my understanding sharpened when I caved again and looked into the new Rhino Mocks 3.5 AAA syntax.  Overall the lesson seems to be that I’m always wrong and I will eventually give in on everything and like it.  My girlfriend will be happy to hear that.

Let me tell you how I understand unit testing now.  Imagine a room.  You are standing on the outside and can comunicate with its ouccupants through a two way loudspeaker.  This room is your unit – if it helps you can imagine the room as painted black.  Now in testing you present your unit with a problem:

Given that you are on 735 Bourbon St New Orleans, Louisiana how far is it to my grandmother’s house?

I used to think that unit testing involved merely waiting for the answer:

Your grandmother’s house is 1345 miles away and you haven’t visited in months you shmuck.

Now I realize its more about everything else that is said.  “Where does your grandmother live?” and “Can you be so kind as to  slip an atlas under the door?” should definitely be questions that you hear over that loudspeaker.  If you’ve separated your resposibilities properly the unit will also require a calculator that given two points on a map can tell you the distance (the specific calculator implementation should depend on whether you intend to fly or use the highway system).  The unit might even request that you hang a EnRouteToGrandmas = true sign on the door.  

The point is that you’re not so concerned with the final answer as much as you are with the unit asking the right questions.  Afterall, without that information know that it could not possibly be doing what you mean for it to do!

And so I define unit testing as

Verifying that given an input, a piece of code makes all the external requests that you would expect where the unit is small enough that the number of these can be enumerated with ease.

Note that this definition does not necessitate automated testing and this is something that I again think is basically correct.  After all, a framework should not do anything that you cannot do for yourself.

The beautiful part is if you’re limiting the amount of requests that you’re going to expect – let’s say no more than four – single responsibility emerges almost all on its own.  After all, how much can a piece of code do if its not communicating with the external world?

So there you go, thats my understanding of unit testing.  I look forward to being proven wrong in the future and having to refactor everything all over again.  But until that time,  Zachariah, you’ve been blogo-served!

Disclaimer:  I do not know Zachariah personally but he seems like a smart guy that is highly involved in the ALT.Net community.  I do not purport to be more knowledgeable than him in programming issues, I just disagree with him on this one point.

kick it on DotNetKicks.com

January 15, 2009 Posted by | ALT.Net, Programming | , , | Leave a comment

New Years Resolutions

Ok, so just like everyone else in the universe I’m posting some resolutions on my blog.  The first and most obvious if you look at the dates of my previous posts is to blog more.  A heck of a lot more.  I’m aiming for once a week.  Nothing intense, I’m no Steve Yegge and I don’t have all that much to say but I do have some thoughts that I need to start writting down and I do need to practice my writting. 

That being said, some career development-oriented resolutions:

  • Attend at least one professional – preferably an ALT.Net centered – conference.  Got to figure out which one first.
  • Learn about continuous integration and how to use a build system.  I am thinking NANT, CruiseControl.NET or TFS.  I just want to be able to build all these awesome open source projects so that I can…
  • …read other people’s code.  For all my complaining of being a lonely developer working in a godawful architecture I certainly make few enough attempts to connect with well written code.
  • Learn more about dynamic languages.  I have spent plenty of time in the PHP world and I’ve got to say, I just don’t get it.  Yes some tasks can be done with slightly less code, but you have to give up such IDE luxuries like Intellisense and automatic refactoring.  With all the chatter lately of Ruby and with IronRuby 71% complete maybe I’ll try my hand at that. 
  • Javascript.  Find an excuse to get beyond the basics.  I think it is a language with much to teach me and I want to learn.  It doesn’t hurt that JQuery as about as awesome as can be.
  • Learn more about .NET internals and the CLR.  I would love to be able to get a deep enough understanding to design my own language – not that I have any intention of doing something so crazy.

January 1, 2009 Posted by | Album | Leave a comment