I’ve moved

I’ve finally moved my blog to my own hosting and my own domain. I did this primarily so I could have more control over the site, and a little more control over the styles and plug-ins.

Here’s a few of my recent posts:

Discovering Typemock is about my frustration mocking some static methods, the ridiculous hoops I was forced to jump through, and the clean implementation I was finally able to do with Typemock.

My take on identifier semantics (Id vs No vs Code vs Key) is just my simple conventions for these popular identifier names. I don’t believe they’re all the same, and should be used under different circumstances.

My new blog is at http://www.whileicompile.com

Consultants are advisors, not decision makers.

Overview

I was having lunch with my friend & colleague last week and we had a disagreement about whose decision it is to make a change when you see something wrong in the client’s software.

Mechanic analogy

My colleague used an analogy about a mechanic; it was a good one, so I’ll use it here.

Let’s say you bring your car in for a $30 oil change, and your mechanic notices a problem with the timing belt.  My colleague suggests, he doesn’t just ignore it, he tells you it’s an emergency must be changed immediately (the emphasis is my friends).

Well I agree the mechanic shouldn’t just ignore it, and I was relieved my friend didn’t suggest the mechanic should simply change the timing belt, driving the bill from $30 to $930, but I’m not sure if telling the car owner they must change the timing belt ‘now’ is appropriate either.

In my opinion the mechanic should tell the customer what he found, the risk in not fixing it, outline the options to fix it, and the cost & associated risk with each option[1].  Then make a recommendation.

It’s the customer’s decision, not the mechanics, and even if the customer makes a foolish decision, it’s his decision, not the mechanics.

Fortunately, all the mechanics I’ve dealt with seem to understand this.

As consultants we’re advisors, not the decision maker

In my opinion, our role as consultants / advisors includes the responsibility to inform the client of any problems or potential problems you’ve noticed, the risks in not fixing it, options to resolve the problem, along with the costs and risks associated with each.

Unethical behaviour

To me it seems unethical to go rogue, and just start making changes the client doesn’t know about, and didn’t approve.

It’s also unethical to purposely instil fear, uncertainty, and doubt when explaining the options to the client so they make the decision you want them to; regardless of your intentions.

The client owns your time

After all, the client does own the software, is responsible for its maintenance, and they do own your time to direct as necessary.

Wait, what?  Who owns your time?

Yeah, when you’re consulting, your client (or employer) owns your time.  … you sold it to them; remember?

Every consulting and/or employment agreement is different, but if you’re charging by the hour, it probably says something like this; they bought your focused efforts to solve their problems for a specific duration.

And how they spend your time (their resource) is up to them (within reason[2]).

So deciding feature X is more important than feature Y, or bug X is more important than bug Y, is their call, not yours.

An extreme example

But what if it’s a big important issue?  What if it’s a major security flaw in the banking software you work on.  The flaw is dangerous. The flaw could potentially put users at risk. The flaw could even push the company into bankruptcy.

But your manager doesn’t want to invest the time to fix it and his manager stands behind him.  You’ve exhausted all other paths of reason, to the point of being on the verge of getting fired.

You still don’t have the right to change it … not even on your own time, because the software is their asset, not yours.

Your decision

Now I’m not saying you have no say, you can refuse.  Bottom line: if they refuse to make a change that you feel is necessary and/or potentially dangerous, you can give notice and quit.

Exceptions

Like everything, there are exceptions to this general rule.  Here are 2:

The first is if you are working on one task, notice another problem, and can fix it without seriously adjusting your time budget on the original task.  An example of this might be noticing and fixing a 1-off error in a loop.  I once had the responsibility of making manual year end changes to customer databases, with an estimated time of 3 days for each.  It took me 4 iterations working smart within the allotted time to write and test a utility which dropped that task to a 1 hour job[3].  The utility was separate, so I didn’t change the base product, and it cost my employer nothing.  So it is possible to make an impact working like this.

The second exception is you have built up a lot of trust with the client, and are completely unsupervised, making whatever changes deemed necessary to accomplish the client’s goals.  I’ve been in this situation a few times, and can honestly say, it’s a rare situation to be in.  It’s reserved for the situation where the client trusts your character and your responsibilities are so mysterious that they cannot make a decision, so they leave it up to you.[4]

In Summary

So basically, in my opinion, as consultants it’s not within our rights to make the decision to change or coerce the client into allowing you to change something.  It’s our obligation to inform them of the problem, outline their options along with the risks and cost of each, and make recommendations.

… that’s it.

---

[1] Yeah, there aren’t too many options to discuss for changing a timing belt, but we’re discussing software remember.

[2] There are limits of course, everybody will have their own personal & ethical limitation, and many (not enough though) will have professional limitations of what they’re willing to do.  Professional limitations might include, I was hired as a programmer, not a janitor, or worse yet, and Access developer.  ;-)h

[3] I could’ve dropped it to 5 minutes, but 1 hour is where the 80/20 principle told me to stop.  Somebody did this after I left, and it took significantly longer.

[4] Neither my colleague or I are currently in this situation.

Response to Seth Godin’s – Where do ideas come from?

Seth Godin is wrong

I just read Seth Godin’s blog post Where do ideas come from? And in my opinion he completely missed it.

Sorry, I think Seth is awesome as do many others, but on this one, I don’t think he really answered the question; where do ideas come from?  Instead, in my opinion, what he wrote was more a list of favourable conditions

While I’m no Seth Godin, I’m going to share my understanding of ideas and their source[1].

I hope I don’t need to explain why a post about ideas and creativity is on a programming blog.

Ideas come from questions

Ideas are answers to those questions.

Although it may appear that ideas come out of nowhere while driving to work or zoning out in the shower, they are really answering previously asked questions burning in your subconscious.  There are many conditions which help answers come more easily, but they never come if you don’t have the question in the first place.

Questions need to be installed into your subconscious

A fleeting question doesn’t quite make it into your subconscious to be answered later.  The question needs to be important enough to be prioritized by your mind.

Installations depth comes from emotion

The importance of the question is largely based on the intensity of the emotion driving the question.  That’s why many innovations come during massive change and crisis.  Fear and greed inspire many ideas.

New information is assimilated to answer these questions

New questions are never installed for questions you already have the information needed to answer it.  So you need more information to answer the question and will need time to assimilate new information which is filtered against your question.

This information can be from; feedback from action, random thoughts in the shower, synergy, social media, blogs, news, conversations, books, radio, lectures, etc…  And yes, Seth even television[2]

Fear constrains ideas

Fear eliminates ideas.  Whether criticism of a bad idea, or constraint, or even a well-meaning, ‘helpful’ person who’s experienced proves it cannot be done.

When people fear criticism, ideas are never put forth which may inspire better ideas.  All ideas must be valued for an innovative environment.

Forcing ideas constrains you to existing information

You’ll hear many people say that you sit down, brainstorm, make your decisions quickly and take immediate action, which is great, when you have all the information you need.  But unless you have all the information you need, forcing it quickly usually won’t give you an awesome idea.

Synergy is creativity’s compound interest

Ideas usually require time, but they can be sped up with synergy[3].

Synergy is instantaneous compound interest for creativity.

One idea (good or bad) leads to another idea, then another, and another.  Before you know it, the feedback cycle has taken you down an entirely different path.

But Seth was right about one thing

Ideas that don’t ship are worthless

---

[1] I’m also not a psychologist, so take this as more my own, possibly misunderstood, idea of how ideas happen.

[2] Television may not appear to be a motivator for ideas, because most people watch TV passively, but if your questions are installed deeply enough, it can still be a half decent source of information.

[3] I realize the word Synergy has been over used as a business buzzword, but it is a real concept.  Don’t let the buzzword throw you off.

My reaction to being named as a Canadian programmer worth following on Twitter

Yesterday John Bristowe published a list of Developers in Canada You Should Follow on Twitter.

I was humbled and honoured to make the list. … actually, I was a little more excited than that, here’s a dramatization ….

… Thanks John … I’ve been dying to use this video in a blog post. 😉

The UI programmers (not so) secret weapon

An Example:

Suppose you had software which matches buyers and sellers, and new users are created via a ‘new user’ wizard[1].  Let’s say the wizard has 4 pages for Basic User Info, Review, Processing, and Completion Status Report.  And there are 5 buttons; Cancel, Previous, Next, Run, and Finish.

Cancel is displayed from the Basic User Info, Review, and Processing pages.
Previous is displayed from the Review page.
Next is displayed from the Basic User Info page.
Run is displayed from the Review page.
Finish is displayed from the Completion Status Report page.

This isn’t difficult to manage the display from the events[2] right?

Well … in reality, it doesn’t take much of a change for your simple display functionality to become …

The Problem

Manipulating application display during events quickly turns into a complex, bug riddled, difficult to maintain, mess.

It may not seem like that big of a deal when you have only a few controls and a very simple (or no) workflow.  Actually, you may argue, managing visual display in the events may even seem like the most efficient strategy.  After all, you will never display, disable, or otherwise needlessly manipulate a control, but as your application grows more complex, this approach can leave you tearing your hair out[3].  This is especially true when you have multiple execution paths leading to the same event handlers with different state.

What if you received a change request for the above requirements, where additional info is required for buyers or sellers, resulting in a new wizard page for each, and in addition to that, business sellers require a page to gather Tax Information?

Your complexity has started to grow, resulting in buyers have a workflow of
Basic User Info -> Buyer Info -> Review -> Processing -> Completion Status Report

Individual sellers have a workflow of
Basic User Info -> Individual Seller Info -> Review -> Processing -> Completion Status Report

And commercial sellers have a workflow of
Basic User Info -> Commercial Seller Info -> Tax Info -> Review -> Processing -> Completion Status Report

Notice how these changes lead to different execution paths all arriving at the Review page.  The question then, is; where does the back button on the Review page send the user?  The back button requires logic to know which wizard page to display.

It’s not difficult to imagine the need for logic to be added in more than one place[4], which can result in your display being processed differently from every event.  Not only does this lead to repeated code, but can also create bugs where buttons do/don’t display where they should.

Eventually, as this logic grows increasingly complex, simple, 5 minute, change requests will eventually take you hours to determine there won’t be any side effects.

At this point, developers usually find …

Inadequate Solutions

So how do people manage this?  I mean, beyond having the logic at the control level.

Well, you can put controls for different states on their own panel for their states.  So in the example above, you could add the buttons directly onto the wizard page control[5].  This would ensure the correct buttons, for the panel, are displayed, but it doesn’t solve the ‘Back’ button logic of which page to display.

Or you could add some kind of control grouping mechanism, like a collection of the controls, to display or hide all controls at once.  The problem with this, is what happens when you have functionality which will enable/disable a specific control, or do something else?  Do you add another grouping or another function to manipulate your control group, or do you have that one function do enabling/disabling as well?  What happens if the enabling/disabling logic is different from the display logic?  You also have the problem of one control being in multiple groups; will your groupings need to be called in the order of hide, then show, just to get the control that’s in both groups to display?  And if you do, will you let the control flicker? Or will you add extra functionality to prevent specific controls from hiding in the first place, so it won’t flicker?

Or maybe you can make a centralized function to manage the controls where the display is based on the passed in parameters.  Well this is very close, but no cigar.  Not only are the parameters needless, as I’ll show later, but this won’t completely remove logic from the events and can needlessly increase logic in that function.

The solution most have preferred is to make screens as minimal as possible, with a new screen for each piece of functionality.  This is often effective, at the cost of usability, but sometimes has its own problems like managing state while gathering data from multiple screens to persist it all in one shot.  Not even to mention that this strategy would be ineffective in our example.  Besides, in my experience, you can never really get away from at least some display manipulation.

This is a small example; imagine having a screen with 50 controls, where many of them change state.  Imagine how difficult it would be to manage the display this way.

Hopefully you can see how this can turn into a major pain in the butt.

Well, the good news is this problem has been solved …. Solved a long time ago actually[6].

The solution is ….

The UpdateUI() Method

It’s pretty simple really; just create a single function for the entire page, view, form, or screen (whatever it’s called in your technology).  All this function does, is manage the control display.  Then call this function at the end of every event, after all data manipulation.

It’s pretty simple; there are just …

5 Rules for the UpdateUI() method

1.     Fast – Don’t do anything which isn’t fast.  The state data driving the display decisions should already be calculated.

2.     No parameters – You should be able to make all decisions about display using the current screens state.

3.     No side effects – Don’t manipulate data within this method.  Not only can it slow it down, but it can also lead to inconsistent display evaluations.

4.     Manage each control individually – Don’t hide all controls, then show the controls you want displayed.  This causes flicker and adds unnecessary complexity.

5.     Call it last – call this function at the end of every event.

Code Sample

Here’s a little sample of what the wizard UpdateUI() code might look like.

public void UpdateUI()
{
 basicUserPg.Visible = (CurrentPage == NewUserWizardPage.BasicUserInfo);
 buyerPg.Visible = (CurrentPage == NewUserWizardPage.BuyerInfo);
 individualSellerPg.Visible = (CurrentPage == NewUserWizardPage.IndividualSellerInfo);
 commercialSellerPg.Visible = (CurrentPage == NewUserWizardPage.CommercialSellerInfo);
 commercialSellerTaxPg.Visible = (CurrentPage == NewUserWizardPage.CommercialSellerTaxInfo);
 reviewPg.Visible = (CurrentPage == NewUserWizardPage.Review);
 processingPg.Visible = (CurrentPage == NewUserWizardPage.Processing);
 compeletedPg.Visible = (CurrentPage == NewUserWizardPage.CompletionStatusReport);

 cancelButton.Visible = (CurrentPage == NewUserWizardPage.BasicUserInfo
                     || CurrentPage == NewUserWizardPage.BuyerInfo
                     || CurrentPage == NewUserWizardPage.IndividualSellerInfo
                     || CurrentPage == NewUserWizardPage.CommercialSellerInfo
                     || CurrentPage == NewUserWizardPage.CommercialSellerTaxInfo
                     || CurrentPage == NewUserWizardPage.Review);
 previousButton.Visible = (CurrentPage == NewUserWizardPage.BuyerInfo
                     || CurrentPage == NewUserWizardPage.IndividualSellerInfo
                     || CurrentPage == NewUserWizardPage.CommercialSellerInfo
                     || CurrentPage == NewUserWizardPage.CommercialSellerTaxInfo
                     || CurrentPage == NewUserWizardPage.Review);
 nextButton.Visible = (CurrentPage == NewUserWizardPage.BasicUserInfo
                     || CurrentPage == NewUserWizardPage.BuyerInfo
                     || CurrentPage == NewUserWizardPage.IndividualSellerInfo
                     || CurrentPage == NewUserWizardPage.CommercialSellerInfo
                     || CurrentPage == NewUserWizardPage.CommercialSellerTaxInfo);
 runButton.Visible = (CurrentPage == NewUserWizardPage.Review);
 finishButton.Visible = (CurrentPage == NewUserWizardPage.CompletionStatusReport);
}

This is quite a bit simpler than having the code sprinkled throughout the screen’s logic.

You may also want to move many expressions into their own protected properties. For example, you may want to change …

 cancelButton.Visible = (CurrentPage == NewUserWizardPage.BasicUserInfo
                     || CurrentPage == NewUserWizardPage.BuyerInfo
                     || CurrentPage == NewUserWizardPage.IndividualSellerInfo
                     || CurrentPage == NewUserWizardPage.CommercialSellerInfo
                     || CurrentPage == NewUserWizardPage.CommercialSellerTaxInfo
                     || CurrentPage == NewUserWizardPage.Review);

… to …

cancelButton.Visible = DisplayCancelButton;

Unfortunately, if your screen has a lot of controls, you may end up with a lot of these properties.  Because the state used to make the display decisions is usually in that screens class, it’s difficult to move these expressions into a separate class.  Or at least it is difficult to make a generalized statement about how to do it.

Where to use

This technique can be used in every GUI language or technology I’ve ever worked with.  I’ve used this strategy in Classic VB, C++ (MFC[7], Win32), Classic ASP, PHP, JavaScript, ASP.NET webforms[8] , etc…

This technique can significantly reduce Ajax initiated display adjustments.

Costs

The cost to managing your display this way is a slight increase in computing power required to calculate every display decision on every event.  However, in reality, if your UpdateUI() function causes a performance issue that’s noticeable, you’ve probably missed one of the rules stated above.

In my opinion, the increased simplicity, and cleanliness of you code, more than makes up for the slight increase in processing.

Conclusion

The UpdateUI() method is an efficient and effective way to keep your display manipulation code clean & bug free.  Even if your screen is relatively simple, you still benefit from having all your code in one place, and if it does grow in complexity, it will be manageable.  This technique has saved me countless hours of frustration.

Copyright © John MacIntyre 2010, All rights reserved

---

[1] These examples are really difficult to come up with, so let’s just go with this.  In reality many screens will be much more complex as well.

[2] I don’t necessarily mean ‘code behind your buttons’, but any logic executed specifically for this button, as a result of clicking it.

[3] For proof, see my avatar.  😉

[4] As if one place isn’t enough

[5] Wizard page controls are on their own panel of course, I mean we’ve all definitely learned that haven’t we?

[6] MFC had UpdateUI() as a virtual method on its main window class in the mid-1990s.  Fortunately, this was one of the only new paradigms Microsoft has ever added which did not undo the solution I came up with in the last paradigm

[7] The name actually comes from the CView::UpdateUI() method in MFC which was a core method for all UI related activity, before MFC I was calling it EnableCtrls().

[8] In ASP.NET WebForms, this function is called Page.PreRender.  However, this solution isn’t the greatest, since any control with an event needs to be populated in Page.OnInit just to wire up events properly.

Thanks to Cory Fowler for reviewing my post.

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 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

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

@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

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

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

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

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

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

@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

@_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

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

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

My Clean Code Experience No. 1 (with before and after code examples)

Public Code Review
Robert C. Martin was kind enough to review the code in this post at on his new blog Clean Coder. Be sure to read his review when you finish reading this post.

Introduction

Clean Code

After expressing an interest in reading Robert C Martin‘s books, one of my Twitter followers was kind enough to give me a copy of Uncle Bob’s book Clean Code as a gift*. This post is about my first refactoring experience after reading it and the code resulting from my first Clean Code refactor.

Sample code

The code used in this post is based on the data access layer (DAL) used in a side project I’m currently working on. Specifically, my sample project is based on a refactor on the DAL classes for comment data. The CommentData class and surrounding code was simplified for the example, in order to focus on the DAL’s refactoring, rather than the comment functionality. Of course; the comment class could be anything.

Download the my clean code refactor sample project (VS2008)

Please notice:
1. The database can be generated from the script in the SQL folder
2. This code will probably make the most sense if you step through it
3. This blog post is about 1,700 words, so if you aren’t into reading, you will still get the jist of what I’m saying just from examining the source code.

What Clean Code isn’t about

Before starting, I want to point out that Clean Code is not about formatting style. While we all have our curly brace positioning preferences, it really is irrelevant. Clean Code strikes at a much deeper level, and although your ‘style’ will be affected tremendously, you won’t find much about formatting style.

My original code

Original comment DAL class

My original comment DAL class is in the folder called Dirty.Dal, and contains one file called CommentDal.cs containing the CommentDal class. This class is very typical of how I wrote code before reading this book**.

The original CommentDal class is 295 lines of code all together and has a handful of well named methods. Now, 295 lines of code is hardly awful, it doesn’t seem very complex relatively speaking, and really, we’ve all seen (and coded) worse. Although the class interface does seem pretty simple, the simplicity of its class diagram hides its code complexity.

public static void Create(IEnumerable<CommentData> Comments, SqlConnection cn)
{
    // validate params
    if (null == cn) throw new ArgumentNullException("cn");
    if (cn.State != ConnectionState.Open) throw new ArgumentException("Invalid parameter: connection is not open.", "cn");
    if (null == Comments) throw new ArgumentNullException("Comments");
    foreach (CommentData data in Comments)
    {
        if (data.CommentId.HasValue)
            throw new ArgumentNullException("Create is only for saving new data.  Call save for existing data.", "data");
    }

    // prepare command
    using (SqlCommand cmd = cn.CreateCommand())
    {
        cmd.CommandText = "ins_comment";
        cmd.CommandType = CommandType.StoredProcedure;
        // add parameters
        SqlParameter param = cmd.Parameters.Add("@comment_id", SqlDbType.Int);
        param.Direction = ParameterDirection.Output;
        cmd.Parameters.Add("@comment", SqlDbType.NVarChar, 50);
        cmd.Parameters.Add("@commentor_id", SqlDbType.Int);

        // prepare and execute
        cmd.Prepare();

        // update each item
        foreach (CommentData data in Comments)
        {
            try
            {
                // set parameter
                cmd.Parameters["@comment"].SetFromNullOrEmptyString(data.Comment);
                cmd.Parameters["@commentor_id"].SetFromNullableValue(data.CommentorId);

                // save it
                cmd.ExecuteNonQuery();

                // update the new comment id
                data.CommentId = Convert.ToInt32( cmd.Parameters["@comment_id"].Value);
            }
            catch (Exception ex)
            {
                string msg = string.Format("Error creating Comment '{0}'", data);
                throw new Exception(msg, ex);
            }
        }
    }
}

This method can be simplified dramatically into a more readable style with fewer control statements.

But first, notice how the methods are segmented into line groupings which are similar, with each grouping isolated with a single line of white space both before & after it, plus a comment to prefix most groupings. Each of these groupings is a smell, indicating each should be its own method.

Before reading Clean Code, this was clean to me … this was beautiful code to me.

My new ‘clean’ code

The comment DAL classes after refactoring

I’ve got a feeling I missed a lot in this book and will probably end up rereading it several times, but the biggest takeaways from reading it in my first pass were:

Smaller well named classes & methods are easier to maintain and read. You may notice in the Clean.Dal directory, the classes are smaller, with file sizes hovering around the 50 LOC mark. 50 LOC for an entire class, when in the past, only my smallest methods would be less than 50 LOC. I’ve now realized; no code grouping is too small to separate into its own property, method, or even class***. Sometimes it’s wise to refactor a single expression into a property just to label it****.

Here is the equivalent of my new Create method:

public static void Execute(IEnumerable<CommentData> comments, int userId, SqlConnection cn)
{
    ThrowExceptionIfExecuteMethodCommentsParameterIsInvalid(comments);
    using (CommentInsertCommand insCmd = new CommentInsertCommand(cn))
    {
        foreach (CommentData data in comments)
            data.CommentId = insCmd.Execute(data, userId);
    }
}

From the above code, you may notice not only how much smaller and simpler the ‘Create’ method has become, but also that its functionality has been moved from a method to its own smaller class. The smaller class is focused on its single task of creating a comment in the database and is therefore not only easier to maintain, but will only require maintaining when a very specific change in functionality is requested, which reduces the risk of introducing bugs.

The small class / property / method idea extends to moving multi-line code blocks following control statements into their own methods.

For example:

while(SomethingIsTrue())
{
    blah1();
    blah2();
    blah3();
}

Is better written as

while (SomethingIsTrue())
    BlahBlahBlah():

With the ‘while’s block moved into its own BlahBlahBlah() method. It almost makes you wonder if having braces follow a control statement is a code smell, doesn’t it? *****

Also, as part of the small well named methods idea, detailed function names make comments redundant & obsolete. I’ve come to recognize most comments are a code smell. Check this out; my colleague Simon Taylor reviewed my code while I was writing this, pointed out that although my dynamic SQL was safe, colleagues following me may not see the distinction of what was safe, and may add user entered input into the dynamic SQL. He suggested a comment for clarification.

He was absolutely right, but instead of adding a comment, I separated it into its own method, which I believe makes things very clear. See below:

protected override string SqlStatement
{
    get
    {
        return GenerateSqlStatementFromHardCodedValuesAndSafeDataTypes();
    }
}

protected string GenerateSqlStatementFromHardCodedValuesAndSafeDataTypes()
{
    StringBuilder sb = new StringBuilder(1024);
    sb.AppendFormat(@"select	comment_id, 
                                    comment, 
                                    commentor_id
                        from		{0} ",
                    TableName);
    sb.AppendFormat("where      Comment_id={0} ", Filter.Value);
    return sb.ToString();
}

Not only is this less likely to go stale, it will also clearly identify exactly what is going on both at the method declaration and everywhere it is called.

Moving control flow to the polymorphic structure is another technique to achieve clean code. Notice the ‘if’s in the ‘Clean.Dal’ version are pretty much reserved for parameter validation. I’ve come to recognize ‘if’s, especially when they deal with a passed in Boolean or Enum typed method parameters as a very distinct code smell, which suggests a derived class may be more appropriate.

Reusable base classes are also a valuable by-product of writing clean code. A reusable class library is not the goal in itself, in an anti-YAGNI kind of way, but is instead a natural side effect of organizing your code properly.

Clean code is also very DRY. There is very little if any duplicated code.

The structure is 100% based on the working, existing, code, and not on some perceived structure based on real world domain is-a relationships which don’t always match up perfectly.

One more thing, unfortunately not everything is going to fit nicely into the object oriented structure, and sometimes a helper class or extension method will be the best alternative. See the CommentDataReaderHelper class as an example.

A closer look

Here’s a quick overview of class hierarchy, we’ll use CommentUpdateCommand as an example:

New CommentUpdateCommand class and its inheritance hierarchy.

One of the first things you may notice about this class, is there only one entry point; the static method Execute(). This provides a very simple and obvious way to use the class. You can place a breakpoint in this method and step through the class and its hierarchy to fully understand it.

The class hierarchy is based completely on existing working code and was designed to share functionality effectively without duplicating code. Each class is abstracted to the point it needs to be, and no more, yet, the duties of each class in the hierarchy is crystal clear as if it was designed from the beginning to look like this.

Here are the classes,
The AppCommandBase class manages an SqlCommand object.
The ProcCommandBase class executes a given stored procedure.
The CommentCommandBase class shares functionality specific to the Comment table
The CommentUpdateCommand class implements functionality specific to the comment update stored procedure and hosts the static entry point method ‘Execute’.

The cost of writing Clean Code

When I started writing this, I didn’t know there were any downsides to writing clean code, but once I began this post, and started gathering evidence to prove how awesome the clean code strategy was, some evidence ruled against my initial euphoria. Here is a list of observations which could be looked upon unfavorably.

Increased LOC – This was really pronounced in a later experience (Clean Code Experience No. 2 coming soon), but I actually thought the LOC was decreased in this sample. At least once you take into account the reusable base classes … but it wasn’t. While I haven’t done an in depth analysis of the LOC, it appears that the code specific to the comments, before taking into account any abstract base classes, has a similar LOC count as the Dirty.Dal class. Now much of this is from structure code, like method declarations, but it was still disappointing.

Increased structural complexity – I suppose this should have been obvious, but it didn’t occur to me until writing this blog post, the complexity didn’t really disappear; it’s just been moved from the code to the polymorphic structure. However, having said that, I suspect, maintaining Clean Code with a vigorous polymorphic structure would be lower cost than a traditional code base.

Method call overhead – With all the classes, properties, and methods, it has occurred to me, that method call overhead may have a performance trade off. I asked Uncle Bob about this on Twitter, and his reply was “Method call overhead is significant in deep inner loops. Elsewhere it is meaningless. Always measure before you optimize.”. Pretty good advice I think.

But even after realizing the trade offs, Clean Code is still, clearly, the way to go.

In Summary

The Clean Code book has changed my outlook on how I write, and I think everyone should be exposed to this material. I’ve already re-gifted my Clean Code book, and the drive to write this blog post comes from a burning desire to share this information with my new colleagues.

I would love to hear your input on this blog post, my sample code, and the whole Clean Code movement. Please comment below.

Reminder
Don’t forget to read Uncle Bob’s review at on his new blog Clean Coder.

* Why did somebody I’ve never met send me a book as a give? Because he wanted me in the C.L.U.B. (Code Like Uncle Bob). And to answer your next question, yes I have since re-gifted the book.
** Very typical of how I wrote code before reading Clean Code … with the exception of some pretty nasty exception handling code. I removed it not due to its ugliness, but because its ugliness may have taken away from the code logic refactoring that I am trying to emphasize.
*** And when I say no code is too small, stay tuned for Clean Code Experience No 2, which was so unconventional, it left me questioning if I’d taken this concept way too far.
**** Although to be fair, I my first exposure to this idea was from Scott Hanselman after reading his first MVC book (this link is to his second MVC book).
***** Wrap your head around that, those of you who feel a single line following a control statement should have braces. LOL

Copyright © John MacIntyre 2010, All rights reserved

PS-Big thanks to Ben Alabaster to pushing me to write this post.

What’s wrong with the noun/adjective/verb object oriented design strategy

The programming course I took way back in 1993 was basically a 1 year intro to C. And like any good student, as I learned the language, I also started learning about code reuse and experienced a delightful satisfaction every time I realized 2 different functions had similar code which could be moved into a new function.

Eventually, I noticed a pattern, in that I would write a bit of functionality until it worked, then I would refactor it into a more elegant solution with as little repeated code as possible.

My code evolved quite nicely.

Then I learned C++, object oriented programming, and was introduced to the holy grail of object oriented design advice, which went something like this:

Take your requirements and circle all the nouns, those are your classes. Then underline all the adjectives, those are your properties. Then highlight all your verbs, those are your methods

This Noun / Adjective / Verb design strategy seemed like the most ingenious piece of programming wisdom ever spoken … but it’s led us down a misguided path.

It’s the verb that’s misunderstood. The verb should be another class, not a method. It should be a process class. As a programming concept, a process is just as much a ‘thing’ as any real world object. The verb should be a class, which accepts the noun as an input to be processed.

But there’s also another problem; Up until this little shortcut was articulated, code was structured based on the implemented code, with similar functionality refactored into its own reusable units, but once noun/adjective/verb idea became widespread, code was suddenly structured according to domain.

For example, the domain focus was really evident in the way we structured our ‘is-a’ relationships, with inheritance being based more on the real world domain, than the implementation code.

Inheritance should be based on most efficient code reuse, not the domain, because as anybody who has heard the square is not a rectangle* example can attest, sometimes the domain ‘is-a’ relationship just doesn’t work.

* With regards to the square is not a rectangle example, please be aware the solution outlined does not resolve the problem, as described in Uncle Bobs comment. The 1996(?) magazine article, The Liskov Substitution Principle is available, which contains the example as originally described. I didn’t post this url, since it’s not focused exclusively on the square / rectangle issue.

Copyright © John MacIntyre 2010, All rights reserved