Monday, August 16, 2010

Error from misplaced behavior

I was recently faced with a strange problem at a client site and decided to share.  Perhaps the only value this will have is me simply venting.  However, I hope other developers will come across this entry and learn from my experience and we'll all benefit.  Isn't that the point of a blog? Venting, I mean.

So, we are using Visual Studio 2010 on .NET Framework 2.0 application written in VB.NET.  I'll give you a minute to stop laughing.....

Anyway, an object was generating an exception, but I couldn't catch it in the debugger.  In other words, I had a breakpoint in the constructor, but it was never being hit despite an exception being thrown: "Type initializer for 'SomeClass' threw an exception" or something very close to that.  I tried a few breakpoints, cleaned the solution, Rebuild all, etc. restarted VS and then decided something strange was going on.  Well, I went out to Bing  (I'll give you another minute....) to look up how to get Visual Studio to break on exceptions even when they are being caught and handled.  I was pleased to see that my friend Steve Smith had written one of several blog entries describing how to do this.  As I had hoped, this helped me to discover the problem, although being observant would have helped.

The code was something like this:

Public Class SomeClass

Private someFileLocation As String = SharedClass.SomeMethodThatThrewAnException() & "\folder\somefile.txt"

Public Sub New()
   LooksLikeTheFirstLineOfCodeToRun() ' Breakpoint here wasn't hit under default VS settings
End Sub

I hadn't seen ( hem) this kind of code in a long time.  In fact, as soon as I found what was going on, I thought, "Well, I could have smelled that."  However, I just didn't expect anything like this and I didn't spend any time looking at the code above the constructor.  Enough said, really.

However, just to be thorough...

Variables should not be initialized in this way.  The exception cannot be caught in this class which is a code smell.  One should immediately recognize that the behavior of determining "someFileLocation" is not encapsulated.  At the very least, I would expect something like:
Public Sub New()
End Sub

Single Responsibility Principle and Dependency Inversion
More importantly, however, is the fact that this class should not have the responsibility of initializing "someFileLocation".  This object should exist to provide a specific, narrow set of behaviors.  The behavior of creating the file location is a separate distinct responsibility.  Further, this class is dependent upon the details of how the file location is determined, when it should be isolated from that detail.  The file location should have been passed into the object instead and then I wouldn't have been forced to read Steve's blog.  :)

So, I used to see this kind of code and say, "Well, you just don't do stuff like this!" I would see it as symptomatic of a lazy developer saying, "Well, I need this file location, but constructing it is so simple, I'll just initialize an instance variable, what could go wrong?"  But now I realize that this is just an obvious case of violating principles, but others I have encountered (and created) are more subtle.

So, the next time you see something simple and ugly that obviously needs refactoring, take a few minutes to articulate to someone specifically why it is wrong.  It's good practice for those times when you smell something wrong, but it's not obvious why.