What is too simple and small to refactor? (Clean Code Experience No. 2)

Introduction

Shortly after reading Robert C Martin‘s Clean Code, I refactored the data access layer from a project I was working on, and was amazed by how much the code improved. It really was night and day. My first clean code refactoring experience was an obvious improvement.

I was still on that clean code high, when a little function entered my life that I was compelled to refactor. This one left me questioning the limits of what I should refactor and if my refactor even qualified as clean.

I’d like to share that second experience with you in this post.

Original Code

Here is the original code. The code was written by and owned by Jim Holmes. Jim was kind enough to give me permission to use it in this post, as I was unable to find an equivalent example. Thanks Jim.

public static float WageCalculator( float hours,
                                    float rate,
                                    bool isHourlyWorker)
{
    if (hours < 0 || hours > 80)
    {
        throw new ArgumentException();
    }
    float wages = 0;

    if (hours > 40)
    {
        var overTimeHours = hours - 40;
        if (isHourlyWorker)
        {
            wages += (overTimeHours * 1.5f) * rate;
        }
        else
        {
            wages += overTimeHours * rate;
        }
        hours -= overTimeHours;
    }
    wages += hours * rate;

    return wages;
}

So as you can see, this simple method calculates a workers weekly pay based on hours worked, their hourly rate, and if they receive overtime or straight pay. There really isn’t much to it, is there?

So why was I compelled to refactor such a simple piece of code?

As soon as I opened this function, I felt it was doing too much. Mostly it was the isHourlyWorker parameter.

Since reading Clean Code, I’ve come to realize boolean and enum parameter types are a huge tell that they should be refactored into separate classes.*

My refactored code

So what did my refactored code look like after spending 30 minutes or so playing with it?

Well, here’s the new class diagram first, so you get some idea what you’re looking at.

And here’s the code

public abstract class WageCalculatorBase
{
    public float HoursWorked { get; protected set; }
    public float HourlyRate { get; protected set; }

    public WageCalculatorBase(float hours, float hourlyRate)
    {
        if (hours < 0 || hours > 80)
            throw new ArgumentOutOfRangeException("Hours must be between 0 and 80.");

        HoursWorked = hours;
        HourlyRate = hourlyRate;
    }

    public abstract float Calculate();
}

public class WageCalculatorForEmployee : WageCalculatorBase
{
    public WageCalculatorForEmployee(float hours, float hourlyRate)
        : base(hours, hourlyRate)
    {
    }

    public override float Calculate()
    {
        if (IsOvertimeRequired)
            return CalculateWithOvertime();
        return CalculateWithoutOvertime();
    }

    protected bool IsOvertimeRequired
    {
        get
        {
            return HoursWorked > 40;
        }
    }

    protected float CalculateWithoutOvertime()
    {
        return HoursWorked * HourlyRate;
    }

    protected float CalculateWithOvertime()
    {
        float overTimeHours = HoursWorked - 40;
        return (overTimeHours * 1.5f + 40) * HourlyRate;
    }

    public static float Calculate(float hours, float hourlyRate)
    {
        WageCalculatorForEmployee payCalc = new WageCalculatorForEmployee(hours, hourlyRate);
        return payCalc.Calculate();
    }
}

public class WageCalculatorForContractor : WageCalculatorBase
{
    public WageCalculatorForContractor(float hours, float hourlyRate)
        : base(hours, hourlyRate)
    {
    }

    public override float Calculate()
    {
        return HoursWorked * HourlyRate;
    }

    public static float Calculate(float hours, float hourlyRate)
    {
        WageCalculatorForContractor payCalc = new WageCalculatorForContractor(hours, hourlyRate);
        return payCalc.Calculate();
    }
}

And the code to execute this would be as simple as

weeklyPay = WageCalculatorForEmployee.Calculate(hoursWorked, hourlyRate);

or

weeklyPay = WageCalculatorForContractor.Calculate(hoursWorked, hourlyRate);

Or as flexible as

WageCalculatorBase wageCalculator = WageCalculatorFactory.Get(employeeInfo, hoursWorked, hourlyRate);
weeklyPay = wageCalculator.Calculate();

So as I said earlier, it was the isHourlyWorker parameter which compelled me to refactor. Notice how this parameter no longer exists, and has been replaced by a class for each of the potential values. isHourlyWorker has become the WageCalculatorForEmployee class, and !isHourlyWorker has become the WageCalculatorForContractor class.

Now one of the first questions you may have about the refactor is why didn’t I implement the Calculate method in the WageCalculatorBase class, instead of declaring it as abstract? Especially since both derived classes have identical methods, namely the CalculateWithoutOvertime method in the WageCalculatorForEmployee class, and the Calculate method in the WageCalculatorForContractor class.

What the hell happened to DRY?

I considered doing that, but decided that the Calculate method is such an important and critical piece of functionality, that implementation should not be left to chance. I felt an explicit implementation should be required.

And while we’re on the topic of the CalculateWithoutOvertime() method, you may be asking yourself, why this method even exists? I mean couldn’t CalculateWithoutOvertime(), CalculateWithOvertime(), and the IsOvertimeRequired property have been easily implemented in the Calculate() method?

Yes, as a matter of fact, I could have done it in a single expression, but felt it might be complex enough to warrant a comment, so I added the commenting into the structure, and kept the complexity way down.

Observations

Something else you may notice is the lack of control statements and flow branching. Notice, there are barely any ‘if’s. You may also notice the methods are very small, with the longest method being only 4 LOC, and most are 2.

You may also notice the increase in structural complexity, as noticed in Clean Code Experience #1.

But the real kicker to this refactor is that the original 26 lines of code became 74 when the refactoring was completed (including whitespace and everything). That’s a near 300% LOC increase!

This r-e-a-l-l-y bothered me, and left me perplexed as to if my refactor was foolish or wise.

Good idea?

So was it a good idea to refactor it? Is this clean code? Or was I just over stimulating myself with the refactoring? Was I partaking in refactoring masturbation?

I mean really … this wasn’t a complex function. There was nothing wrong with the function and the amount of code actually increased.

It took me a long time to wrap my head around this, but I finally decided this was good code. This was clean code. This was appropriate if creating code from scratch. This might be appropriate if working on client code.

.. Wait! .. what?

What do I mean, might be appropriate?

Would I do it in real life?

There is an ROI on business software. Unfortunately, software is not merely our art and our craft. Software is an investment. Software is a financial investment. More specifically, it’s not even our financial investment, it’s our employers.

So should you spend 30 minutes refactoring the above code? Is there an ROI worth it?
As much as I want to do that refactor, and believe later maintenance will benefit from it, I seriously question if the ROI would warrant it. I don’t think I would refactor this on its own.

…. Unless I was working on that code as part of my current task. If I’m already working on that code, then yes, by all means, it’s appropriate. It’s appropriate as part of my current task and as an act of craftsmanship to leave the code cleaner than I found it.

If it was my project, would I do it?

Yes, without a doubt I would. But that’s because I see software that I write for myself as more of a vehicle of artistic expression than a financial investment.**

What do you think?

I’d be interested in hearing your thoughts.

Is this clean code?

Is this good design?

Should you refactor code that is this small?

When should you refactor something like this?

What are the guidelines as to when it’s worth it to refactor or not?

* I don’t remember if he said this in the book or if this was my own insight. I don’t’ even know if Uncle Bob would agree with that statement
** My inability to view my own software as a financial investment is also the reason I’ve never released anything awesome and made a ton of money.

Copyright © John MacIntyre 2010, All rights reserved

Visual Studio Bug – ‘if’ followed by a try / catch causes debugger stepping error

Yesterday I was debugging and stepped into a method. I wanted to get past my parameter validation checks and into the meat of the method, so I quickly, F10’d my way down the method, but I noticed a line of code was stepped on which should not have been touched.

The code was a simple parameter validation like:

if (enumerableObj == null)
    throw new ArgumentNullException("enumerableObj");

with several similar parameter validation lines above it and a try/catch block containing the meat of the method below it.

The odd thing was, I thought I saw the debugger step on the throw statement even though the enumerableObj should have had a value.

I assumed I had somehow passed in a null value to the enumerableObj parameter and had nearly missed the problem in my haste. I had been moving quickly, so quickly in fact that I had stepped about 3 more lines into the method before I even stopped. To be honest at this point, I wasn’t even sure if I saw it step into the ‘if’ block, so I repositioned my debug cursor back to the ‘if’ condition, and stepped again. Sure enough, it stepped into the ‘if’ block.

I assumed I passed in a null parameter, but when I evaluated enumerableObj, it was set, what’s more, evaluating the entire enumerableObj == null expression resulted in false, as expected. But why the heck was I being stepped into the ‘if’ block when the ‘if’ condition was false?

I retried it again, just in case the enumerableObj had somehow been set as a result of a side effect somewhere, but even then, it still stepped into the ‘if’ block.

So, I did the standard stuff; cleaned my solution, deleted my bin and obj directories, reopening the solution, restarted Visual Studio, & rebooted, all the while rebuilding and retesting the project with each change. Nothing seemed to work. I even cut & pasted my code into notepad, then cut & pasted from notepad back into Visual Studio to ensure there was no hidden characters in my files.*

None of this worked, so I started commenting out code in the method, and eventually was able to isolate it to the above code failing if, and only if, it was followed by a try / catch block. Seriously! If the try / catch block was there, it would step onto the throw statement even though it should not have, but when you removed the try / catch block, everything worked just fine.

In order to isolate the problem for debugging purposes and to have something I could ask for help with, I isolated the problem to a simple command line app.

using System;
using System.Collections.Generic;

namespace IEnumerableBug
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                List lst = new List();
                lst.Add(999);
                IsIEnumerableNull(lst);
                Console.WriteLine("Post IsIEnumerableNull() call");
            }
            catch (Exception ex)
            {
                Console.WriteLine("-- Error --");
                Console.Write(ex.ToString());
            }
            finally
            {
                Console.WriteLine("\n\nPress any key to exit...");
                Console.ReadKey();
            }
        }

        public static void IsIEnumerableNull(IEnumerable enumerableObj)
        {
            if (enumerableObj == null)
            {
                // this should never be stepped over, but is (at least for me)
                // however execution moves to the next line once this is thrown instead of interupting execution
                // Also, a breakpoint on this line is never hit, as you would expect.
                throw new ArgumentNullException("enumerableObj");
            }

            // commenting out this line of code will cause the debugger to step through
            // the above control statement properly
            try {} catch { }
        }
    }
}

Once I got it isolated, I also realized that even though it was stepping onto the throw statement, it didn’t actually jump to the appropriate catch block, it just continued on it’s way to the next line of code. This is why I was able to F10 3 lines past the throw the first time I stepped on it.

Anyway, I threw the code up on Pastebin.com and asked my Twitter followers to see if the problem was repeatable for them. After the standard Twitter, 140 character limit, miscommunication confusion, everybody came back with a negative result. “It works fine on my machine.” … typical 😉

Close up, click for a screen shot of the entire IDE

At this point I was wondering if it’s some wonky bug only on my machine or if I hadn’t gotten enough sleep and I was doing something so incredibly moronic that I was about to publicly brand myself as an idiot.

Fortunately 2 things happened at that point; 1) I remembered one of my old laptops had VS installed on it, and I was able to repeat the problem on that computer, and posted a screenshot, so people wouldn’t think I completely lost it.

And 2) @james_a_hart came back telling me that he could repeat it and that it was a problem with the way Visual Studio is stepping through the debugger symbols. He also reduced the problem a lot further than I had. I had been so caught up in the problem, I hadn’t even realized how much more the test command line app could be simplified.

using System;

namespace IEnumerableBug2
{
    class Program
    {
        static void Main(string[] args)
        {
            if (new object() == null) 
                throw new Exception(); 
            try { }  catch { }
        }
    }
}

I don’t know who James works for, but I think he has a pretty sophisticated VS virtual machine setup, because he was able to confirm the problem only happened on 64bit Visual Studio 2008 and 2010.

Oh and for the record, in my testing, this only seemed to happen only with a throw statement. Switching the throw statement with a Console.WriteLine(), worked as expected.

using System;

namespace IEnumerableBug2
{
    class Program
    {
        static void Main(string[] args)
        {
            if (new object() == null) 
                Console.WriteLine("Equals NULL");
            try { }  catch { }
        }
    }
}

It was a heck of a day, and a huge waste of time, but I’m just glad I now know what the problem is.

I want to thank everybody who helped me by brainstorming with me on this, or running my code to check if they could repeat the problem; @james_a_hart, @JMBucknall, @dullroar, @SteveSyfuhs, @InfinitiesLoop, @klmr, @SyntaxC4, @CarolFil, and @brianrcline.

* As I understand it, cs files are plain text, so this was unlikely to be the problem, but I felt it was an assumption that I should test.

Hey #region haters; Why all the fuss?

I hear a lot of programmers saying #regions are a code smell. From programmers I don’t know to celebrity programmers, while the development community appears to be passionately split.*

The haters definitely have a point about it being used to hide ugly code, but when I open a class and see this, it just looks elegant to me.

Elegant Regions

Now none of these regions are hiding thousands of lines of ugly code, actually, most of these regions contain only 3 properties and/or methods and the last curly brace is on line 299. So the whole thing with 17 properties and methods including comments and whitespace is only 300 LOC. … really, how much of a mess could I possibly be hiding?

To me, the only question is whether I should have this functionality in the ContainerPageBase or the MasterPageBase**.

You may also notice the regions I have are not of the Fields / Constructors / Events / Properties / Methods variety. It has taken some time for me to accept that all data members (aka fields) do not need to be at the top of the class as I was classically trained to do and that perhaps grouping them by functionality is a better idea. This philosophy only makes regions that much more valuable.

… is anybody still here? …. have I converted anyone? 😉

* These posts are fairly old, but in my experience in the developer community; the consensus hasn’t changed.
** The Database Connection & Current User regions may have some scratching their heads. There are valid reasons for them, however the Data Connection region will never be included at this level again. More on that in a future post.

Copyright © John MacIntyre 2010, All rights reserved

7 Features I Wish C# Had

A while ago I saw StackOverflow question What enhancements do you want for your programming language?. I was able to, to my surprise, actually come up with something, but to be honest I don’t really think very much about what in my current programming language should be changed. Don’t get me wrong, I bitched and moaned about Visual Basic for about a decade, but it wasn’t that I wanted VB ‘changed’; I just didn’t want to work in it at all. But I digress, actually, you may want to avoid me if I ever get on the topic.

This question about what I would like to see in a programming language is something I’ve thought about, since, but the only real features I can think of consist around building a domain specific language around a specific industry and methodology. I think it would be cool to build a language around a technical analysis technique called Elliot Wave for example.

More recently, I’ve heard Jeff Atwood assert, on the StackOverflow podcast, that any good programmer can whip off 10 things they hate about their favorite programming language in a flash.*

I couldn’t come up with 10 things I hate, so I’m going to settle for 7 features I’d like to see. There may be good reasons why we don’t have some of them, but here’s my list anyway:

1. Parameter constraints

One of the first things I learned to do when I started programming, is to validate the parameters. This has saved me untold hours of debugging and I currently start every method with something like:

void DoSomething(float percentage, string userName)
{
	if (0 > percentage || 100 < percentage) 
		throw new ArgumentOutOfRangeException();
	if (string.IsNullOrEmpty(userName)) 
		throw new ArgumentNullException();
	if (0 > userName.Length) 
		throw new ArgumentNullException();
	if (userName.Equals("Guest")) 
		throw new ArgumentException("User must log in.");

	// .... do stuff
}

This is in almost every method. So why can’t the language do this for me automatically?

Yes, I know about declarative programming, but isn’t that just pushing it into the attributes? Where a 3rd party tool will process it?

What I’m suggesting is something similar to the generics keyword ‘where’. Why not have something like this:

void DoSomething(float percentage where 0 <= percentage && 100 >= percentage, 
		string userName where !string.IsNullOrEmpty(userName) && 20 >= userName.Length && !"Guest".Equals(userName))
{
	// .... do stuff
}

Really complex validation rules could be separated into its own function or you could always fall back to validation in the body of the method the way we do it now..

2. An ‘IN’ operator

I find the following code a bit redundant:

if (x == 1 || x == 2 || x == 3 || x == 5 || x == 8 || x == 13)
{
	// .... do stuff
}

I’d prefer to borrow the SQL keyword and just write

if (x in (1, 2, 3, 5, 8, 13))
{
	// .... do stuff
}

EDIT: Apparently SecretGeek already created a class for SQL Style Extensions for C# which does exactly what I wanted (for strings anyway). Thanks Darrel Miller for the link.

3. A 3 way between operator

I’ve always wished we could replace

if (fiscalYearStartDate <= currentDate && currentDate <= fiscalYearEndDate)
{
	// ... do stuff
}

with

if (fiscalYearStartDate <= currentDate <= fiscalYearEndDate)
{
	// ... do stuff
}

4. Only allow var on true anonymous types

var adds awesome functionality to C#, like the ability to create anonymous types from an expression. However, when I start seeing code where all the variables are typed ‘var‘ I get a little worried.

Now I know about type inference and that var is the same at run time as explicitly stating the type, but when you want to look up what the heck variable x is; it’d be nice if the variable definition would tell me

ObjectX v = new ObjectX();

instead of

var v = new ObjectX();

I also realize, I can figure out the type by looking on the right side of the assignment operator and in the above example above it’s pretty darn clear exactly what type v is. But what if the expression is a method?

var v = DoSomething();

Now I’ve got to look up the definition of DoSomething().

Not a big deal, but you’re already 2 definitions away from your core task. This is needless resistance as far as I’m concerned.

Well, you say, I could just use intellisense to hover over the method to get the method’s return type. That is true, because, thank god, the return type for a method (along with parameters) requires the type be specified, so I can get the method type from intellisense with only one definition source code jump. However, this logic is flawed because if the type was used instead of var, intellisense would have told me without any source code definition jumps what the type was. Rather than the useless “(local variable) object v” tip I receive with it.

You know, I am pretty anal about this type of thing, and NO, I haven’t had a real world problems yet, but it just feels wrong.

5. Constant methods and properties

One of the features I always used when I coded in C++ was constant functions. The great thing about constant functions is they prevent side effects. So you know if someone, like yourself, alters functionality which is not expected to change anything, has a side effect, it won’t compile and will have to be dealt with.

This isn’t the most popular functionality in C++ and while this concept helped me write bullet proof code on my own, when I started working on a team, they weren’t very thrilled with the keyword and quickly removed it. But hey, that’s kind of a tell isn’t it?

To be honest, I don’t usually have problems like this with projects I initiate. This may be irrelevant with good design, but I would like the peace of mind in having it anyway.

6. JIT properties without a local variable to cache the value

Remember how you’d need to create a local variable for every property you create? Like:

private int _id = 0;
public int Id
{
	get {return _id;}
	set {_id = value;}
}

Which became

public int Id { get; set;}

But what about properties with JIT functionality or have a side effect? Something like this, seems awfully unfair:

private int _nextId = 0;
public int NextId
{
	get {return _nextId++;}
	set {_nextId = value;}
}

Why not have something like a thisProperty keyword? Something like :

public int NextId
{
	get {return thisProperty++;}
	set;
}

7. Warn command

I’m not talking about the preprocessor #warning. I want something like throw, but without interrupting program flow.

Why? Lets say your data access layer pulls a null or otherwise unexpected value out of a database for a value which should have a very specific set of values.**

This is recoverable and no big deal, you use a default value, but you might like to somehow warn the user this was done. So; you can’t throw an exception since that would interrupt your program flow, I can’t think of any framework component you could add***, you could add some external dependency I suppose, you can build your own external dependency which all future apps will require, you cannot use one of the GUI level features like Session since it wouldn’t be available from the DAL DLL level, and you definitely don’t want to add parameters to every method so you can pass some warning collection up & down the stack!

But something like a warn command looks awfully elegant!

warn new UnexpectedDataWarning("Unexpected status code. Set to 'Open'.");

Then in the GUI level, you can traverse the warnings collection and display them to the user.

* I can’t figure out which episode it was, so no link. Please comment if you know. Thanks.
** Yes, the database should have constraints, but sometimes things are not under your control or perhaps you have a logical reason to not create constraints … but that’s another post … probably early next week.
*** At least I don’t know of anything. Please let me know if there is something built into core framework which would allow this.

Copyright © John MacIntyre 2010, All rights reserved

WARNING – All source code is written to demonstrate the current concept. It may be unsafe and not exactly optimal.

How To Write Dynamic SQL AND Prevent SQL Injection Attacks

One of my pet peeves is when general rules are taken as gospel, and declared as the only acceptable practice regardless of the circumstance.

One of the big ones is Dynamic SQL. There’s a heck of a good reason for this, and it’s called an SQL Injection Attack, and if you are not familiar with it, I would strongly urge you to leave this post right now, and read up on it.

Anyway, Dynamic SQL is not inherently evil, it’s the appending of user entered text that is evil. Appending user entered text is just lazy and can be easily avoided with parameterization.

The trick is to create dynamic SQL with parameters.

Interestingly, I’ve never seen anybody else do this. I am constantly hearing people recommending stored procedures … even when are clearly not flexible enough to meet the required functionality. Don’t get me wrong, stored procedures have a lot of benefits, but flexibility isn’t one it’s popular for.

And now for some code …

I created a console app which queries the SQL Server sample database AdventureWorks. The following static method was added to the Program class.

public static int GetOrderCount(string productPrefix, SqlConnection cn)
{
	// initialize SQL
	string starterSql = "SELECT count(*) FROM Production.Product";
	StringBuilder sbSql = new StringBuilder(starterSql);

	// add parameters
	if( !String.IsNullOrEmpty(productPrefix))
		sbSql.Append( " where [name] like @namePrefix");

	// initialize the command
	SqlCommand cmd = new SqlCommand(sbSql.ToString(), cn);
	if (cmd.CommandText.Contains("@namePrefix"))
		cmd.Parameters.AddWithValue("@namePrefix", productPrefix + "%");

	// get count
	return Convert.ToInt32( cmd.ExecuteScalar());
}

Basically, the function queries the number of orders where the product name starts with a certain prefix.

The strength of doing this via dynamic SQL is we only need to filter on the product name when a valid prefix parameter is passed in. So, if the optional parameter (productPrefix) exists and is valid, the filter condition is added to the SQL and the parameter is added to the SqlCommand object.

In this overly simplified example, we could manage the same thing by just setting the productPrefix variable to the ‘%’ wild card, but then we’d be doing a filter for nothing. Not to mention things might be a little more difficult if the operator were ‘equals’ instead of ‘like’, or if there were multiple optional parameters. Creating SQL dynamically means we don’t need to write some funky kludge and our SQL is always nice, simple, and doing minimal work.

To execute my function, I added the following code to the Main(…) method.

// get total count
Console.WriteLine( "There are {0} products in total.", 
			Program.GetOrderCount( null, cn));

// get totals for various prefixes
string[] prefixes = new string[6] { "a", "b", "c", 
						"'; drop table Production.Product;--", 
						"d", "e" };
foreach(string prefix in prefixes)
{
	Console.WriteLine("There are {0} products"
			+ " prefixed with '{1}'.",
			Program.GetOrderCount(prefix, cn), prefix);
}

First we call GetOrderCount(…) without a name prefix to test it without the parameter, then we traverse the array of possible prefixes (this would be the user entered data in a real app). Notice the fourth item? Pretty menacing eh? Don’t worry, it’s safe.

Here are the results

There are 504 products in total.

---

There are 3 products prefixed with ‘a’.
There are 4 products prefixed with ‘b’.
There are 12 products prefixed with ‘c’.
There are 0 products prefixed with ”; drop table Production.Product;–‘.
There are 3 products prefixed with ‘d’.
There are 9 products prefixed with ‘e’.

---

Notice the ‘d’ and ‘e’ prefixes were searched, and items found, proving the ‘drop table’ statement was not injected into the command.

You’d be surprised how much I use this. Many of my objects have a static GetList(…) method, and this method usually has multiple overloads. Keeping with the DRY principle, I prefer to keep all my logic in one place, so this method will usually have one overload with every possible filter parameter, and all the other overloads will just call this one. Surprisingly, the overload with the code, is not overly complex, and is actually pretty simple.

What do you think? Will you use parameterized dynamic sql in the future?

Copyright © John MacIntyre 2009, All rights reserved

WARNING – All source code is written to demonstrate the current concept. It may be unsafe and not exactly optimal.