FUD driven development

disclaimer: You ever notice how some people use the word “passionate” when what they really mean is “annoying”? Hopefully I am not that guy. Smile 

The fact is, here it comes … I get passionate (insert your synonym of choice) about code, both good and bad. I got particularly emotional, shall we say, about a piece of code I needed to read and duplicate in the current prototype I am working on.

My task for the day was re-implementing existing logic in an MVVM application in a ASP.NET MVC prototype. My workflow for the day was shaping up nicely: target an existing piece of functionality, say a View in the WPF application; do a quick re-design with a smaller form factor and a web platform in mind; build out a quick set of View/Controller/Model and Repository classes; wire these into the already existing navigation by adding a link or button to access the new page; then all I needed to do before I could show real data on the screen was to open up the WPF code, find the ViewModel that was loading the same data I was looking for, and make sure I was apples to apples when it came to security and business logic decisions etc. Life was good.

I opened the ViewModel I needed to pull logic from, and an interesting thing happened. At first I thought I was just annoyed and frustrated because the code was not written as well as I would have liked. On later reflection, however, I realized that what really happened was that the code didn’t so much annoy me as it did un-nerve me. It rattled me and totally killed my confidence in what I was doing. So I thought I would analyze just what about the code had this effect and see if I could define it better for myself.

I am not posting real code here, so what follows are pseudo-code examples that demonstrate the flavor of the code in question.

The basic structure of the method is this:

public void LoadData(ReportType contentOption)
{
GetOrdersArgs args = new GetOrdersArgs();
args.SalesPersonId = LoggedInUser.Id;
args.ReportType = contentOption

//other code here

GetOrderData data = SalesRepository.GetOrders(args);
return data;
}

Create the args needed to send to the service, call the repository, return the data. Nice and clean. The real code had more arguments but this should suffice to make the point.

As I was reading the code the first thing that jumped out at me was the fact that the SalesPersonId argument value was being set to the logged in user’s id. In most cases this was fine, but there was also the ability for a manager to view sales data for salesmen they managed, so the logged in user’s id was not always the correct id to pass as an argument. At first glance I thought I had found a bug in the code. Then I read on.

//bug fix. based on the contentOption enum that is requested, the SalesPersonId may need to be changed
//
switch(contentOption)
{
case 1:
args.SalesPersonId = LoggedInUser.Id;
break;
case 2:
args.SalesPersonId = GetSalesPersonId();
break;
//etc
}
//end bug fix


So, what happened here was now fairly clear: the bug was found, perhaps in testing … this is good. The cause, submitting the incorrect id in cases of manager access was properly understood … good as well. The correct fix was inserted and we’re batting a thousand here! So what’s the problem? It isn’t so much what was done as it was what was not done. The old code was left in place. This makes the first batch of code misleading and gives any reviewer, future debugger, a false impression of what is being done in the method, even leading to my false assumption that I had just found a bug. At this point I would understand if your reaction was “so what?”, no big deal etc. However, keep in mind that this sample has been dumbed down to fit in two tidy code snippets to demonstrate the issue, imagine the real code in question was well over a hundred lines long, and the affected logic was multiple lines of code, not one simple assignment.

With all that being said, the question is: why? With the bug understood, the fix understood and implemented, why stop there? The answer is what drove me to write this post: Fear Uncertainty & Doubt (FUD). The method could be considered complex, that I will grant, the only reason I can see for holding back from finishing the job is fear, fear of breaking something else unintentionally. So basically, the thinking could go like this:

  • “I see here at line 115 the value is incorrect because the code above did not account for the manager access scenario.”
  • “I could review lines 1-114 to find the original problem, fix it there, retest the method …”
  • “… or I could just insert logic here to ‘fix’ the problem as I see it”

This is just one small step towards a big ball of mud. The fear to make the code better as we go ensures that we will make it worse. Another lesson I learned is that FUD begets FUD. I was “afraid”, in a way, of  the code in question because I thought I knew what it did and now I was unsure of it’s intention, I had to do more work to make sure I understood the code, and was able to replicate the logic correctly. That, I believe, is what caused my original uneasy feeling when reviewing the method. Unfortunately, if the developer working on the next fix in this code took the same approach, we would then have 2 misleading code segments that were made unnecessary by yet another fix later in the code. Wash, rinse, repeat.

So, when it comes to fixing code and making our code base better and more maintainable as we go, the old mantra stands: “if we are not part of the solution, we are part of the problem”. If our code does not add clarity, or we write, or even just allow to survive, code that is unnecessary or misleading then even if we “fixed the bug” at the end of the day we’ve left the code worse off then we found it.

About me

.NET developer in upstate NY, USA
Current focus technologies: WPF, WCF
Intrigued by: Functional programming ala F#, Code Analysis, Math
Hobbies: this blog, go figure

Month List