Gilded Rose 4
It’s Monday, a bright shiny day so far. Let’s look at Gilded Rose and see what we might do to improve things. We’ve implemented the requested new feature, but we need to leave the campground a bit better than we found it.
Adding tests, and refactoring …
Which is our first topic. Responses to these articles, and other Twitter chats recently, have raised an issue that I’ll paraphrase like this:
Our management won’t give us the time to test, much less refactor, they just demand feature after feature. How can we get permission to do the right thing?
My answer is the same as it has been for years: Don’t ask permission to do the right thing. Just do the right thing. Management probably doesn’t know what you’re doing anyway.
Why do we test and refactor, class? Because it produces working code faster than if we don’t. Now in my articles, including this one, I refactor the hell out of things. What will happen to Gilded Rose here will surely be too much refactoring for a single feature. But there will be more features for G.R., and each time we do one, we can move the code more toward what it needs to be.
We do this because if we don’t test, the code won’t work. We’ve already seen that I managed to write two lines of code one of which didn’t work. You’re smarter than I am: you can probably right ten lines one of which doesn’t work. Or even twenty. If we don’t test, things don’t work. That slows us down and irritates management and customers as well.
And we write tests, rather than just run them manually, because when we change things, particularly in overly complex code like we have here, we make more mistakes. So automated tests help us avoid those regressions.
And because the complex code slows us down, we can go faster by improving it incrementally. It’s tempting to just rewrite, but in my experience that trick rarely works. It takes rather a long time with no payoff that management can see, if almost always introduces new defects. With larger rewrites, there’s often a requirement to keep adding features to the old version, which puts us in a tail chase that rarely ends well.
Rather than rewriting, improving the code reduces mistakes and we get working code done sooner.
And I’m quite serious about not asking permission to do your job. You were hired to do your job, you are paid to do your job, you know better than they do how to do your job. Just do it. Since doing it well allows you to produce working code faster, it’s good for everyone.
Subclassing
Daganev, “all around good guy”, suggested on Twitter that we could use inheritance as part of our refactoring, and said that Sandy Metz had done that in a talk that she gave. Certainly this is an interesting idea, and it would readily allow us to create suitable subclasses for each type of Item, and to give them just the behavior they need.
The rules are a bit unclear here: they say that we cannot change the Item class or the items
collection. I interpret that to mean that we aren’t allowed to put anything in the items
collection other than actual instances of Item, and that we aren’t allowed to subclass Item class. I can understand that the goblin might not want us subclassing since we might override and break his precious code. I’m not sure why we can’t change the items
member variable, other than to make our lives harder.
I think it’s interesting to see what we can do under the strict conditions, so I plan to proceed that way. No subclassing or duck typing on Item, and only instances of Item in the items
collection.
Moving right along …
Let’s review the code and see what we can do quickly before we ship this feature.
def match_name(item,name)
return item.name.downcase.include? name.downcase
end
def update_conjured_item(item)
item.sell_in = item.sell_in - 1
normal_degradation = 1
if item.sell_in < 0
normal_degradation = 2*normal_degradation
end
conjured_degradation = 2*normal_degradation
item.quality = [0,item.quality - conjured_degradation].max
end
def update_item(item)
if (match_name(item,"conjured"))
update_conjured_item(item)
return
end
if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1 #1 for sword
end
end
else
if item.quality < 50
item.quality = item.quality + 1
if item.name == "Backstage passes to a TAFKAL80ETC concert"
if item.sell_in < 11
if item.quality < 50
item.quality = item.quality + 1
end
end
if item.sell_in < 6
if item.quality < 50
item.quality = item.quality + 1
end
end
end
end
end
if item.name != "Sulfuras, Hand of Ragnaros"
item.sell_in = item.sell_in - 1
end
if item.sell_in < 0
if item.name != "Aged Brie"
if item.name != "Backstage passes to a TAFKAL80ETC concert"
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1
end
end
else # if item.name == "Backstage passes ..."
item.quality = item.quality - item.quality # approx zero
end
else # if item.name == "Aged Brie"
if item.quality < 50
item.quality = item.quality + 1
end
end
end
end
def update_quality()
@items.each do |item|
update_item(item)
end #do
end #update_quality
end # Gilded_Rose
As I look at update_conjured
, all written out longhand like that, I like that it tells the story but I don’t like that it mixes computing the right degradation value in with the updating. Let’s extract the degradation computation:
def conjured_degradation(item)
normal_degradation = 1
if item.sell_in < 0
normal_degradation = 2*normal_degradation
end
conjured_degradation = 2*normal_degradation
end
def update_conjured_item(item)
item.sell_in = item.sell_in - 1
item.quality = [0,item.quality - conjured_degradation(item)].max
end
That provides better separation of concerns, updating versus getting the values to be used in updating. I’m not entirely in love with the names in conjured_degradation
but they’ll do for now. Tests run, so that’s good.
I’d like to clean up the main update method just a bit. What looks like an easy improvement there? How about this patch of code:
if item.name != "Aged Brie"
if item.name != "Backstage passes to a TAFKAL80ETC concert"
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1
end
end
else # if item.name == "Backstage passes ..."
item.quality = item.quality - item.quality # approx zero
end
else # if item.name == "Aged Brie"
if item.quality < 50
item.quality = item.quality + 1
end
end
I’ve already commented this because I hate that !=
trick. Let’s invert the sense of that if. I think it’ll make things a bit more clear:
if item.name == "Aged Brie"
if item.quality < 50
item.quality = item.quality + 1
end
else
if item.name != "Backstage passes to a TAFKAL80ETC concert"
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1
end
end
else # if item.name == "Backstage passes ..."
item.quality = item.quality - item.quality # approx zero
end
end
That’s a bit better and the micro tests and text test both run. Running the text test makes me want to create some more micro tests, and maybe we’ll do that today or maybe not.
Let’s invert that inner if as well:
if item.name == "Aged Brie"
if item.quality < 50
item.quality = item.quality + 1
end
else
if item.name == "Backstage passes to a TAFKAL80ETC concert"
item.quality = item.quality - item.quality # approx zero
else
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1
end
end
end
end
Again the tests run. What else? Well, couldn’t we reduce the nesting a bit here if we used elsif
for that if on the passes? Let’s see.
if item.name == "Aged Brie"
if item.quality < 50
item.quality = item.quality + 1
end
elsif item.name == "Backstage passes to a TAFKAL80ETC concert"
item.quality = item.quality - item.quality # approx zero
else
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1
end
end
end
Hm. That’s starting to be almost readable. Let’s fix that silly quality - quality thing:
if item.name == "Aged Brie"
if item.quality < 50
item.quality = item.quality + 1
end
elsif item.name == "Backstage passes to a TAFKAL80ETC concert"
item.quality = 0
else
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1
end
end
end
Tests are good. I’ve been having such a good time I didn’t commit the code. Really should have, because some of these changes might not have worked. I’ll do it now. Here’s the whole file again for your review:
class GildedRose
def initialize(items)
@items = items
end
def match_name(item,name)
return item.name.downcase.include? name.downcase
end
def conjured_degradation(item)
normal_degradation = 1
if item.sell_in < 0
normal_degradation = 2*normal_degradation
end
conjured_degradation = 2*normal_degradation
end
def update_conjured_item(item)
item.sell_in = item.sell_in - 1
item.quality = [0,item.quality - conjured_degradation(item)].max
end
def update_item(item)
if (match_name(item,"conjured"))
update_conjured_item(item)
return
end
if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1 #1 for sword
end
end
else
if item.quality < 50
item.quality = item.quality + 1
if item.name == "Backstage passes to a TAFKAL80ETC concert"
if item.sell_in < 11
if item.quality < 50
item.quality = item.quality + 1
end
end
if item.sell_in < 6
if item.quality < 50
item.quality = item.quality + 1
end
end
end
end
end
if item.name != "Sulfuras, Hand of Ragnaros"
item.sell_in = item.sell_in - 1
end
if item.sell_in < 0
if item.name == "Aged Brie"
if item.quality < 50
item.quality = item.quality + 1
end
elsif item.name == "Backstage passes to a TAFKAL80ETC concert"
item.quality = 0
else
if item.quality > 0
if item.name != "Sulfuras, Hand of Ragnaros"
item.quality = item.quality - 1
end
end
end
end
end
def update_quality()
@items.each do |item|
update_item(item)
end #do
end #update_quality
end # Gilded_Rose
class Item
attr_accessor :name, :sell_in, :quality
def initialize(name, sell_in, quality)
@name = name
@sell_in = sell_in
@quality = quality
end
def to_s()
"#{@name}, #{@sell_in}, #{@quality}"
end
end
This is somewhat improved, and it hasn’t taken much time at all. Easily done as part of putting in our new feature. Let’s call it a day, with a new feature and some improved code.
Now as a rule, if in fact we ship the code with our new feature for conjured items, I would not recommend improving this code, and probably not improving its tests, until we pass this way again.
And we will pass this way again, as surely there will be new products.
Plus, we have something to bring up to Allison, our friendly innkeeper / boss. We’ve noticed that the code as written will not support new products very well at all. New backstage passes to other than the TAFKAL80ETC concert will not work. Different versions of Sulfuras other than the Hand of Ragnaros will not work. If Aged Brie were renamed “Aged Brie from the Brie Region of Brieland”, that wouldn’t work.
Once we tell Allison this, she’ll surely send us back to fix those things. She’s pretty reasonable, so we can probably tell her all of those items at once, but we might want to put each one on a separate card and treat them as separate stories.
One way or another we’ll be back, and we can improve the code more every time we pass through.
Which reminds me of this article: Refactoring - Not on the backlog , which describes the multi-pass approach to code improvement with some diagrams. Check it out.
See you next time!