Wednesday, 16 November 2011

The links have the zipped versions of the before and after refactoring projects from the Break the If Habit presentation. 

Wednesday, 16 November 2011 00:29:11 (Eastern Standard Time, UTC-05:00)   #     Comments [0]  | 

I headed out to Columbia, Maryland this past weekend to speak at the CMAP Fall 2011 Code Camp. I was shocked that so many people wanted to see code refactoring at 8:45 on a Saturday morning, but was happy to have an excellent crowd.

The presentation was on breaking the If habit.  To steal the abstract…

Branching logic is one of the foundations of computer programming. But like vodka, the If statement is best consumed in moderation. While the first If is harmless, and maybe even a little thrilling, soon there's a second, a third, a fourth and before you know it -- you wake up under your desk with a method you don't even recognize much less understand.

In this session we'll explore some of the common If abuses and learn how to refactor them into more readable and adaptable code. We'll use design patterns to put the demon If back in its bottle and keep it there. Saner code is within reach, and this is the first step to break the the If habit.

The inspiration for this talk was some legacy code I had to update.  It was already sagging under a dense thicket of if statements that crossed two methods and already had 18 paths. I was supposed to add yet another set of if statements to support yet another variation of the report this code generated. 

I just couldn’t bring myself to do it.  It had taken me two hours to analyze where the changes had to be made so they wouldn’t break anything else. I knew adding these statements was just stuffing more complexity into two methods that were already bursting with branching logic.  The next change would require even more analysis and would more likely lead to something getting broken.

So, I started ripping out code.  I’d like to claim I was purposefully refactoring to a pattern, but it was really more instinctual, moving the code for each variation into its own class.  What it turned out I was doing was replacing a conditional with polymorphism, or implementing a strategy pattern.

The example from the talk started out looking like this…

public double CalculateProfitSharing(Employee employee, DateTime asOfDate, double grossProfit, EmployeeStats stats)
       {
           TimeSpan lengthofEmployment = asOfDate - employee.StartDate;
           double pool = grossProfit*0.04;
           double profitSharingAmount = 0;
           if(lengthofEmployment.Days > 182)
           {
               double basePartofPool = 1.0/(double)stats.EmployeeCount * pool;
               double loyaltyMultipler = lengthofEmployment.Days/stats.AverageLengthEmploymentinDays;
             
               //current year pro-rate
               double proratedRate = 1;
               if(lengthofEmployment.Days < 365)
               {
                   proratedRate = lengthofEmployment.Days/365.0;
               }
               profitSharingAmount = basePartofPool*loyaltyMultipler*proratedRate;

               if(employee.Sales)
               {
                   profitSharingAmount = 0;
               }
               else if(employee.Level < 4)
               {
                   if(employee.Level == 2)
                   {
                       profitSharingAmount *= 1.5;
                   }
                   else if(employee.Level == 3)
                   {
                       profitSharingAmount *= 2;
                   }
                   if (profitSharingAmount  > 10000)
                   {
                       profitSharingAmount = 10000;
                   }
               }
               else
               {
                   profitSharingAmount *= 2.5;
                   if(profitSharingAmount > 5000)
                   {
                       profitSharingAmount = 5000;
                   }
               }
           }
           if(profitSharingAmount <0 & employee.Level < 4)
           {
               profitSharingAmount = 0;
           }
           if(employee.Level == 4)
           {
               if(profitSharingAmount < -5000)
               {
                   profitSharingAmount = -5000;
               }

               if(profitSharingAmount > 0)
               {
                  
                   if((asOfDate - employee.LastOptionGrant).Days<= 365)
                   {
                       profitSharingAmount = 0;
                   }
               }
           }
           if(employee.Level < 4)
           {
               if(employee.ChoseDeferred)
               {
                   profitSharingAmount *= 1.25;
               }
           }
           return Math.Ceiling(profitSharingAmount);
       }

And ended up looking like this...

public double CalculateProfitSharing(Employee employee, DateTime asOfDate, double grossProfit, EmployeeStats stats)
{
     var profitSharingLevel = GetProfitSharingLevel(employee);
     return profitSharingLevel.CalculateProfitSharing(employee, asOfDate, grossProfit, stats);
}

private ProfitSharingEmployeeLevelBase GetProfitSharingLevel(Employee employee)
{
     if(profitSharingLevels.ContainsKey(employee.Level))
     {
         return profitSharingLevels[employee.Level];
     }

     return new ProfitSharingEmployeeLevelNullObject();
}

There is obviously some code in the new classes that encapsulates the variations so there is not a reduction in the total amount of code. But every method becomes much each easier to understand when you don’t have scroll to through pages of nested if statements.  It also becomes easier to change and maintain code when your rules for each variation aren’t tangled together in gobs of if statements.

The presentation also included how to use the Null Object Pattern so you can stop sprinkling your code with null checks (there’s an example above – just imagine GetProfitSharingLevel is public and is called by more than one other method). It was also supposed to show the replacement of branching logic with the Decorator Pattern, but we ran out of time. I’ll try to explore these in more detail in other blog posts, but not tonight.

I think the root cause of most If abuse is the refusal of developers to create objects, especially small ones.  I think there is an innate aversion to creating seemingly trivial objects no matter how helpful they might be.  Packaging two pieces of related information and/or a method together should be reason enough to make a new class, but that doesn’t seem to pass the “I need a new class” threshold for most developers. I know it’s below mine. 

For instance, implementing a calculation for a tiered sales commission is usually done with if statements. 

                double commission = 0;
                 if (yearSales < yearTarget)
                 {
                     commission = 0.005 * yearSales;
                 }
                 else if (yearSales < yearTarget*2)
                 {
                     commission = 0.01 * (yearSales - yearTarget) + 0.005 * yearTarget;
                 }
                 else
                 {
                     commission = 0.02*(yearSales - yearTarget*2) + 0.01 * yearTarget + 0.005 * yearTarget;
                 }
                 compensation += commission;

And it’s fine, it works, it is even fairly compact  – but it takes a while to understand, extending it requires additional if statements and it is buried inside a method – wholly unusable anywhere else.  But the worst sin is that we have a clear object here with two properties and a method, and we have denied its existence.

What we should do is create a small object…

public class CommissionTier
    {
        private double _tierStart;
        private double _rate;

        public CommissionTier(double tierStart, double rate)
        {
            _tierStart = tierStart;
            _rate = rate;
        }

        public double TierStart
        {
            get { return _tierStart; }
        }

        public double GetCommision(double sales)
        {
            return (sales - TierStart)*_rate;
        }

        public override string ToString()
        {
            return string.Format("Tier Start:{0:c0}, Rate {1:0.00%}", _tierStart, _rate);
        }
    }

Then we can use a looping structure that is more clear about what we’re doing. It is also now easily extensible.  To add a tier, we just need to add a new instance of the CommissionTier object to the collection – no logic changes required.  Heck we could even drive tier creation from a database…

_tiers.Add(new CommissionTier(yearTarget * 2, 0.02));
_tiers.Add(new CommissionTier(yearTarget, 0.01));
_tiers.Add(new CommissionTier(0, 0.005));

double commission = 0;
double workingSales = yearSales;
foreach (CommissionTier tier in _tiers)
{
    if (workingSales > tier.TierStart)
    {
        commission += tier.GetCommision(workingSales);
        workingSales = tier.TierStart;
    }
}  

By promoting the Commission Tier to an object instead of leaving it strewn about in manually related variables, magic numbers and calculations, all of sudden we have all the tools of OOP available to us. Our logic and our data are packaged together nicely.  The object has control of its data and we can leverage the power of collections. 

So instead continuing to pack procedural programming spaghetti into our methods, we need to be object oriented designers and create more frickin’ objects. Only then can we begin to break the If habit.

Wednesday, 16 November 2011 00:03:34 (Eastern Standard Time, UTC-05:00)   #     Comments [0]  | 

Theme design by Dean Fiala

Pick a theme: