It’s Monday, the cat has been fed and allowed into her outdoor cage. I’ll grab a banana and maybe an Oats ‘n (sic) Honey bar and do a bit more refactoring. Here’s The Method:

  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 #1 for sword
        end
      end
    elsif brie?(item)
      update_brie(item)
    elsif tickets?(item)
      update_ticket(item)
    end
    if item.name != "Sulfuras, Hand of Ragnaros"
      item.sell_in = item.sell_in - 1
    end
    if expired?(item)
      if brie?(item)
        increment_quality_up_to_50(item)
      elsif tickets?(item)
        set_quality_zero(item)
      else
        if item.quality > 0
          if item.name != "Sulfuras, Hand of Ragnaros"
            item.quality = item.quality - 1
          end
        end
      end
    end 
  endgilded-rose-

Well, first off we can change this …

    if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"

… to this:

    if !brie?(item) and !tickets?(item)

That’s somewhat more clear, at least by my standards.

There’s another advantage to creating and using these quick-check methods: the checks themselves are weak, checking only a single specific product name. When backstage passes come along for Osric and the Vandals, the code will have to be changed to make it work. Here we have a single place to change, and we can create a more robust check. Right now we’re refactoring, but we should probably make a card for improving the name checks.

I even committed the code. I’d be a better person, perhaps, if I would commit the code on every green bar.

Speaking of green bars (Ruby doesn’t even give me a green bar) a few people on Twitter have been advising me to try mutation testing because it helps one find loopholes in the tests. Mutation testing makes small directed changes to your code under test and runs the tests. Presumably on any such change, the tests should fail. If they don’t, you’ve just learned something.

I’ve never used mutation testing and I don’t have the tools. Perhaps I’ll pair with someone and try it later on. For now, I’ll proceed like the dullard I am.

However, dullard though I may be, I did have one decent idea. Why not convert the text test to run as a micro test so that when I run my tests, that one runs as well. That would save me some time and also make it a bit more likely that I’ll remember to run both kinds of tests. Since we’re at a green bar and the code is committed, I think I’ll do that.

OK, this is predictably ugly, but it works:

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

    assert_equal(expected, actual.string)
    # if actual.string != expected
    #   puts actual.string
    # end
  end

I just had to comment out the check for ARGV, which I wasn’t using anyway, and the dump of the output, because our testing framework will dump the results if they don’t match anyway. I even inadvertently checked to be sure the test will fail, by tabbing the big string in to make it line up better, which is fine except for the leading spaces that result.

So that’s good. Back to the update …

  def update
    item = @item
    if !brie?(item) and !tickets?(item)
      if item.quality > 0
        if item.name != "Sulfuras, Hand of Ragnaros"
          item.quality = item.quality - 1 #1 for sword
        end
      end
    elsif brie?(item)
      update_brie(item)
    elsif tickets?(item)
      update_ticket(item)
    end
    if item.name != "Sulfuras, Hand of Ragnaros"
      item.sell_in = item.sell_in - 1
    end
    if expired?(item)
      if brie?(item)
        increment_quality_up_to_50(item)
      elsif tickets?(item)
        set_quality_zero(item)
      else
        if item.quality > 0
          if item.name != "Sulfuras, Hand of Ragnaros"
            item.quality = item.quality - 1
          end
        end
      end
    end 
  end

In the spirit of doing things that are suitably easy for a Monday, let’s create a sulfuras-checking method and use it:

  def update
    item = @item
    if !brie?(item) and !tickets?(item)
      if item.quality > 0
        if !sulfuras?(item)
          item.quality = item.quality - 1 #1 for sword
        end
      end
    elsif brie?(item)
      update_brie(item)
    elsif tickets?(item)
      update_ticket(item)
    end
    if !sulfuras?(item)
      item.sell_in = item.sell_in - 1
    end
    if expired?(item)
      if brie?(item)
        increment_quality_up_to_50(item)
      elsif tickets?(item)
        set_quality_zero(item)
      else
        if item.quality > 0
          if !sulfuras?(item)
            item.quality = item.quality - 1
          end
        end
      end
    end 
  end

  def sulfuras?(item)
    item.name == "Sulfuras, Hand of Ragnaros"
  end

This went smoothly except for two mistakes. First, I forgot to put the item parameter into the sulfuras? method, and second when I edited the != I had copied and pasted to make ==, I managed to make just =, which kind of broke most of the program. Both were fairly obvious and the whole process took just a couple of minutes. I was right to work with small changes, though. Of course that’s almost always right.

Let’s commit, we’re green again.

Now let’s look at those three negative usages of sulfuras?. What are they saying? Well, quality doesn’t decrease if it’s Sulfuras (in two different places, isn’t that interesting) and neither does sell_in. The spec expresses exactly that:

“Sulfuras”, being a legendary item, never has to be sold or decreases in Quality

Well, hell, she said, if Sulfuras doesn’t adjust either of the input values, how about if we just early-out the whole thing. It won’t change behavior, so it is in fact a refactoring, although I don’t know the name of it, if it has a name. Anyway, try this:

  def update
    item = @item
    return if sulfuras?(item)
    if !brie?(item) and !tickets?(item)
      if item.quality > 0
        item.quality = item.quality - 1 #1 for sword
      end
    elsif brie?(item)
      update_brie(item)
    elsif tickets?(item)
      update_ticket(item)
    end
    item.sell_in = item.sell_in - 1
    if expired?(item)
      if brie?(item)
        increment_quality_up_to_50(item)
      elsif tickets?(item)
        set_quality_zero(item)
      else
        if item.quality > 0
          item.quality = item.quality - 1
        end
      end
    end 
  end

As predicted, that works. Now that this method is small, I’m regretting the trick I did with item = @item to avoid editing lots of lines of code. Let’s change all this code to reflect the fact that we are processing exactly one item and it is our member variable. I did the whole wrappers file, including our other wrapper for conjured goods. I’ll show you the whole thing later, for now let’s just look at our formerly horrible update.

  def update
    return if sulfuras?
    if !brie? and !tickets?
      if @item.quality > 0
        @item.quality = @item.quality - 1 #1 for sword
      end
    elsif brie?
      update_brie
    elsif tickets?
      update_ticket
    end
    @item.sell_in = @item.sell_in - 1
    if expired?
      if brie?
        increment_quality_up_to_50
      elsif tickets?
        set_quality_zero
      else
        if @item.quality > 0
          @item.quality = @item.quality - 1
        end
      end
    end 
  end

What is that comment about sword, anyway? Did I put it there? Let’s remove that.

Now let’s re-order that first if nest:

  def update
    return if sulfuras?
    if brie?
      update_brie
    elsif tickets?
      update_ticket
    else
      if @item.quality > 0
        @item.quality = @item.quality - 1
      end
    end
    @item.sell_in = @item.sell_in - 1
    if expired?
      if brie?
        increment_quality_up_to_50
      elsif tickets?
        set_quality_zero
      else
        if @item.quality > 0
          @item.quality = @item.quality - 1
        end
      end
    end 
  end

Let’s extract and simplify those two ifs about quality:

  def update
    return if sulfuras?
    if brie?
      update_brie
    elsif tickets?
      update_ticket
    else
      decrement_quality_down_to_zero
    end
    @item.sell_in = @item.sell_in - 1
    if expired?
      if brie?
        increment_quality_up_to_50
      elsif tickets?
        set_quality_zero
      else
        decrement_quality_down_to_zero
      end
    end 
  end

  def decrement_quality_down_to_zero
    @item.quality = @item.quality - 1 if @item.quality > 0
  end

Now let’s extract that whole block beginning if expired?:

  def update
    return if sulfuras?
    if brie?
      update_brie
    elsif tickets?
      update_ticket
    else
      decrement_quality_down_to_zero
    end
    @item.sell_in = @item.sell_in - 1
    handle_expired_items
  end

  def handle_expired_items
    if expired?
      if brie?
        increment_quality_up_to_50
      elsif tickets?
        set_quality_zero
      else
        decrement_quality_down_to_zero
      end
    end 
  end

Now let’s simplify handle_expired_items:

  def handle_expired_items
    return if ! expired?
    if brie?
      increment_quality_up_to_50
    elsif tickets?
      set_quality_zero
    else
      decrement_quality_down_to_zero
    end
  end

I had hoped to turn those if-elsifs into one-liners but the else thwarted me. Still, by my standards it’s a bit nicer. Tests run, let’s commit.

I’ll include all the code at the bottom, and it’s up on GitHub. But let’s look at just these two bits:

  def update
    return if sulfuras?
    if brie?
      update_brie
    elsif tickets?
      update_ticket
    else
      decrement_quality_down_to_zero
    end
    @item.sell_in = @item.sell_in - 1
    handle_expired_items
  end

  def handle_expired_items
    return if ! expired?
    if brie?
      increment_quality_up_to_50
    elsif tickets?
      set_quality_zero
    else
      decrement_quality_down_to_zero
    end
  end

Notice that they are now quite parallel. Each has special handling for brie, then tickets, then an else. Also note that there are two decrements on quality. Does that mean that somehow expired items decrement quality twice? Yes, in fact it does, according to the spec:

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

That’s just awfully obscure, though, isn’t it? On the other hand, our method update_brie looks like this:

  def update_brie
    increment_quality_up_to_50
  end

This makes me suspect that brie that is past its expiry date increases in quality by 2, not by 1. The spec isn’t clear on that, in my opinion. What about the tests, do we have a test for that? As it happens, we do not. Let’s write one:

  def test_past_due_brie_increases_by_1
    gr = GildedRose.new([])
    brie = Item.new("Aged Brie", sell_in=0, quality=40)
    gr.update_item(brie)
    assert_equal(41, brie.quality)
  end

This test fails, with the new quality of 42, as the code now almost clearly says.

Of interest, however, is whether it would have done that originally. The text test does not cover the case. And honestly I don’t know which way it was, though I do recall thinking upon reading the code that there was a way to double-touch something. Let’s first just review the original code:

  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

Imagine we go in there with expired brie, as with our current unit test.

If you trace through that, I think you’ll agree that in fact it will increment quality twice, just as it does now. I’m not entirely comfortable with that, but I’d say I’m above 90%. I’m also wondering if that’s what’s intended, so I’ll ask Allison next time I encounter her.

For extra certainty, we could revert our code to a suitable spot and run our unit tests. That would be the best thing to do. If it is wrong, we know exactly the line to remove, the one controlled by if brie? in the handle_expired_items.

Today I am past my own sell-in date, so I’ll put off dealing with this question until next time. I do expect to be vindicated, but we’ll find out.

Looking forward, I’m inclined to combine the update and handle_expired_itemsback together. In fact, I really can’t resist doing it right now. Hold my water bottle:

Nope. On second glance, that code decrements the sell_in date right in the middle, so we can’t just fold the two together.

Good time to stop, and we’re on a red bar with that double decrement test, to remind us where to start next time. Meanwhile this code is surely much simpler than it began, and I’m quite sure it works just like the original.

See you next time!

# Gilded Rose

require "./wrappers"

class GildedRose

  def initialize(items)
    @items = items
  end

  def match_name(item,name)
    return item.name.downcase.include? name.downcase
  end

  def appropriate_wrapper_class(item)
    return ConjuredWrapper if match_name(item, "conjured")
    return OriginalWrapper
  end

  def update_item(item)
    wrapper_class = appropriate_wrapper_class(item)
    wrapped_item = wrapper_class.new(item)
    wrapped_item.update
  end

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

# wrapper classes

class ConjuredWrapper
  def initialize(item)
    @item = item
  end

  def conjured_degradation
    normal_degradation = 1
    if @item.sell_in < 0
      normal_degradation = 2*normal_degradation
    end
    conjured_degradation = 2*normal_degradation
    return conjured_degradation
  end

  def update
    @item.sell_in = @item.sell_in - 1
    @item.quality = [0,@item.quality - conjured_degradation].max
  end

end

class OriginalWrapper
  def initialize(item)
    @item = item
  end

  def update
    return if sulfuras?
    if brie?
      update_brie
    elsif tickets?
      update_ticket
    else
      decrement_quality_down_to_zero
    end
    @item.sell_in = @item.sell_in - 1
    handle_expired_items
  end

  def handle_expired_items
    return if ! expired?
    if brie?
      increment_quality_up_to_50
    elsif tickets?
      set_quality_zero
    else
      decrement_quality_down_to_zero
    end
  end

  def decrement_quality_down_to_zero
    @item.quality = @item.quality - 1 if @item.quality > 0
  end

  def sulfuras?
    @item.name == "Sulfuras, Hand of Ragnaros"
  end

  def update_brie
    increment_quality_up_to_50
  end

  def update_ticket
    increment_quality_up_to_50
    if @item.sell_in < 11
      increment_quality_up_to_50
    end
    if @item.sell_in < 6
      increment_quality_up_to_50
    end
  end

  def tickets?
    @item.name == "Backstage passes to a TAFKAL80ETC concert"
  end

  def brie?
    @item.name == "Aged Brie"
  end

  def set_quality_zero
    @item.quality = 0
  end

  def expired?
    @item.sell_in < 0
  end

  def increment_quality_up_to_50
      @item.quality += 1 unless @item.quality >= 50
  end

end

#test-gided-rose.rb

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

class TestGildedRose<Minitest::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

  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

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

  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

  def test_brie_quality_increases_to_50
    gr = GildedRose.new([])
    brie = Item.new("Aged Brie", sell_in=0, quality=49)
    gr.update_item(brie)
    assert_equal(50, brie.quality)
    gr.update_item(brie)
    assert_equal(50, brie.quality)
  end

  def test_past_due_brie_increases_by_1
    gr = GildedRose.new([])
    brie = Item.new("Aged Brie", sell_in=0, quality=40)
    gr.update_item(brie)
    assert_equal(41, brie.quality)
  end

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

    assert_equal(expected, actual.string)
    # if actual.string != expected
    #   puts actual.string
    # end
  end

end