September 15, 2010

Reply: What is too simple and small to refactor

This code is partly inspired by Cory Fowler's refactoring of code provided by John MacIntyre in his last blog post - "What is too simple and small to refactor". John used the inheritance model to remove a boolean parameter he didn't like, Cory's was a slight departure and introduced funcs to inject the calculations, but still stuck with the inheritance model. I like Cory's approach, but I wanted to see if I could use funcs to entirely ditch the inheritance model - mostly just to see how far I could take the concept he introduced. My base calculations are all collated into a single static class with properties for each calculation returning a Func<float, float, float>. Each instance of employee has the calculation Func<> pushed in using dependency injection. This means that an employee could have an entirely custom calculation without requiring an instance of WageBase/CalculationBase or whatever other inheritance scheme was used in the other posts.
My WageCalculations class is basically a bunch of properties that return Func<float, float, float> this allows us to reference the calculations in a statically typed manner WageCalculations.BasicWithoutOvertime, WageCalculations.BasicWithOvertime etc.
My employee class is very simple, one of the constructors allows the PayCalculation property to be specified at instantiation. When we need to run the calculation, we just call empInst.CalculatePay(); So here's my employee class, nothing complicated:
Disclaimer: In the interest of keeping this blog post as short as possible none of the demonstration code contains any exception handling. Exception handling can however be found in the downloadable project at http://dl.dropbox.com/u/3029830/Prototypes/Prototype%20-%20WageCalculator.zip.
public class Employee
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public float BaseRate { get; set; }

    public Func<float, float, float> PayCalculation { get; set; }

    public Employee(string firstName, 
                    string lastName, 
                    float baseRate, 
                    Func<float, float, float> payCalculation)
    {
        FirstName = firstName;
        LastName = lastName;
        BaseRate = baseRate;
        PayCalculation = payCalculation;
    }

    public float CalculatePay(float periodHours)
    {
        return PayCalculation(periodHours, BaseRate);
    }
}
Here's my PayCalculations class that provides all the basic pay calculations for the system:
public static class PayCalculations
{
    public static Func<float, float, float> BasicWithoutOvertime 
    { 
        get 
        {
            return (hours, baserate) =>
            {
                return hours * baserate;
            };
        }
    }
    public static Func<float, float, float> BasicWithOvertime
    {
        get
        {
            return (hours, baserate) =>
            {
                if (hours < 40) return hours * baserate;
                return ((hours - 40f) * 1.5f + 40f) * baserate;
            };
        }
    }
    public static Func<float, float, float> Salary
    {
        get 
        {
            return (hours, baserate) =>
            {
                return baserate;
            };
        }
    }
    public static Func<float, float, float> Contractor
    {
        get
        {
            return (hours, baserate) =>
            {
                /* Base rate */
                float subtotal = Math.Min(hours, 40) * baserate;
                hours -= Math.Min(hours, 40);
                /* Time plus a half */
                if (hours > 0) subtotal += 1.5f * Math.Min(hours, 20) * baserate;
                hours -= Math.Min(hours, 20);
                /* Double time */
                if (hours > 0) subtotal += 2.0f * Math.Min(hours, 20) * baserate;
                hours -= Math.Min(hours, 20);
                /* Doube time plus a half */
                if (hours > 0) subtotal += 2.5f * hours * baserate;

                return subtotal;
            };
        }
    }
}
Finally I've got my application code that sets up a bunch of new employees and passes their pay calculations into the constructor before calculating their pay for 50 hours:
class Program
{
    static void Main()
    {
        List Employees = new List()
        {
            new Employee("John", "MacIntyre", 40.25f, PayCalculations.BasicWithoutOvertime), 
            new Employee("Ben", "Alabaster", 40.25f, PayCalculations.BasicWithOvertime),
            new Employee("Cory", "Fowler", 2935f, PayCalculations.Salary),
            new Employee("John", "Doe", 150f, PayCalculations.Contractor), 
            new Employee("Jane", "Doe", 0f, (hours, baserate) => 3500),
            new Employee("Joe", "Bloggs", 34.25f, (hours, baserate) => {
                return hours < 15 ? 15 * baserate : hours * baserate;
            })
        };

        float hoursWorked = 50;
        Employees.ForEach(employee => {
            Console.WriteLine("{0}, {1}: ${2:#,###.00}", 
               employee.LastName, 
               employee.FirstName, 
               employee.CalculatePay(hoursWorked));
        });
    }
}
I'm not sure I can make it any simpler with this approach, I think I've taken the concept about as far as I can in this direction. Perhaps if anyone cares to take this a step further, they can write a contributory blog post and provide a link in the comments. I'd love to see other approaches to this refactor.

5 comments:

  1. I have one problem with this: your BaseRate units vary depending on the type of employee - for a "BasicWithoutOvertime" person it's in $/hr; for a "Salary" person it's just in $ apparently. In other words, if you weren't being hand-wavy by just using "float" you wouldn't be able to give it a proper type. It's arguably just an arbitrary value used for different purposes by different calculations.

    Another thing to consider - you could make the functions public static readonly fields, which would probably be more compact than the properties.

    ReplyDelete
  2. Given the "there are two arbitrary values here" point, they're not really part of the employee... they're part of the salary computation. Perhaps use higher order functions: methods which *create* a `Func` based on whatever parameters are appropriate; so for Salary you'd just give one value, whereas for others you'd give two.

    Oh, and I meant to say before: *never* use float for monetary values. Decimal all the way, baby!

    ReplyDelete
  3. @JonSkeet yes, the arbitrary values aren't really part of the employee at all, I see your point. In the real world though, I think the calculation func should be part of the employee record because this is something that's individual to each employee i.e. the way my pay is calculated is entirely personal and doesn't necessarily relate to the way my colleague or indeed any of my coworkers are paid. So I think IRL terms, the calculation itself should be part of the employee object.

    However, I agree perhaps that the act of calculating their pay should be separated out into its own class.

    I'm intrigued as to how I'd generate a func from a higher order func. Can't wait to see your blog post on the subject.

    ReplyDelete
  4. Okay, my "fields" idea was silly (or at least not the best route), but I decided to blog about it instead of adding too many comments here:

    http://msmvps.com/blogs/jon_skeet/archive/2010/09/15/reply-to-a-reply-tweaking-refactoring.aspx

    ReplyDelete
  5. Hey Ben,

    Thanks for the well thought out response.

    I really like how you used the Func the way Cory did. I think this is possibly a better approach.

    However, I think putting it in the Employee class is a bit presumptuous since the employee was largely undefined in Jim's original code.

    And I don't know what the hell your contracting agreement is like, but I think I'll be speaking to you before engaging in my next consulting gig. I've never heard of a consultant getting time & a half, double time, & double time & a half ... are you unionized? LOL

    @JonSkeet - The reason it's a float is because Jims original code was a float. I didn't think it would be appropriate to change such a basic structure.

    Thanks again Ben. I'm glad the post inspired you.

    ReplyDelete

There was an error in this gadget