Gilded Rose: Now for something completely different.

Chet, Amitai Schleier, and I played with the Gilded Rose exercise the other day. I think I’ll write about it a bit, starting from scratch.

Emily Bache has provided the Gilded Rose refactoring “kata” in a batch of languages. Be sure to read the README.md file for Emily’s repo, as it provides some history and valuable links.

I’ve fiddled just a bit with the code, and have put the version I’m starting with into a GitHub repo at https://github.com/RonJeffries/gold-1. I haven’t made any code changes at all, but I have added a few comments to the program as I read it. Here’s all the code we’re starting with:

Emily’s provided texttest-fixture.rb:

#!/usr/bin/ruby -w

require File.join(File.dirname(__FILE__), 'gilded-rose')

puts "OMGHAI!"
items = [
  Item.new(name="+5 Dexterity Vest", sell_in=10, quality=20),
  Item.new(name="Aged Brie", sell_in=2, quality=0),
  Item.new(name="Elixir of the Mongoose", sell_in=5, quality=7),
  Item.new(name="Sulfuras, Hand of Ragnaros", sell_in=0, quality=80),
  Item.new(name="Sulfuras, Hand of Ragnaros", sell_in=-1, quality=80),
  Item.new(name="Backstage passes to a TAFKAL80ETC concert", sell_in=15, quality=20),
  Item.new(name="Backstage passes to a TAFKAL80ETC concert", sell_in=10, quality=49),
  Item.new(name="Backstage passes to a TAFKAL80ETC concert", sell_in=5, quality=49),
  # This Conjured item does not work properly yet
  Item.new(name="Conjured Mana Cake", sell_in=3, quality=6), # <-- :O
]

days = 2
if ARGV.size > 0
  days = ARGV[0].to_i + 1
end

gilded_rose = GildedRose.new items
(0...days).each do |day|
  puts "-------- day #{day} --------"
  puts "name, sellIn, quality"
  items.each do |item|
    puts item
  end
  puts ""
  gilded_rose.update_quality
end

What we have here is just a main program that applies the provided code to a few representative objects for sale and prints the results. There is some kind of tool that can save and compare that output for us, but I plan to go another way. Nonetheless, running the existing program is the only way we can be sure we haven’t changed anything we didn’t mean to.

A starting unit test:

require "./gilded-rose"
require "minitest/autorun"

class TestGildedRose<Minitest::Test
  
  def test_hookup
    assert true
  end

  def test_foo
    items = [Item.new("foo", 0, 0)]
    GildedRose.new(items).update_quality()
    assert_equal items[0].name, "foo"
  end

end

This is adapted from Emily’s provided test to use minitest/autorun, which is the testing framework I sort of know how to hook up and use. I use Sublime Text as my editor on the Mac and a Command-B will build and run the tests. The result of doing that right now is:

# Running:

..

Finished in 0.000856s, 2336.4486 runs/s, 2336.4486 assertions/s.

2 runs, 2 assertions, 0 failures, 0 errors, 0 skips
[Finished in 0.6s]

The existing program:

class GildedRose

  def initialize(items)
    @items = items
  end

  def update_quality()
    @items.each do |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
  end
end

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 the code we’re supposed to change. We’re supposed to add a feature to it, and of course the real exercise is to get the code under enough tests to allow us to refactor it to a cleaner form as part of doing that.

For completeness, the instructions:

======================================
Gilded Rose Requirements Specification
======================================

Hi and welcome to team Gilded Rose. As you know, we are a small inn with a prime location a prominent city ran by a friendly innkeeper named Allison. We also buy and sell only the finest goods. Unfortunately, our goods are constantly degrading in quality as they approach their sell by date. We have a system in place that updates our inventory for us. It was developed by a no-nonsense type named Leeroy, who has moved on to new adventures. Your task is to add the new feature to our system so that we can begin selling a new category of items. First an introduction to our system:

• All items have a SellIn value which denotes the number of days we have to sell the item
• All items have a Quality value which denotes how valuable the item is
• At the end of each day our system lowers both values for every item

Pretty simple, right? Well this is where it gets interesting:

• Once the sell by date has passed, Quality degrades twice as fast
• The Quality of an item is never negative
• “Aged Brie” actually increases in Quality the older it gets
• The Quality of an item is never more than 50
• “Sulfuras”, being a legendary item, never has to be sold or decreases in Quality
• “Backstage passes”, like aged brie, increases in Quality as its SellIn value approaches;
  Quality increases by 2 when there are 10 days or less and by 3 when there are 5 days or less but Quality drops to 0 after the concert

We have recently signed a supplier of conjured items. This requires an update to our system:

• “Conjured” items degrade in Quality twice as fast as normal items

Feel free to make any changes to the UpdateQuality method and add any new code as long as everything still works correctly. 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).

Just for clarification, an item can never have its Quality increase above 50, however “Sulfuras” is a legendary item and as such its Quality is 80 and it never alters.

From prior experience with Chet and Amitai, and from trying to read that marvelous code, I’m fairly sure that what the program currently does is not what the spec says. I don’t remember the details, but I do recall that it had to do with when quality increases. The spec isn’t clear enough to be sure. I’m sure we’ll encounter that issue again as we go forward.

Let’s get started

When faced with a legacy program like this one, or even one that wasn’t written maliciously, as this one clearly was, we’re torn between two goals that are at least a bit in conflict. On the one hand, we have some new capability that “they” want put into the system. On the other hand, we need some time to get the code under test and into decent shape to be changed.¹

Our mission is to add “Conjured” items to the system, which follow all the rules except that they degrade in quality twice as fast as normal items, that is, they lose two quality points per day.

Now honestly, I think it would be pretty straightforward to put an if statement at the top of the update_quality method and handle the conjured items and walk away. Doing that would have the advantage of getting a bit of testing done, and it would provide a template for what a more reasonable program might look like.

One reasonable look for the whole method might be to move it in the direction of a sort of switch statement, or if-elseif blocks, each one handling a particular kind of product. We could, in due time, extend our initial if statment for “conjured” to include all the other “normal” items, and deal with the individual special cases one at a time, untangling that quaint and curious structure of if-nots that are in there now.

We could do that first move pretty simply, and I fear that’s what most teams faced with this kind of code would do: make a special case for our current new feature and move on.

It happens that I’ve seen the result of working that way, at an Internet advertising company I worked with for a while. They wound up with a single huge method of if upon if upon if, and they were never sure quite where to put the new ones. It was truly awful. The more they worked, the worse it got and the more surprise defects they encountered.

Now Gilded Rose is a small enough company that it might not be that bad, but we’ve got permission to improve this code, not just add a feature. So we plan to leave this code better than it was.

Knowing me, we’ll go wild on that, unless I become bored. One never knows.

We do have that start at a text test. When we run it, we get this output:

OMGHAI!
-------- 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

[Finished in 0.4s]

We can see some interesting issues already. They include:

• Conjured Mana Cake is clearly wrong, but we knew that;
• Aged Brie is increasing in quality by 1, which may or may not be what the spec means;
• The test doesn’t cover enough days, or enough items, to tell us everything we’d like to know.

Still, we can probably use this text test as a “characterization test” for the application. A characterization test is a test that captures the behavior of the whole application, or a big part of it, and preserves it in what’s often called a “Golden Master”. When we change the program, we run it again, and if the output matches the Golden Master, we conclude that we haven’t broken anything. If the new output is deemed better than the old, we make it the new Golden Master.

I think I’ll do that first. For now, I’ll take a really simple approach: I’ll save the current output in a string, and then compare subsequent runs’ output to that string. A more clever programmer would save the output to a file, and we might do that soon. For now, I’ll go with the string.

Mysteriously, that worked almost immediately. Here’s the new test:

#!/usr/bin/ruby -w

require File.join(File.dirname(__FILE__), 'gilded-rose')

expected = "-------- 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

"

items = [
  Item.new(name="+5 Dexterity Vest", sell_in=10, quality=20),
  Item.new(name="Aged Brie", sell_in=2, quality=0),
  Item.new(name="Elixir of the Mongoose", sell_in=5, quality=7),
  Item.new(name="Sulfuras, Hand of Ragnaros", sell_in=0, quality=80),
  Item.new(name="Sulfuras, Hand of Ragnaros", sell_in=-1, quality=80),
  Item.new(name="Backstage passes to a TAFKAL80ETC concert", sell_in=15, quality=20),
  Item.new(name="Backstage passes to a TAFKAL80ETC concert", sell_in=10, quality=49),
  Item.new(name="Backstage passes to a TAFKAL80ETC concert", sell_in=5, quality=49),
  # This Conjured item does not work properly yet
  Item.new(name="Conjured Mana Cake", sell_in=3, quality=6), # <-- :O
]

days = 2
if ARGV.size > 0
  days = ARGV[0].to_i + 1
end

actual = StringIO.new
gilded_rose = GildedRose.new items
(0...days).each do |day|
  actual.puts "-------- day #{day} --------"
  actual.puts "name, sellIn, quality"
  items.each do |item|
    actual.puts item
  end
  actual.puts ""
  gilded_rose.update_quality
end

puts actual.string == expected

And the output is:

true
[Finished in 0.2s]

Now I will surely regret not displaying any details in the event that the strings don’t match, and I’ll surely also regret the need to fiddle around to get a new Golden Master when I need one. But the scaffolding is in place, and that’s a good thing.

With this in place, I’d like to make a small change to the update_quality method. Presently, it loops over all items, updating each one in place. I want to begin writing some small tests that check individual item updates. To make that easier, I want an update_item method that just updates one item, so that I can more easily test it.

So I want to extract the entire contents of update_quality into update_item. After doing that, I’ll run the text test again and if it passes, I can be sure I did it right. Here goes, hold my Diet Coke.

class GildedRose

  def initialize(items)
    @items = items
  end

  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

  def update_quality()
    @items.each do |item|
      update_item(item)
    end #do
  end #update_quality
end # Gilded_Rose

And it works! I’m not very surprised, it was a pretty simple change. But I am a bit surprised, because this legacy program is a house of cards and I rather expect it to fall down every time I touch it.

So this is good. We’ve got a characterization test in place, albeit a weak one, and we’ve used it to refactor the program just enough to let us test some individual updates.

That’s enough for this morning, I think. Tune in next time!