Gilded Rose 2
It’s April 17th. It’s snowing. We’re on lockdown. Let’s look at Gilded Rose and see what we might do that would be interesting.
We have a simple characterization test in place. I don’t trust it much, as it only covers a couple of days, but it’s a start. We have a new requirement, support for Conjured goods that lose quality twice as fast as normal. We have some really horrid code that we could improve. Perfect.
I’m inclined to address the new requirement, and I think we can do that pretty safely. If we can do it in a reasonable way, it should give us some ideas for how the rest of the code might be improved.
My rough plan is to add handling of Conjured items at the top of our update_item method. I’ll do all the updating needed, of both sell-in and quality, and return out of the method when finished, ensuring that we don’t use any of the old code.
And of course, I’m going to do the job with microtests, TDD style. Here’s the method before we begin:
def update_item(item)
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
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
So ugly. Sublime has a nice feature that lets you “fold” complex statements, but when it copies the text it (rightly) includes the folded bits, so I can’t really show you what it looks like. Roughly like this:
def update_item(item)
if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert" ...
else
if item.quality < 50 ...
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" ...
else # if item.name == "Aged Brie"
if item.quality < 50
item.quality = item.quality + 1
end
end
end
end
Maybe that’s better, but I have my doubts. Anyway, I need a test. The spec is: “Conjured” items degrade in Quality twice as fast as normal items. Should be easy enough to test.
def test_conjured
gr = GildedRose.new([])
item = Item.new("Conjured thing", sell_in=3, quality=10)
gr.update_item(item)
assert_equal(2, item.sell_in)
assert_equal(8, item.quality)
end
This fails. Full disclosure, though: I wrote it first with just the assert on sell_in, expecting it to fail because I hadn’t written my code yet, but it passed, because (of course) the default behavior does decrement the sell_in correctly. Anyway, after adding the check on quality going down by two, it does fail:
1) Failure:
TestGildedRose#test_conjured [/Users/ron/Dropbox/Ruby/gilded-rose-1/test-gilded-rose.rb:21]:
Expected: 8
Actual: 9
Now to make it work.
I don’t recall whether I mentioned noticing that the tests for items in the original code are all based on literal values of the whole item, not on key strings like “backstage pass” or the like. That strikes me as a bug, and we should deal with it. I’d ask the customer, Allison, if they were available, but in their absence, we’ll have to decide whether to change the system or not. For now, we could write a test to remind us of the problem. I’ll try to remember that. It’s not a good idea to divert right in the middle of something, so let’s get our conjured code working first.
How about this:
def match_name(item,name)
return item.name.downcase.include? name.downcase
end
def update_conjured_item(item)
item.sell_in = [0,item.sell_in-1].max
item.quality = item.quality - 2
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"
...
My test runs. I actually built that bit by bit, first the match, then the update of sell_in, then the update of quality. And there’s a bug, even though our test runs. Do you see it? Of course you do … quality can never go below zero. Let’s test that and then fix it.
def test_conjured_quality_non_negative
gr = GildedRose.new([])
item = Item.new("Conjured thing", sell_in=2, quality=2)
gr.update_item(item)
assert_equal(1, item.sell_in)
assert_equal(0, item.quality)
gr.update_item(item)
assert_equal(0, item.sell_in)
assert_equal(0, item.quality)
end
Here we create a short-lived not very high quality conjured thing, update it once and check, update again and check. This test fails, as expected:
1) Failure:
TestGildedRose#test_conjured_quality_non_negative [/Users/ron/Dropbox/Ruby/gilded-rose-1/test-gilded-rose.rb:32]:
Expected: 0
Actual: -2
The fix:
def update_conjured_item(item)
item.sell_in = [0,item.sell_in-1].max
item.quality = [0,item.quality - 2].max
end
We adjust quality as we did sell_in, and the test runs. I’ll do one other little change to the second test. Since my name-matching function hasn’t really been exercised, I’ll change the second test to use a different name.
def test_conjured_quality_non_negative
gr = GildedRose.new([])
item = Item.new("low-value conjured item", sell_in=2, quality=2)
gr.update_item(item)
assert_equal(1, item.sell_in)
assert_equal(0, item.quality)
gr.update_item(item)
assert_equal(0, item.sell_in)
assert_equal(0, item.quality)
end
The tests still run. I think our feature works as requested. The playground isn’t any worse than it was (or at least not much), and we have a pattern developing for how to improve this code.
We’ll stop here for now, and commit our results. See you next time!
Oops! One more thing. I just remembered to run our characterization test, and of course it fails. The saved version includes a conjured item, and accepts an incorrect answer:
-------- day 0 --------
name, sellIn, quality
+5 Dexterity Vest, 10, 20
Aged Brie, 2, 0
Elixir of the Mongoose, 5, 7
Sulfuras, Hand of Ragnaros, 0, 80
Sulfuras, Hand of Ragnaros, -1, 80
Backstage passes to a TAFKAL80ETC concert, 15, 20
Backstage passes to a TAFKAL80ETC concert, 10, 49
Backstage passes to a TAFKAL80ETC concert, 5, 49
Conjured Mana Cake, 3, 6
-------- day 1 --------
name, sellIn, quality
+5 Dexterity Vest, 9, 19
Aged Brie, 1, 1
Elixir of the Mongoose, 4, 6
Sulfuras, Hand of Ragnaros, 0, 80
Sulfuras, Hand of Ragnaros, -1, 80
Backstage passes to a TAFKAL80ETC concert, 14, 21
Backstage passes to a TAFKAL80ETC concert, 9, 50
Backstage passes to a TAFKAL80ETC concert, 4, 50
Conjured Mana Cake, 2, 5
That last line needs to be 2, 4, not 2, 5. I’ll change that in the golden master and the test should now run. And it does.
Now for a break. I know I need a test for the weak match in the existing code, but it’ll take more brains and time than I have to spare just now. Next time?
Bye for now!