Remember our new method for handling conjured items?

  def update_conjured_item(item)
    item.sell_in = [0,item.sell_in-1].max
    item.quality = [0,item.quality - 2].max
  end

We decided – well, I decided – that sell_in can never drop below zero. It made sense to me, and my pair didn’t say anything other than “Meow”, so I went with it. But this morning I woke up wondering … could it?

Reading the spec again, though, I don’t see anything about whether sell_in can go negative or not. It doesn’t make much sense to me to let it be negative, but what does the code do?

A closer look at the text test gives us part of the answer. Look at the “sulfuras” entry in these two days of output:

-------- 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, 4

Now “Sulfuras” never has to be sold nor does its value decrease, but the test explicitly includes a sell_in of -1. So maybe it can go negative. Let’s look at the code and see what we can deduce:

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

Yes. That if statment with the arrow tells us that sell_in can go negative and in fact for some items their value increases as they get past their date. What about our conjured item, though? The only special instruction is that its quality decreases twice as fast. And we do see in the spec that quality never goes negative. But it seems that our sell_in assumption was wrong. We need to adjust.

The fix of course is easy, just remove the max from the sell_in adjustment. But we need to fix up the tests as well. We do have this test:

  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

We could run that for another day and test that sell_in goes to -1. Or we could write a new test. Since this one is already a bit complex, let’s do a new one:

  def test_conjured_sell_in_negative
    gr = GildedRose.new([])
    item = Item.new("low-value conjured item", sell_in=0, quality=2)
    gr.update_item(item)
    assert_equal(-1, item.sell_in)
  end

This test fails:

  1) Failure:
TestGildedRose#test_conjured_sell_in_negative [/Users/ron/Dropbox/Ruby/gilded-rose-1/test-gilded-rose.rb:39]:
Expected: -1
  Actual: 0

And the fix is simple:

  def update_conjured_item(item)
    item.sell_in = item.sell_in - 1
    item.quality = [0,item.quality - 2].max
  end

So that’s a fortunate thought. And it tells us that our text test is pretty weak. We were given that test, and it seemed that we should be allowed to assume that it was sufficiently comprehensive to include everything important. Apparently, when we assume … you know the rest.

Should we beef up that test and improve our golden master? I think we should.

We can improve it in at least two ways. We could run it for more days, or we could add items to the test, carefully arranging them so that the two days of test are enough. (Note that it’s really only one day of test, since the first batch of output comes out before we update.)

Adding items is more pointed, more directed, more thoughtful. We carefully choose things to test. Running for more days, it seems to me, is more likely to turn up unexpected results, perhaps even results where the current code doesn’t match the spec.

I think we should do both. First, however, I’ll add an item, then look at making the test run for more days. I’ll use my new Conjured item … and now that I think of it, I’ll use another item from the legacy code. I think I’ll make both of them produce a -1 sell_in. But I’m not feeling god-like, so I’ll do them one at a time.

#!/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
Conjured Voodoo Doll, 0, 1

-------- 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, 4
Conjured Voodoo Doll, -1, 0

"

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),
  Item.new(name="Conjured Voodoo Doll", sell_in=0, quality=1)
]

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

You can see my new Conjured Voodoo Doll, and the expected output. I made it extra tricky by setting the quality to 1 so that it should drop, not by two, but be pinned to quality zero, since quality never goes negative.

That worked. Now let’s find some ordinary item and test it the same way.

#!/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
Conjured Voodoo Doll, 0, 1
+1 Sword: Assbiter, 0, 20

-------- 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, 4
Conjured Voodoo Doll, -1, 0
+1 Sword: Assbiter, -1, 19

"

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),
  Item.new(name="Conjured Voodoo Doll", sell_in=0, quality=1),
  Item.new(name="+1 Sword: Assbiter", sell_in=0, quality=20)
]

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
puts actual.string

You can see the new puts there at the end, because, to my surprise, this test failed! As you can see, Assbiter’s value went from 20 to 18 for some reason. So I needed to look at the output.

But why? I’m sure it’s not going through my “conjured” code. What else is going on?

This is of course a common and serious issue with legacy code like this. We think we know what it does and then it surprises us. I guess there’s nothing for it but to look at the code, and maybe trace it, to see what’s going on.

Well, hell. A read of the code shows us this patch:

      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

The code is doing it on purpose. So I go back to the spec, and sure enough:

Once the sell by date has passed, Quality degrades twice as fast

So the test is wrong, the code is right. But what does that mean for my conjured item, which degrades twice as fast. After its sell_in, should it degrade by 4 per day? We really need to ask Allison. Let’s decide for her that, yes, conjured items go down by 4 if they’ve gone past their date.

Now some of our tests are wrong, so we’ll change them and make them work. First the existing text 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
Conjured Voodoo Doll, 0, 1
+1 Sword: Assbiter, 0, 20

-------- 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, 4
Conjured Voodoo Doll, -1, 0
+1 Sword: Assbiter, -1, 18

"

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),
  Item.new(name="Conjured Voodoo Doll", sell_in=0, quality=1),
  Item.new(name="+1 Sword: Assbiter", sell_in=0, quality=20)
]

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
if actual.string != expected
  puts actual.string
end

You can see that I’ve tweaked the text test to print the actual string if it doesn’t match. If this gets much more weird, I plan to beef this up to display only unmatched lines or something.

In my experience, this is quite common with legacy-focused tests. We start with something pretty dumb but quick to do, then as time passes, we improve the tests to provide more information faster. Our hope is that we’ll just see a lot of “true” and not need to dig in further. Sometimes our hopes are dashed and we improve our tools to help us, as needed.

I still need a test for the double-dipping on my conjured items. Let’s do the unit test first:

  def test_conjured_double_dip_past_sell_in
    gr = GildedRose.new([])
    item = Item.new("double-dip conjured item", sell_in=0, quality=10)
    gr.update_item(item)
    assert_equal(6, item.quality)
  end

This fails, as expected (but see below) and works when we change our method to this:

  def update_conjured_item(item)
    item.sell_in = item.sell_in - 1
    item.quality = [0,item.quality - 2].max
    if item.sell_in < 0
      item.quality = [0,item.quality - 2].max
    end
  end

So there we have code that subtracts two from quality and then if sell_in is negative, does it again. It works. Remind me to upgrade the text test to include an item like this one.

However …

This method is starting to look like legacy code to me. It doesn’t really express the spec, which I’d state as something like this:

Conjured items degrade twice as fast as normal items, including after they’re expired, i.e. four times as fast as normal, or twice as fast as the double degradation of normal items.

This code doesn’t express that at all. Let’s see if we can improve its ability to communicate, perhaps at the cost of a bit more code:

  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

Now I can’t say that I love this, but it seems to me to be closer to what we mean. Of course, some of this might be factored out into class variables or high-level constants or something, but for now I’m happy to keep it internal to our new code.

The tests still run, by the way.

What would be better? Well, this would be better if we could say it:

  def update_conjured_item(item)
    item.sell_in = item.sell_in - 1
    item.quality = [0,item.quality - item.current_degradation].max
  end

The amount an item degrades is clearly a function of the item itself. Normals go by one, conjured by double normal, sulfuras never, out of date items double their usual degradation.

So if the world were fair, we’d put the item’s degradation in as a method on item and we’d let it dynamically tell us how much to degrade.

Or, better yet, how about if items just knew how to adjust their own quality? That would be even better:

  def update_conjured_item(item)
    item.sell_in = item.sell_in - 1
    item.adjust_quality
  end

And probably they should adjust their sell_in as well.

This principle is often called “Tell, don’t ask”. We shouldn’t ask the item what its quality is and then compute a new quality and stuff it back. We should tell the item what we want it to do and have it do it.

Unfortunately, there’s a goblin in the corner who won’t let us change the Item class. So that’s out, at least for now.

But stay tuned: I think we’ll get a lot closer to that ideal as we go forward.

For now, we’ve done almost enough, except that I think I’ll add one more test to the text test, matching my new unit 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
Conjured Voodoo Doll, 0, 1
+1 Sword: Assbiter, 0, 20
conjured double dipper, 0, 10

-------- 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, 4
Conjured Voodoo Doll, -1, 0
+1 Sword: Assbiter, -1, 18
conjured double dipper, -1, 6

"

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),
  Item.new(name="Conjured Voodoo Doll", sell_in=0, quality=1),
  Item.new(name="+1 Sword: Assbiter", sell_in=0, quality=20),
  Item.new(name="conjured double dipper", sell_in=0, quality = 10)
]

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
if actual.string != expected
  puts actual.string
end

That returns true. It’s about lunch time. I was delayed a bit by submitting an order to the store for pickup. We’ve found a serious problem, and it was sheerly by luck in my opinion. Somehow I just started wondering.

This is a very common situation when we’re new to some complex requirements and complex code base. We need to expect sudden enlightenment … and, unfortunately, sometimes we’ll break something even so. We’ll be as careful as we can. And we would do well to be pairing and or working closely with our customer, who understands things better than we do.

Even so, there’s always trouble. There is no magic wand, and there’s no chance to avoid human error. If our code is life-critical or even big money critical, we need to proceed with great caution, building up stronger and stronger tests as we go on.

This will be going on in the face of some manager somewhere who wants us to get more done sooner. Sticking to the best approaches we know is our best hope in these situations, even when we’re under pressure to cut corners.

See you next time!