22
FEB2012
Refactoring: The Gilded Rose
It's time for another refactoring challenge. This time we will attempt a fun problem called The Gilded Rose Code Kata.
That original description of the problem was for C# developers and it didn't have things us Rubyists love, like tests. Luckily for us though, Jim Weirich ported the code to Ruby and added the tests as he did so. Let's use that version of the challenge.
As always, I recommend you try the refactoring before you read what I have to say about it. Being familiar with the problem will help you understand what we are dealing with below.
Setup
The first step in this problem is to get the code running locally. I started by pulling down Jim's code from GitHub:
$ git clone https://github.com/jimweirich/gilded_rose_kata.git
Cloning into gilded_rose_kata...
remote: Counting objects: 114, done.
remote: Compressing objects: 100% (46/46), done.
remote: Total 114 (delta 71), reused 109 (delta 66)
Receiving objects: 100% (114/114), 15.38 KiB | 13 KiB/s, done.
Resolving deltas: 100% (71/71), done.
$ cd gilded_rose_kata
Then I needed to make sure the specs would run for me:
$ rake
rspec .
............................................*********....
Pending:
#update_quality with a single conjured item before the sell date
# No reason given
# ./gilded_rose_spec.rb:177
#update_quality with a single conjured item before the sell date
# No reason given
# ./gilded_rose_spec.rb:178
#update_quality with a single conjured…
# No reason given
# ./gilded_rose_spec.rb:182
#update_quality with a single conjured item on sell date
# No reason given
# ./gilded_rose_spec.rb:188
#update_quality with a single conjured item on sell date
# No reason given
# ./gilded_rose_spec.rb:189
#update_quality with a single conjured…
# No reason given
# ./gilded_rose_spec.rb:193
#update_quality with a single conjured item after sell date
# No reason given
# ./gilded_rose_spec.rb:199
#update_quality with a single conjured item after sell date
# No reason given
# ./gilded_rose_spec.rb:200
#update_quality with a single conjured…
# No reason given
# ./gilded_rose_spec.rb:204
Finished in 0.23681 seconds
57 examples, 0 failures, 9 pending
The passing specs represent the functionality the old code already handles. The pending specs are for the new feature that hasn't been added yet. It's great to have these specs to lean on. If I didn't, I would definitely feel the need to add some before I would be comfortable untangling that rat's nest of code.
Now, these specs worked for me on the first try, most likely because I've experimented with this code before. If they don't work for you right off the bat, it's probably because of missing dependencies. You need both RSpec and Jim's rspec-given enhancement. You should be able to get everything you need with:
$ gem install rspec
$ gem install rspec-given
Now we're ready to do some damage.
The Source of Truth
One of the most interesting aspects of this problem, in my opinion, is that you have two potential sources of truth. One one hand, you have the problem description. On the other, the code in need of refactoring.
A refactoring is meant to be a process where you slowly improve some code using a set of careful steps. If you follow a strict definition of that, we should begin with the code, clean it up, then add the requested new feature.
But you probably know by now that I love to break rules.
In my opinion, the problem description is a much better source of truth for this challenge. It pretty clearly states all of the requirements for this code. I can mostly just read down the list and code up those rules to end up at a solution. It also includes the new feature, so satisfying that will be automatic. If I run into questions along the way, I can try to answer them using the existing code.
So, just to be crystal clear, what I'm going to show below isn't strictly a "refactoring." That's kind of the point. You have to pick your battles and I felt like I could do a faster and better job by just coding up the rules. If you prefer to call this exercise coding to a specification, I'm fine with that.
Carving Out My Own Workspace
Since I want to keep the old code around for reference, I decided to make a copy of it and do my fixes in there:
$ cp gilded_rose.rb gilded_rose_clean.rb
I removed the body of update_quality() and the unneeded comment at the bottom of the file describing the hardcoded tests in the original code. That got me down to looking at just this:
def update_quality(items)
end
# DO NOT CHANGE THINGS BELOW -----------------------------------------
Item = Struct.new(:name, :sell_in, :quality)
I needed to point the specs at this new code and I also wanted to disable most of them for getting started. Those two changes looked like this:
require 'rspec/given'
require 'gilded_rose_clean' # pointed at my code
describe "#update_quality" do
context "with a single" do
Given(:initial_sell_in) { 5 }
Given(:initial_quality) { 10 }
Given(:item) { Item.new(name, initial_sell_in, initial_quality) }
When { update_quality([item]) }
context "normal item" do
Given(:name) { "NORMAL ITEM" }
context "before sell date" do
Then { item.quality.should == initial_quality-1 }
Then { item.sell_in.should == initial_sell_in-1 }
end
context "on sell date" do
Given(:initial_sell_in) { 0 }
Then { item.quality.should == initial_quality-2 }
Then { item.sell_in.should == initial_sell_in-1 }
end
context "after sell date" do
Given(:initial_sell_in) { -10 }
Then { item.quality.should == initial_quality-2 }
Then { item.sell_in.should == initial_sell_in-1 }
end
context "of zero quality" do
Given(:initial_quality) { 0 }
Then { item.quality.should == 0 }
end
end
# The remaining "single item" contexts commented out, for now...
end
# The "several objects" contexts commented out, for now...
end
Finally, I just verified that I had some failing specs to start with:
$ rake
rspec .
FFFFFF.
Failures:
1) #update_quality with a single normal item before sell date
Failure/Error: Then { item.quality.should == initial_quality-1 }
expected: 9
got: 10 (using ==)
# ./gilded_rose_spec.rb:17:in `block (5 levels) in <top (required)>'
2) #update_quality with a single normal item before sell date
Failure/Error: Then { item.sell_in.should == initial_sell_in-1 }
expected: 4
got: 5 (using ==)
# ./gilded_rose_spec.rb:18:in `block (5 levels) in <top (required)>'
3) #update_quality with a single normal item on sell date
Failure/Error: Then { item.quality.should == initial_quality-2 }
expected: 8
got: 10 (using ==)
# ./gilded_rose_spec.rb:23:in `block (5 levels) in <top (required)>'
4) #update_quality with a single normal item on sell date
Failure/Error: Then { item.sell_in.should == initial_sell_in-1 }
expected: -1
got: 0 (using ==)
# ./gilded_rose_spec.rb:24:in `block (5 levels) in <top (required)>'
5) #update_quality with a single normal item after sell date
Failure/Error: Then { item.quality.should == initial_quality-2 }
expected: 8
got: 10 (using ==)
# ./gilded_rose_spec.rb:29:in `block (5 levels) in <top (required)>'
6) #update_quality with a single normal item after sell date
Failure/Error: Then { item.sell_in.should == initial_sell_in-1 }
expected: -11
got: -10 (using ==)
# ./gilded_rose_spec.rb:30:in `block (5 levels) in <top (required)>'
Finished in 0.00348 seconds
7 examples, 6 failures
Failed examples:
rspec ./gilded_rose_spec.rb:17…
rspec ./gilded_rose_spec.rb:18…
rspec ./gilded_rose_spec.rb:23…
rspec ./gilded_rose_spec.rb:24…
rspec ./gilded_rose_spec.rb:29…
rspec ./gilded_rose_spec.rb:30…
rake aborted!
Command failed with status (1): [rspec ....]
Tasks: TOP => default => spec
(See full trace by running task with --trace)
Perfect. I'm ready to work.
My Fellow Cheater
You know I love to cheat. Well, there's a reason I love Ruby: she cheats too.
My first instinct for building the kind of code we need here is to build an object hierarchy. Item would be at the top of that tree and I could make subclasses like LegendaryItem to handle the specific cases. Sadly, this is blocked by the rules:
However, do not alter the Item class or Items property as those belong to the goblin in the corner who will insta-rage and one-shot you as he doesn't believe in shared code ownership (you can make the UpdateQuality method and Items property static if you like, we'll cover for you).
If you overlook the C#-isms and World of Warcraft jokes in there, the heart of it is that it blocks us from fleshing out the Item class. While it doesn't specifically address subclasses, the code sort of blocks it:
# DO NOT CHANGE THINGS BELOW -----------------------------------------
Item = Struct.new(:name, :sell_in, :quality)
It looks like I can't define any subclasses below the definition of Item. I could get around that using an evil Ruby trick:
END {
class MyItem < Item; end
p MyItem.new
}
# DO NOT CHANGE THINGS BELOW -----------------------------------------
Item = Struct.new(:name, :sell_in, :quality)
END { … } allows me to run some Ruby code just before the interpreter is exiting. Item will have been defined by then, so I can safely subclass it and do some work. This gets me around the restriction, but it uglies up the code and that's counter to my goals.
Also, it's not clear if the rules allow me to move update_quality() into an END { … } block and I would need to do that to make this work. Finally, the END { … } blocks could interfere with running the specs, depending on how the ordering is handled.
There's a lot of reasons not to do this, so it's clear I need another idea.
I could still build the hierarchy, if those objects just wrapped the Item. That code would look something like:
class ItemUpdater
def initialize(item)
@item = item
end
attr_reader :item
def update
# ...
end
end
class LegendaryItemUpdater < ItemUpdater
def update
# ...
end
end
This approach would work fine. The update_quality() method would just need to apply the correct wrapper for the Item, then trigger that specialized version of update().
This may be the best way to go overall, but it still bugs me a little that an Item can't just update itself. I really want to rework the class hierarchy dynamically at runtime. Well, it turns out Ruby can do that.
First, we can just define some updating methods in a Module:
module Aging
def update
update_quality
update_sell_in
end
def update_sell_in
self.sell_in -= 1
end
def update_quality
self.quality -= sell_in > 0 ? 1 : 2
self.quality = 0 if quality < 0
end
end
These handle the Aging rules for normal items.
Now, we can mix these methods into an Item as we process it, then let it update() itself:
def update_quality(items)
items.each do |item|
item.extend(Aging) unless item.is_a? Aging
item.update
end
end
That's all it takes to get those first set of specs passing:
$ rake
rspec .
.......
Finished in 0.00316 seconds
7 examples, 0 failures
The only question that remains is, "Is this legal under the rules?" I think it's gray area. I am modifying the Item dynamically at runtime. The object will have some new methods after it is passed through update_quality(). The rules don't seem to block that though as I read them. I haven't modified the Item class definition or the code below the do-not-touch line.
More importantly, this feels right and Rubyish to me. If I was forced to correct some hostile code I couldn't change, I might try something just like this. Plus it makes sense. That object is both an Item and it is Aging. That's exactly how the code handles it.
It you weren't allowed to do even this, I would go with the wrapped approach. For now, I'm going forward with my cheat.
Overriding Mix-ins
We've handled the normal items, but now we need variations on that basic theme. I didn't go with classes, but we can still override the methods. In Ruby, the last Module mixed-in wins, when it comes to method lookup. That method can delegate up the chain normally though, hitting methods mixed-in from a Module before it.
Knowing that, I created a Module for each of the cases I still needed to handle:
module BetterWithAge; end
module Popular; end
module Legendary; end
module Conjured; end
Then I tweaked update_quality() one last time to account for these subtypes:
def update_quality(items)
items.each do |item|
item.extend(Aging) unless item.is_a? Aging
subtype = case item.name
when /Aged/i then BetterWithAge
when /Backstage pass/i then Popular
when /Sulfuras/i then Legendary
when /Conjured/i then Conjured
end
item.extend(subtype) if subtype and not item.is_a? subtype
item.update
end
end
Again, since those subtypes are always added after Aging, they can override the Aging methods as needed.
It's worth noting that I always check for the Module before mixing it in. The system sounds like it could be pretty long running and an Item should be updated each day. If the same references are kept around, we only need to add in the Module the first time we see that Item.
I tried to write the regular expressions here as generic as possible to support other names that might be in a similar category. This could be good or bad, depending on how things play out. It may be necessary to build more of an exact mapping in the future, but since we would only need to modify this one small chunk to do that I'm comfortable taking the risk.
Wait Until You're Sure
The rest is a simple matter of programming, as they say.
One by one, I uncommented groups of specs and fleshed out the related Module to make them pass. There weren't too many surprises there, but I'll point out a couple of things as we go.
First, I handled the cheese:
module BetterWithAge
def update_quality
self.quality += sell_in > 0 ? 1 : 2
self.quality = 50 if quality > 50
end
end
It did occur to me that there's some minor duplication in this code. I could perhaps make it possible to change the sign of the amount that gets applied to quality(). This is only the second time I've seen this pattern though. I cringe a little, but choose to let it ride for now.
On to the backstage passes:
module Popular
def update_quality
self.quality += case sell_in
when -Float::INFINITY..0 then -quality
when 1..5 then 3
when 6..10 then 2
else 1
end
self.quality = 50 if quality > 50
end
end
There are two tricks here. First, I built an infinite Range to catch all of the negative numbers. I love it that Ruby can handle this because it just checks if values are between the bounds.
The other trick is that I adjust the quality() by the opposite of its current value when I want to zero it out. Together, these two tricks allowed me to get the rules into one clean case statement and avoid a lot of special rules.
That code got the tests passing, but it introduced another bit of duplication. I find myself adjusting quality() again by yet another amount. I also had to duplicate the upper bound check here. This is the third time now, so it's time to admit I have a problem and fix it.
This is an actual refactoring, so let's handle it step by step. My tests are green. It's safe to take action.
First, let's add the upper bound check to Aging:
module Aging
# ...
def update_quality
self.quality -= sell_in > 0 ? 1 : 2
self.quality = 0 if quality < 0
self.quality = 50 if quality > 50
end
end
I rechecked the tests and that's a fine change.
Now, I would like to remove the upper bound check from the other two methods, but that won't work. They don't hand up to Aging's update_quality(), so the checks wouldn't be run. Unfortunately, they can't make that handoff, because they would then modify quality() twice. That gives me a hint about what I should do.
The only difference between the three methods at this point is that they adjust quality() by different amounts. Extract that amount and we're all set for the later Module's to override that instead. That led me to this code:
module Aging
# ...
def update_quality
self.quality += quality_adjustment
self.quality = 0 if quality < 0
self.quality = 50 if quality > 50
end
def quality_adjustment
sell_in > 0 ? -1 : -2
end
end
The tests are still passing, so I haven't broke anything yet.
Now I can revise the cheese updating code:
module BetterWithAge
def quality_adjustment
-super
end
end
That was easier, the tests still pass, and I think it reads better. It's the same process, it just has the opposite effect. The code now reflects that.
Let's also fix the backstage passes:
module Popular
def quality_adjustment
case sell_in
when -Float::INFINITY..0 then -quality
when 1..5 then 3
when 6..10 then 2
else 1
end
end
end
Again, the tests are passing so this works.
I hope you saw how it was easier to DRY (Don't Repeat Yourself) this code up once I had that third data point. It showed me that the bounds checks were also shared. I didn't know that with just the first two. Sometimes, being over anxious to fix code cheats you out of having better data.
Besides, the first two instances weren't a gross violation of the rule. If that code became production code, we weren't going to be seriously hurting. It's more important to apply the right amount of paranoia than it is to unfailingly follow some system.
The Last Two Subtypes
The other two item types are almost trivial. For a Legendary Item, we just need to shut the changes off:
module Legendary
def update
# do nothing: doesn't need to be sold and doesn't degrade
end
end
That gets another set of specs passing. At this point we've replaced the functionality of the old code and we didn't even need to wade into that mess. Hooray!
This is one of the few cases where I do like to use a comment. If some code does nothing, it's nice to know why. Otherwise, I might think I could safely remove the method. In this case, that would break things.
It's worth noting that I pretty much ignored the quality() should always be 80 for these items rule. Since I never adjust the quality() of these items, it won't be my fault if that's not the case. I decided that meant it didn't apply to me. If you are concerned about it though, I would just override the getter here to return the fixed value.
A Conjured Item is just about as easy:
module Conjured
def quality_adjustment
super * 2
end
end
I feel like this just shows again how helpful getting that earlier refactoring right was. It's still paying dividends.
That added the desired new feature, and removing the pending() call shows that all of the tests now pass.
Further Reading
Here are some links for those who want to dig further into these topics:
- Jim provided his own solution to the refactoring in a branch of his repository. His code is sort of a cross between my own and the wrapper approach. It's worth a look.
- If you want to dig more into the true refactoring process, like the steps I took in the middle article, the Refactoring book can't be beat. Even though it's not Ruby, it applies to all of programming.
- If you want more practice programming to a specification, I have a favorite problem that's a lot of fun to play with. Give it a shot.
Leave a Comment (using GitHub Flavored Markdown)