Reply to a reply... tweaking refactoring
This is a reply to Ben Alabaster's blog post, which is itself a reply. You can follow the trail yourself. I'll assume you've read the post - I'm not going to go over anything already written there, other than my comments.
I took issue with three aspects of Ben's refactoring:
- The use of float for currency
- The fact that "BaseRate" effectively doesn't have a well-defined unit; in some cases it's "dollars per hour" and in others it's "dollars per pay cheque, regardless of hours" smells
- The use of properties for the pay functions
I'll tackle the last one first, because I was stupid. I suggested using public static readonly fields instead of properties. This was dumb. All we need is a simple static class with public static methods - we can use them with exactly the same source code as before for the Employee construction, but without all the fancy lambda expressions:
public static class PayCalculations
{
public static float BasicWithoutOvertime(float hours, float baseRate)
{
return hours * baseRate;
}
public static float BasicWithOvertime(float hours, float baseRate)
{
if (hours < 40) return hours * baseRate;
return ((hours - 40f) * 1.5f + 40f) * baseRate;
}
public static float Salary(float hours, float baseRate)
{
return baseRate;
}
public static float Contractor(float hours, float baseRate)
{
float subtotal = Math.Min(hours, 40) * baseRate;
hours -= Math.Min(hours, 40);
if (hours > 0) subtotal += 1.5f * Math.Min(hours, 20) * baseRate;
hours -= Math.Min(hours, 20);
if (hours > 0) subtotal += 2.0f * Math.Min(hours, 20) * baseRate;
hours -= Math.Min(hours, 20);
if (hours > 0) subtotal += 2.5f * hours * baseRate;
return subtotal;
}
}
Less code, less nesting, less use of fancy C# 3 features... generally nicer. The construction code remains the same, because it uses method group conversions to build the delegates.
Fixing the "float should be decimal" problem is easy, of course. Let's move on to the units and "wrong" values. The problem is that the BaseRate property means different things for different employees, and in some cases it's not even needed at all. That's a reasonably strong indicator that it's in the wrong place. Let's accept that all employees' pay may depend on the number of hours they've worked that week, but that's all. Everything else depends on the particular contract that the employee is using, and that can vary. So let's put the variance into what creates the function - so we can build a "salaried employee on $2935 week" function, a "per hour worker on $40.25 basic without overtime" etc. This is effectively like creating an IPayContract interface and multiple implementations, then creating instances of those implementations which have specific values. Except we're using delegates... so having ripped out the lambda expressions, I'm going to put them back in :) But this time we're just going to use a Func<decimal, decimal> as we only to know how much to pay given a certain number of hours worked. (The first decimal here could potentially be float or double instead, but if anyone ever did claim to work 3.1 hours, they'd probably want pay that reflected it.)
Here are the pay calculations:
public static class PayCalculations
{
public static Func<decimal, decimal> BasicWithoutOvertime(decimal dollarsPerHour)
{
return hours => dollarsPerHour * hours;
}
public static Func<decimal, decimal> BasicWithOvertime(decimal dollarsPerHour)
{
return hours => {
decimal basicHours = Math.Min(hours, 40);
decimal overtimeHours = Math.Max(hours - 40, 0);
return (basicHours * dollarsPerHour) + (overtimeHours * dollarsPerHour * 1.5m);
};
}
public static Func<decimal, decimal> Salary(decimal dollarsPerWeek)
{
return hours => dollarsPerWeek;
}
public static Func<decimal, decimal> Contractor(decimal baseRate)
{
return hours => {
decimal basicHours = Math.Min(hours, 40);
decimal overtime = Math.Min(Math.Max(hours - 40, 0), 20);
decimal doubleTime = Math.Min(Math.Max(hours - 60, 0), 20);
decimal chargingThroughTheNoseTime = Math.Max(hours - 80, 0);
return (basicHours * baseRate)
+ (overtime * baseRate * 1.5m)
+ (doubleTime * baseRate * 2m)
+ (chargingThroughTheNoseTime * baseRate * 2.5m);
};
}
}
And now, when we construct the employees, we don't have to specify a base rate which was only meaningful in some cases - instead, we give that value to the pay calculator instead:
List<Employee> employees = new List<Employee>
{
new Employee("John", "MacIntyre", PayCalculations.BasicWithoutOvertime(40.25m)),
new Employee("Ben", "Alabaster", PayCalculations.BasicWithOvertime(40.25m)),
new Employee("Cory", "Fowler", PayCalculations.Salary(2935m)),
new Employee("John", "Doe", PayCalculations.Contractor(150m)),
new Employee("Jane", "Doe", hours => 3500m),
new Employee("Joe", "Bloggs", hours => 34.25m * Math.Max(hours, 15))
};
Now, look at the Salary method and the comment in it... I'm still concerned about the units. We're meant to be returning a simple dollar value (and in another system I'd probably bake that information into the types used) but we've still got dollarsPerWeek. What's wrong here? Well, it all boils down to an assumption: we're running this once a week. We've got a constant of 1 week... so we could rewrite the method like this:
public static Func<decimal, decimal> Salary(decimal dollarsPerWeek)
{
decimal weeksWorked = 1m;
return hours => dollarsPerWeek * weeksWorked;
}
Now the units work... although it looks a bit silly. Of course, it makes our assumption very explicit - and easy to challenge. Maybe we actually run payroll once per month... in which case the fact that we're expressing the salary in dollars per week is wrong - but very obviously wrong, which is a good thing.
Conclusion
It doesn't feel right to have a blog post with no conclusion. Lessons applied here:
- Remember all the tools available, not just the shiny ones. Using method group conversions made the initial "constant function" approach simpler to understand.
- Units are important! If the same field effectively represents different units for different instances, there's something wrong
- If a field is only relevant for some instances of a type, it's probably in the wrong place
- Don't use floats for currency. See any number of Stack Overflow questions for reasons why :)
EDIT: As noted by Barry Dorrans, there's a lot of scope for introducing constants in here, for further goodness. I'm not going to bother updating the code just for Barry though. That way madness lies.