An exercise in Test Driven Development: Kata No. 9
An exercise in Test Driven Development: Kata No. 9
You Aren't Going to Need it - until you do
Last night, at the North West Ruby Group, we did an exercise in Kata No. 9.
This meant using Test-Driven Development to design a Checkout class that could deal with calculating the price for multiple products, including when certain discount rules were applied.
Our genial host, Rob, wanted us to do things by designing a simple test first, then doing the bare minimum to get that test to pass.
The first example was:
describe "#total" do
it "returns zero" do
checkout = Checkout.new
total = checkout.total
expect(total).to eq 0
end
end
And to make it pass:
class Checkout
def total = 0
end
Pretty trivial.
But that's kind of the point - too often we jump ahead and start building stuff that isn't necessary - complicating our code and wasting time and effort. So the idea is - we have a requirement, it's sat in front of us, and we need to deliver it now - so do the bare minimum to get that requirement fulfilled.
Also known as "YAGNI" - you aren't going to need it.
So we worked through the exercise, eventually ending up with a Checkout that looked something like this:
class Checkout
def initialize
@total = 0
end
def total
(@total > 115) ? 95 : @total
end
def scan product
if product == "A"
@total += 50
elsif product == "B"
@total += 30
else
@total + 20
end
end
end
A ridiculous way of dealing with the discount for product B ("2 for 45"), but the tests passed, and it was simple, if a bit ugly (I really don't like if statements). But as we moved on the next condition "when the checkout contains B and 3 As" (triggering a different discount rule for product A), Will and I got bored. We were the oldest people in the room (I think) and we've been doing this for a long time. The slow, incremental, steps were frustrating and we knew that hacking the total method was never going to be the long term solution.
When I'm doing TDD, I tend to draft a specification for what I'm about to create (that's why it's called RSpec, not RTest). I fill it with expectations, so when I run it I have multiple failing tests. Then I go through and make them pass.
The approach we were being asked to take was to add one test at a time, make that pass, then move on to the next - you should never have a failing test. As a learning exercise for someone unfamiliar with TDD, that's understandable. As a tool for ensuring that you stick with YAGNI, it makes perfect sense.
But, looking at the requirements we had in front of us at that moment in time, it definitely mentioned pricing rules. My immediate thought, when I read it was "there is a checkout and there are pricing rules - we will need objects representing both". At that point I did not know what they would look like - and it may be that one or both of those objects may be unnecessary, but I see TDD as an act of discovery. Write the specification (in full), then use that specification to discover a way to implement it.
So I broke the rules of the exercise and wrote a specification for the discount "the unit price for A is 50 and the discount is '3 for 130'"
It looked like this:
describe APriceCalculator do
subject(:calculator) { APriceCalculator.new }
it "returns 50 for 1" do
expect(calculator.price_for(1)).to eq 50
end
it "returns 100 for 2" do
expect(calculator.price_for(2)).to eq 100
end
it "returns 130 for 3" do
expect(calculator.price_for(3)).to eq 130
end
it "returns 180 for 4" do
expect(calculator.price_for(4)).to eq 180
end
it "goes up to 7"
end
That final clause "it goes up to 7" was because I knew that, for a discount grouped in threes, we should ensure we test up to a quantity of six, probably seven, just to be sure it was working. Again, thinking ahead, but I left the clause as pending, so we could come back to it.
Then we implemented it as simply as possible. Our maths was poor so it took a few attempts.
class APriceCalculator
def price_for quantity
return 130 if quantity == 3
number_of_bundles = quantity / 3
return quantity * 50 if number_of_bundles == 0
(number_of_bundles * bundle_price) + ((quantity % bundle_size) * 50)
end
end
Then the equivalent for product B's price of 30, with a discount of "2 for 45".
describe BPriceCalculator do
subject(:calculator) { BPriceCalculator.new }
it "returns 30 for 1" do
expect(calculator.price_for(1)).to eq 30
end
it "returns 45 for 2" do
expect(calculator.price_for(2)).to eq 45
end
it "returns 75 for 3" do
expect(calculator.price_for(3)).to eq 75
end
it "returns 90 for 4" do
expect(calculator.price_for(4)).to eq 90
end
end
And the BPriceCalculator looked like this:
class BPriceCalculator
def price_for quantity
return 45 if quantity == 2
number_of_bundles = quantity / 2
return quantity * 30 if number_of_bundles == 0
(number_of_bundles * 45) + ((quantity % bundle_size) * 30)
end
end
Now we had two specifications for price calculators that passed, plus a specification for the checkout that was failing.
And we also had two pricing calculator classes that looked almost identical. So I combined them into one class (our first proper refactoring) - I used a Struct so I didn't have to write a boiler-plate initialize method that simply copied parameters into instance variables - let's make Ruby do the work.
class BundleDiscountCalculator < Struct.new(:unit_price, :bundle_size, :bundle_price, keyword_init: true)
def price_for(quantity)
return unit_price if quantity == bundle_size
number_of_bundles = quantity / bundle_size
return quantity * unit_price if number_of_bundles == 0
(number_of_bundles * bundle_price) + ((quantity % bundle_size) * unit_price)
end
end
We updated the specs to use this new class:
describe "Price calculator for A" do
subject(:calculator) { BundleDiscountCalculator.new(unit_price: 50, bundle_size: 3, bundle_price: 130 }
...
end
describe "Price calculator for B" do
subject(:calculator) { BundleDiscountCalculator.new(unit_price: 30, bundle_size: 2, bundle_price: 45 }
...
end
Our calculator specs still passed (so the refactoring was a success) but our checkout spec still failed (as we hadn't touched that yet). So the next step was to get the checkout working.
At this point we realised that the price is based upon the quantity of each product that had been scanned - 2 B's would trigger the discount, 3 A's would too, but 2 A's would not. So the checkout needs to track how many of each product we had scanned. Again, according the kata, this is probably jumping ahead, but TDD gives you the space to experiment and that's what we were doing.
Our checkout became something like this:
class Checkout
def initialize pricing_rules
@pricing_rules = pricing_rules
@contents = Hash.new(0)
end
def total
total = 0
@contents.each do |product, quantity|
total += @pricing_rules[product].price_for(quantity)
end
total
end
def scan(product) = @contents[product] += 1
end
Instead of updating the total when the product is scanned, we store each the quantity of each product in a hash. Then, when we need the total, we use the pricing_rules (that were mentioned in the initial requirements document) to calculate the price.
To get the checkout spec to pass, we needed to pass in the pricing rules. But the next problem arose - what about products, like C and D, that didn't have any discounts? We needed to update our calculator to deal with just unit prices.
My first thought was to add a simple conditional to the calculator. If the bundle_size is nil then return unit_price * quantity. But Will realised that the maths still worked if you pass in a bundle_size of 1 and bundle_price matching the unit_price. I took this a step further and added an initializer to the calculator, to handle this automatically.
class BundleDiscountCalculator < Struct.new(:unit_price, :bundle_size, :bundle_price, keyword_init: true)
def initialize(**)
super
self.bundle_size ||= 1
self.bundle_price ||= unit_price
end
...
end
If you omit the bundle_size and bundle_price parameters when creating the calculator, it automatically configures itself for the case when there is no discount. We wrote a spec to prove this worked (writing the spec after the fact in this particular case), which passed:
describe "Price calculator with no discount" do
subject(:calculator) { BundleDiscountCalculator.new(unit_price: 22) }
it "returns the unit price for quantity 1" do
expect(calculator.price_for(1)).to eq 22
end
it "returns the unit price x 2 for quantity 2" do
expect(calculator.price_for(2)).to eq 44
end
it "returns the unit price x 3 for quantity 3" do
expect(calculator.price_for(3)).to eq 66
end
it "returns the unit price x 4 for quantity 4" do
expect(calculator.price_for(4)).to eq 88
end
end
Now we could go back to the checkout spec and pass in our pricing rules for each condition we were testing. Which looked like this:
describe Checkout do
let(:pricing_rules) do
{
"A" => BundleDiscountCalculator.new(unit_price: 50, bundle_size: 3, bundle_price: 130),
"B" => BundleDiscountCalculator.new(unit_price: 30, bundle_size: 2, bundle_price: 45),
"C" => BundleDiscountCalculator.new(unit_price: 20),
"D" => BundleDiscountCalculator.new(unit_price: 15)
}
end
...
end
And now everything passed.
SUCCESS! We had a working checkout that implemented the requirements.
If this were a real project, I would have committed at this point (using Tekin's advice to make the commit message long, explaining the thought process that lead us to this code).
However, there was one final thing to do.
The specs were passing but there was a lot of code in there that I thought was a bit ugly.
The price calculator had been renamed BundleDiscountCalculator even though it calculated the price without a discount.
The implementation of the BundleDiscountCalculator#price_for(quantity) method definitely looked like a candidate for simplification.
And I didn't like the word bundle - I just came up with it because I couldn't think of anything else at the time.
Finally the Checkout#total method used Collection#each plus an external variable to track the return value, which felt wrong and was probably unnecessary.
But now we had passing specs, we were safe to go back and tidy things up.
The final code ended up looking like this:
require "rspec"
class Checkout
def initialize pricing_rules
@pricing_rules = pricing_rules
@contents = Hash.new(0)
end
def total
@contents.sum do |product, quantity|
@pricing_rules[product].price_for(quantity)
end
end
def scan(product) = @contents[product] += 1
end
class ProductPriceCalculator < Struct.new(:unit_price, :discount_bundle_size, :discount_bundle_price, keyword_init: true)
def initialize(**)
super
self.discount_bundle_size ||= 1
self.discount_bundle_price ||= unit_price
end
def price_for(quantity) = ((quantity / discount_bundle_size) * discount_bundle_price) + ((quantity % discount_bundle_size) * unit_price)
end
describe "Price calculator with no discount" do
subject(:calculator) { ProductPriceCalculator.new(unit_price: 22) }
it "returns the unit price for quantity 1" do
expect(calculator.price_for(1)).to eq 22
end
it "returns the unit price x 2 for quantity 2" do
expect(calculator.price_for(2)).to eq 44
end
it "returns the unit price x 3 for quantity 3" do
expect(calculator.price_for(3)).to eq 66
end
it "returns the unit price x 4 for quantity 4" do
expect(calculator.price_for(4)).to eq 88
end
end
describe "Price calculator for A" do
subject(:calculator) { ProductPriceCalculator.new(unit_price: 50, discount_bundle_size: 3, discount_bundle_price: 130) }
it "returns 50 for 1" do
expect(calculator.price_for(1)).to eq 50
end
it "returns 100 for 2" do
expect(calculator.price_for(2)).to eq 100
end
it "returns 130 for 3" do
expect(calculator.price_for(3)).to eq 130
end
it "returns 180 for 4" do
expect(calculator.price_for(4)).to eq 180
end
it "returns 230 for 5" do
expect(calculator.price_for(5)).to eq 230
end
it "returns 260 for 6" do
expect(calculator.price_for(6)).to eq 260
end
it "returns 310 for 7" do
expect(calculator.price_for(7)).to eq 310
end
it "returns 360 for 8" do
expect(calculator.price_for(8)).to eq 360
end
end
describe "Price calculator for B" do
subject(:calculator) { ProductPriceCalculator.new(unit_price: 30, discount_bundle_size: 2, discount_bundle_price: 45) }
it "returns 30 for 1" do
expect(calculator.price_for(1)).to eq 30
end
it "returns 45 for 2" do
expect(calculator.price_for(2)).to eq 45
end
it "returns 75 for 3" do
expect(calculator.price_for(3)).to eq 75
end
it "returns 90 for 4" do
expect(calculator.price_for(4)).to eq 90
end
end
describe Checkout do
let(:pricing_rules) do
{
"A" => ProductPriceCalculator.new(unit_price: 50, discount_bundle_size: 3, discount_bundle_price: 130),
"B" => ProductPriceCalculator.new(unit_price: 30, discount_bundle_size: 2, discount_bundle_price: 45),
"C" => ProductPriceCalculator.new(unit_price: 20),
"D" => ProductPriceCalculator.new(unit_price: 15)
}
end
describe "#total" do
context "when the checkout is empty" do
it "returns zero" do
checkout = Checkout.new(pricing_rules)
total = checkout.total
expect(total).to eq 0
end
end
context "when the checkout contains A" do
it "returns 50" do
checkout = Checkout.new(pricing_rules)
checkout.scan "A"
expect(checkout.total).to be 50
end
end
context "when the checkout contains B and A" do
it "returns 80" do
checkout = Checkout.new(pricing_rules)
checkout.scan "A"
checkout.scan "B"
expect(checkout.total).to be 80
end
end
context "when the checkout contains B, A and B" do
it "returns 95" do
checkout = Checkout.new(pricing_rules)
checkout.scan "B"
checkout.scan "A"
checkout.scan "B"
expect(checkout.total).to be 95
end
end
context "when the checkout contains A, B and C" do
it "returns 100" do
checkout = Checkout.new(pricing_rules)
checkout.scan "A"
checkout.scan "B"
checkout.scan "C"
expect(checkout.total).to be 100
end
end
context "when the checkout contains A, B, C and D" do
it "returns 115" do
checkout = Checkout.new(pricing_rules)
checkout.scan "A"
checkout.scan "B"
checkout.scan "C"
checkout.scan "D"
expect(checkout.total).to be 115
end
end
context "when the checkout contains B and 3 As" do
it "returns 160" do
checkout = Checkout.new(pricing_rules)
checkout.scan "A"
checkout.scan "B"
checkout.scan "A"
checkout.scan "A"
expect(checkout.total).to be 160
end
end
end
end
The BundleDiscountCalculator has been renamed ProductPriceCalculator, explaining the reason for its existence much more clearly.
The ProductPriceCalculator#price_for(quantity) method has been squashed down into a single line (arguably a bit less readable, but it does the job in the most efficient manner). I would never have come up with that piece of maths on my own, but, again, I could experiment and use the spec to prove the new version worked.
Finally, Checkout#total method uses Collection#sum to calculate the return value, eliminating the extra variable.
The only issue was I could not think of a replacement for the word bundle - so I renamed the attributes discount_bundle_size and discount_bundle_price, making them more expressive.
Again, in a real project, I would commit this, with a message along the lines of "tidied up the code" and listing the changes, as above.
I'm pretty pleased with this final version. It handles the requirements (as proved by the specifications), the classes are small, the methods succinct, there aren't any if statements dirtying up the code and it uses ruby idioms like Collection#sum and endless methods[1].
I hope you found this exploration of TDD useful. Give me a shout[2] if you have any thoughts.
