While I Compile

… I compile my thoughts about programming

My week (09/18/2010)

Blog Posts

Earlier this week I posted What is too simple and small to refactor? about a  follow up to my first Clean Code experience where I took a very small function, and refactored it.  In the end I was truly questioning; how small is too small to refactor?  This post received quite a bit of a response, including a response from Uncle Bob Martin and several refactors from Cliff Mees, Neal Blomfield (his response), Cory Fowler Ben Alabaster begin_of_the_skype_highlighting     end_of_the_skype_highlighting, and even Jon Skeet.

My Twitter Worth Mentioning(?)

“..I’d explain why, but I have to, like, go put on lipstick.”-@aalear responding to a comment “females are too busy being beautiful” #gogirl 8:25 PM Sep 15th

My recent blog posts have generated a lot of feedback among my friends & colleagues. I’m glad. It’s a great conversation. 12:09 AM Sep 15th

I’m going to create a restaurant review site & call it StickyTables.com 12:51 PM Sep 13th

“Clean Code is a design philosophy more than a naming convention.” – me #justQuotedMyself #dealWithIt ;-) 12:36 PM Sep 13th

If somebody says my code sucks & they’ll redo it, I’d be hurt. But for my design, I’m relieved. #msPaintSucks ;-) 10:56 AM Sep 13th

Just saw a really cool job title on LinkedIn “Experienced Code Poet” 11:04 PM Sep 12th

When I hear “the cloud” I know I no longer understand what the other person is talking about. #widelyMisusedTerm 9:58 PM Sep 12th

Friday; I love you, but you come way too fast. #in 7:12 AM Sep 17th

Even when they score a major coup to attract & add value to users, the announcement is littered with comments like “Who uses @MySpace? ” 11:16 AM Sep 16th

Got to say; one of the biggest challenges I have blogging is coming up /w relevant examples. 8:17 AM Sep 16th

#FF @unclebobmartin not for his Clean Code msg, which is awesome, but for addressing your questions & concerns. #wayToGo about 19 hours ago

A programmer started to cuss Cause getting 2 sleep was a fuss As he lay there in bed Looping round in his head Was while(!asleep()) sheep++; 9:13 PM Sep 8th (this wasn’t mine, it was quoted from a StackOverflow question)

In my opinion ‘no written requirements’ is the biggest kiss of death a project can have. #stackexchange http://bit.ly/d3rho2 5:42 PM Sep 8th

I don’t hate technical buzzwords, only the ones non-tech people have hijacked. 4:17 PM Sep 16th

Wisdom from Twitter

utahkay @utahkay We’ve outsourced your project to a colony of bacteria. Not the best programmers, but you can get a lot of them for cheap. 6:08 PM Aug 28th

Karen Lopez @datachick “repay technical debt as soon as possible” @drsql #24HOP #DarcVC 6:19 PM Sep 16th

Justice Gray @justicegray I have a dream that one day it will take more to be an “industry expert” in software development than just calling yourself one publicly. 8:37 PM Sep 14th

Ben Alabaster @BenAlabaster You had me at “Hello World!” #ProgrammerPickupLines 7:40 PM Sep 17th

Jim Minatel @wrox Stupid password policies just make people use stupid passwords 12:01 PM Sep 17th

Michael Kramer @MichaelKramer Sorry IE9, I can’t use you. I’m still too pissed about IE6. 4:08 PM Sep 15th

Carlos Figueroa @carlosfigueroa If you can’t be replaced, you probably can’t be promoted. 12:56 PM Sep 9th

Chris Ammerman @cammerman Everyone thinks their opinions are rational, but often they’re merely rationalized. 11:07 AM Sep 9th

Kent Beck @KentBeck if you don’t have a good name for it, give it a bad name. a really, really bad name so you’ll fix it later. 7:31 PM Sep 8th

Knowles Atchison Jr @_KYA “The beauty of the Internet is you can just pull a quote out of your ass and attribute it to whoever the fuck you want.” – George Washington 8:19 PM Sep 7th

Recommended links found this week

Periodic Table of the Elements (via John D Cook)

Threading in C#

Why software development metaphors always fail

Finally The Entire Interview with Uncle Bob Martin

Khan Academy

Tools and Utilities for the .NET Developer

Videos this week

This one is freakin hilarious.  Here’s a quote “Does /dev/null/ support sharding?”  Mongo DB is web scale Unfortunately, I can’t embed it.  Here’s the sequel Episode 2 – All The Cool Kids Use Ruby.  I also saw MySQL is Not ACID Compliant, which is basically the same as Mongo DB is Web Scale, but about MySQL.

Here’s a great quote from the following video:  “How many languages to you have to know to get a god damn web page up?” – @unclebobmartin

September 19, 2010 Posted by | Uncategorized | Leave a comment

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

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

Procedure Like Object Oriented Programming

In a previous post What’s wrong with the Nouns/Adjective/Verb object oriented design strategy, I talked about how verbs should be implemented in their own separate class instead of as a method strapped onto an entity class. In my opinion, it’s an appropriate way to work with processes and pass those processes around, while keeping code flexible, testable, and highly maintainable.

But it has led to comments on Twitter and a link to one of Steve Yegge’s post Execution in the Kingdom of Nouns. Basically, Steve said that turning verbs into nouns was a bad idea (at least that’s what I think he was getting at, there were a lot of metaphors in there :-).

It’s easy to see Yegge’s point of view, if you just leave it at that. After all turning your single line of code accessing those actions

commentData.Insert(cn);

into multiple lines of calling code, when you move the logic into its own class,

using (CommentInsertCommand insCmd = new CommentInsertCommand(cn))
{
    insCmd.Execute(commentData);
}

definitely sucks.

So why not add a static method to the process class so you can access it with a single, procedural like, call?

public class CommentInsertCommand : IDisposable
{
    ....

    public static void Execute(CommentData commentData, int userId, SqlConnection cn)
    {
        ValidateCommentParameter(commentData);
        using (CommentInsertCommand insCmd = new CommentInsertCommand(cn))
        {
            commentData.CommentId = insCmd.Execute(commentData, userId);
        }
    }

    protected static void ValidateCommentParameter(CommentData commentData) {...}
}

This way your call is reduced to

CommentInsertCommand.Execute(data, cn);

I think this has merit and is a clean way to manage your classes.

It brings your object oriented code back to a more procedural level.

One problem I haven’t quite figured out yet is the naming. To be honest, I’m a little uneasy about it. I should probably name it ‘Insert’, but that’s redundant with the class name and I’m not crazy about naming it ‘Execute’ or ‘Run’ either. I chose ‘Execute’ in this example so all XXXXCommand classes would be consistent across the application, and the name is consistent with the SqlCommand naming which is important since this class kind of emulates SqlCommand. However, I’d still love to find a better name.

So, the bottom line is this; why not give all your process classes a procedure like entry point?

Why not give more of our object oriented code a procedural language feel?

Copyright © John MacIntyre 2010, All rights reserved

September 10, 2010 Posted by | C#, Code, Programming | 3 Comments

   

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: