While I Compile

… I compile my thoughts about programming

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.
Class diagram of refactor results

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

About these ads

September 14, 2010 - Posted by | C#, Code, Programming | ,

14 Comments »

  1. I definitely remember boolean params being identified as a sign that the method is doing too much in the book. Can’t recall off the top of my head if the suggested solution was separate classes or just separate methods. I would guess that largely depends on the context.

    Comment by Anna Lear | September 14, 2010

  2. I have yet to read the book, but this seems like clean design to me. The outcome is more flexible and extensible than the original, and is easier to read.

    I think you put the right question, when is it worth to refactor something and where should we set the limit.

    I have mixed feelings on this case, on one hand your solution is far more elegant, on the other the original code was smaller and something of a lesser feature of the original application.

    Comment by Nuno Furtado | September 14, 2010

  3. Hi John,

    I don’t believe this code is as clean as it could be. Your base class isn’t providing much value to the rest of your code, also there is a lot of assumptions made in the code which should be opened up to the object.

    I’ll respond in more detail in my own post :)

    Comment by Cory Fowler | September 14, 2010

  4. That would be awesome. Email me the link when you finish.

    Comment by John MacIntyre | September 14, 2010

  5. For anyone that would like to see my response, it can be found here: http://blog.syntaxc4.net/post/2010/09/14/Reply-What-is-too-simple-and-small-to-refactor.aspx.

    Comment by Cory Fowler | September 15, 2010

  6. Here’s my response. I’ve scrapped inheritance entirely in favour of pushing the calculation function into the employee object:

    http://www.endswithsaurus.com/2010/09/reply-what-is-too-simple-and-small-to.html

    Comment by Ben Alabaster | September 15, 2010

  7. I haven’t looked at either of Ben or Cory’s versions yet but my initial impression was …

    The business rules are:
    Base wage (up to 40 hours per week) is calculated at hours * rate.
    Overtime wage for non-hourly workers is calculated at hours * rate.
    Overtime wage for hourly workers is calculated at hours * 1.5 * rate.

    This means the only difference between hourly and non-hourly workers is how the overtime is calculated and therefore it is this difference that should be represented in your refactoring.

    To encapsulate this difference you can either:
    * use inheritance (similar to what you have done but with only the overtime calculation requiring implementation in child classes).
    * use composition by creating an overtime calculator and having the WageCalculator take a dependency on OvertimeCalculator.
    * use functional composition (not sure if thats the right term) and provide the overtime calculation as a constructor parameter of type Func.

    Now off to see how Ben and Cory solved it =)

    Comment by @randomcodenz | September 15, 2010

  8. [...] Posted on 2010-16-09 by Cliff Mees A couple days ago, John MacIntyre [@JohnMacIntyre] posted an article about refactoring a small wage calculator function.  This is John’s second post in a series [...]

    Pingback by Clean Code Experience | Coding In Caledon | September 16, 2010

  9. This is all ironic right? Yes, the original code is horrible. It dances around the point so much it’s basically obfuscated.

    I’d just clean it up, like so

    decimal wages = hours * rate;

    var overTimeHours = hours – 40;

    if (overTimeHours > 0 && isHourlyWorker)
    {
    // hourly workers get paid 50% more on overtime
    wages += 0.5m * overTimeHours * rate;
    }

    return wages;

    And never look at this completely irrelevant piece of code again. But you create a whole inheritence hierarchy of classes? What were you thinking! Surely you must be taking the piss.

    Comment by Joren | September 17, 2010

  10. [...] “John MacIntyre adds too many pasta w/o taste & nutritional value for my taste” [...]

    Pingback by Qualitativ hochwertiger Code ist eine wirtschaftliche Notwendigkeit! « Software-Sanierung | September 22, 2010

  11. That Spaghetti Code quote is awesome … just wish it wasn’t about me.

    Comment by John MacIntyre | September 22, 2010

  12. I haven’t read the book, but from the example here, it looks like you’re thinking of refactoring as “add more architecture”. What you’ve ended up with here is actually too much architecture/structure…

    Refactoring is about making code better, cleaner, etc. Sometimes that even means less architecture/structure. Remember there are always trade-offs at play. I’ll hand over to Einstein to wrap up:

    “Make everything as simple as possible, but not simpler.”

    Comment by Dave Broome | September 23, 2010

  13. The artist-coder in me totally agrees with your urge to factor out the boolean. It greatly decouples both cases.

    Decoupling will also result in behavior scattered throughout the code base: when you come up with even different wage calculator models, you simply add a new implementation.

    But what if I want to have an overview of all possible payment mechanisms? Then I need to search the code base for all implementations of Calculate.

    So your decision/ROI calculation should be based on what kind of changes you’re going to need most: touch all implementations together, or add implementations.

    In this case, you have two implementations, and no immanent third, so I would say: leave it alone… for now.

    Comment by xtofl | September 28, 2010

  14. [...] A Spanish summary of Uncle Bob’s review blog post [...]

    Pingback by Refatorar é bom para aprender OO « Mente de Iniciante | October 24, 2010


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: