Discount rules, part 1
Time for another example.
One of my training workshops involves finding and fixing the coupling in an application I wrote that simulates a shop (products, SKUs, a warehouse, a catalogue, baskets and checkout), operated via a command-line interface. We’ll take a look at the Python version of the application code.
Here’s one function:
class Basket:
def _currentTotal(self):
total = 0
for item in self.items.values():
total += item.count * item.price
if total > 2000:
total -= (total/10)
return total
What do you think of that 2000 there? And the 10?
The programmer’s intention is to give a 10% discount on orders whose total price is over 2000, and this code could definitely benefit from some more helpful naming around that part of the algorithm. But do that 2000 and the 10 represent code smells? Or connascence? Or coupling? Take a moment to think about those questions before you read on.
Well, fans of code smells may indeed say that the 2000 and the 10 are Magic Numbers — numbers plonked into the code without helpful names to tell us what they are intended for. But as for anything else, we’d need to look at the wider application code to make any further statement.
So we look around and find this:
class DisplayBasketCommand:
def run(self, cmd):
items = self.basket.list()
for item in items:
print item.price + " " + item.title
total = self._basketTotal(items)
if total > 2000:
discount = total/10
print "$%8.02f 10%% discount" % (-discount)
total -= discount
print "---------------"
print "$%8.02f total\n" % (total)
def _basketTotal(self, items):
total = 0
for item in items:
total += item.price * item.count
return total
There’s that 2000 and that 10 again! Now we have coupling, and indeed connascence.
We might say that this is two examples of Connascence of Value (although I could also make a good case for Connascence of Algorithm; does it matter which it is?). The thing is, each of these functions knows that the discount threshold is 2000 and the discount amount is 10% — and neither of these facts can be read directly from the code of the function itself.
This is a problem because either value could be changed, and then the application would break. Even if there are tests requiring each function to apply a 10% discount when the total is over 2000, it’s extremely unlikely that there is a single test checking that both the command and the basket apply the same discount in the same way.
Look again at the Basket._currentTotal
function. It is also coupled to the set of behaviours offered by items. But that coupling is evident in the code of _currentTotal
. I want to say that _currentTotal
is explicitly coupled to the item class, whereas it is implicitly coupled to DisplayBasketCommand
: they share knowledge of discount rules and that coupling cannot be read directly from the code of _currentTotal
without the programmer’s perspective of the whole codebase.
I want to be really clear here: the _currentTotal
function itself does not have enough information to know that it is coupled to DisplayBasketCommand
. And vice versa. Only some being that can step outside of this function and see more of the codebase can know that this coupling exists. So then maintaining it becomes a matter of memory, or documentation, or folklore. Any of which is adding risk.
But if we can somehow change the function so that it “knows” that the 2000 and the 10 are coupled, then we can rely on that function to actively help us in maintaining its correctness.
Let’s assume this is the only problem in this application. What would you do next? Take a couple of minutes to consider what you might do to “fix” this coupling.
Now, we look around the application again and discover this:
class Catalogue:
#...
def list(self, out):
for sku in self._sortedSkus():
out.write("%s\t%dp\t%s\n" % (sku.code[2], sku.price, sku.title))
out.write("\n10% discount on orders over $20.00!\n")
Now the coupling / connascence has a higher “degree”, and I think we will probably all agree that this makes it worse.
Does this change how you would refactor it?
In the next article I’ll look at what our options are for refactoring here, all the while remembering that my focus is on making this code more habitable by making the coupling explicit. Stay tuned…