Today I think I’ll double-check that double-decrement issue. Then I want to see if we can combine those two parallel if nests.

My cunning plan is to copy the failing unit test, then revert the code, put in that test, and see if it fails. As we stand:

  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

    1) Failure:
TestGildedRose#test_past_due_brie_increases_by_1 [/Users/ron/Dropbox/Ruby/gilded-rose-1/test-gilded-rose.rb:54]:
Expected: 41
  Actual: 42

Now to revert and test again. I’ve chosen this commit to revert to:

commit 01a059f82c22e52a3c4041dc9a30cace528ca437
Author: Ronald Jeffries <ronjeffries@acm.org>
Date:   Tue Apr 21 12:49:04 2020 -0400

    Added OriginalWrapper and used it

Check out, paste in the test, run it … and it fails! Perfect! We have duplicated the original behavior. Now to check out our latest version and get down to real work.

First, change the above test to expect 42, which is the correct value (obviously). Tests all run. Sweet.

Now I see the parallelism between the following two methods, and I’d like to remove it:

  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

Were it not for the adjustment to sell_in, we could just merge the ifs. It’s clear, I hope, that handle_expired_items` needs the date to be updated, so that the check for expired is valid. (That check also makes me a bit leery of the merge I have in mind, but I think if we can do it we’ll like the result.)

Could we move the adjustment of sell_in before the if nest? I guess the easiest thing is to try it and see what fails.

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

Tests still run. If I were truly confident in the tests, I’d go ahead right now. Since I’m not (yes, I hear you, mutation-test supporters), I’m going to inspect the code in the update as well. In this case it’ll only take a moment, but in a real situation, we’d really want to be more confident in the tests.

Yes, this is troubling. Yes, it’s also a taste of reality.

Here’s the code of concern:

  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

Brie is obviously OK. the ticket update depends on sell_in and is concerning. I think we want to write a test. Here’s why. The way this code “works” is that ticket quality will be incremented once if sell_in is less than 11 and is 6 or more, that is, 10,9,8,7, or 6. So as originally written, a ticket should not increment on day 6 going in, but after this change, since we’ll have decremented the date, it should update. I’ll move the date decrement back and write a test.

Hmm. This test runs correctly, and I don’t think it should:

  def test_ticket_does_not_increase_on_day_6
    gr = GildedRose.new([])
    tick = Item.new("Backstage passes to a TAFKAL80ETC concert", sell_in=11, quality=40)
    gr.update_item(tick)
    assert_equal(41, tick.quality)
    gr.update_item(tick)
    assert_equal(43, tick.quality)
  end

It seems to me that on the first update, sell_in should be 10, which is less than 11, and therefore I should get a second increment to quality. Then, surely on the next day, I should get two more. But on the first update I get only one tick. Why?

Ah, I see. The update_ticket is called before the date is incremented. So the decision to bump quality is something like “after the day expires”, not “when the day starts”. This is a bit confusing to me. Maybe more than a bit.

Let’s look at the text test. The input is:

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

Looking at those, I expect the second two tickets to get extra value. I expect the 10 day one go to from 49 to 51, and the 5 day on to go from 49 to 52.

But that’s not what the golden master says:

Backstage passes to a TAFKAL80ETC concert, 15, 20
Backstage passes to a TAFKAL80ETC concert, 10, 49
Backstage passes to a TAFKAL80ETC concert, 5, 49
...
Backstage passes to a TAFKAL80ETC concert, 14, 21
Backstage passes to a TAFKAL80ETC concert, 9, 50
Backstage passes to a TAFKAL80ETC concert, 4, 50

What’s up with that? The spec clearly says “10 days or less” and “5 days or less”.

Have I saved a bad golden master without noticing? Let’s look all the way back in history … and no, those values are the values in the very first run of the tests.

OH!! Those tests max at 50, so they don’t even test the special values. And we’ve already noticed that the way the text test works is to output the starting values and then the ending values for all the days it increments.

Whew. Our test will be the first real test for tickets. Let’s review it and be sure we’re good.

I paused right here to get a bowl of cereal. And, you know, I’m really kind of ticked off at this situation we find ourselves in. We have some questionable behavior that we’ve discovered, first the question of whether Brie increases its quality by 2 after expiry. The code says yes, the spec is unclear.

Now, we have this interesting behavior of the tickets, where they get their extra value after day ten and five, not on day ten or five. Here, I think the spec rather clearly has it the other way around. Let’s finish these tests and then I want to steam some more.

My first test has been elaborated thus:

  def test_ticket_does_not_increase_on_day_10_but_after_10
    gr = GildedRose.new([])
    tick = Item.new("Backstage passes to a TAFKAL80ETC concert", sell_in=11, quality=40)
    gr.update_item(tick)
    assert_equal(10, tick.sell_in)
    assert_equal(41, tick.quality)
    gr.update_item(tick)
    assert_equal( 9, tick.sell_in)
    assert_equal(43, tick.quality)
  end

This runs green. Now for another, testing day 5. Same pattern.

  def test_ticket_does_not_increase_on_day_5_but_after_5
    gr = GildedRose.new([])
    tick = Item.new("Backstage passes to a TAFKAL80ETC concert", sell_in=6, quality=40)
    gr.update_item(tick)
    assert_equal(5, tick.sell_in)
    assert_equal(42, tick.quality)
    gr.update_item(tick)
    assert_equal( 4, tick.sell_in)
    assert_equal(45, tick.quality)
  end

OK. This rather clearly documents what the code actually does, and we’ll have to check with Allison on whether it is what she wants.

Now I can try moving the date up and I’m sure it won’t work if that’s all I do.

Sure enough, both those tests fail as expected, ticking a day early.

My theory that I could combine those parallel ifs is in question, but it should be “obvious” that we can adjust the update_ticket code to make the tests run as they did before. (If we leave it this way it probably does what Allison said, but we can’t change behavior without her permission or request.)

If I do this:

  def update_ticket
    increment_quality_up_to_50
    if @item.sell_in < 10
      increment_quality_up_to_50
    end
    if @item.sell_in < 5
      increment_quality_up_to_50
    end
  end

I expect the tests to run with the date decrement moved up.

And they do. Now we can move the elements of the if block up if we wish. It would look like this:

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

The tests now run. All of them. We can delete the handle_expired_items method entirely.

Look at what we have accomplished. There are a few helper methods, but our update has gone from 48 very intricate lines to 14 straightforward ones. But we’re not done.

We now have a bit of oddness inside the ifs. We could expand update_brie to make that block be at a consistent level, but expanding update_ticket is more messy because of that date stuff. Instead, let’s fold our conditional lines inside those two methods:

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

  def update_brie
    increment_quality_up_to_50
    increment_quality_up_to_50 if expired?
  end

  def update_ticket
    increment_quality_up_to_50
    if @item.sell_in < 10
      increment_quality_up_to_50
    end
    if @item.sell_in < 5
      increment_quality_up_to_50
    end
    set_quality_zero if expired?
  end

And yes, the tests all run. The update_ticket method is a bit odd, because it will increment quality and then clobber it if the ticket is expired. The update_brie seems nearly sensible.

For now, I’m going to leave things here. It’s a sunny day, the cat’s on deck, oops, no, she just came on off the deck, anyway, I’m calling the code done for today. Time to double check the tests and commit.

Summing up

This has been an interesting ride. We’ve refactored the original hideous method from 48 lines down to 12, with some mostly-sensible helper methods. The new structure handles the special cases explicitly and individually. I think I could maintain this version, even if I didn’t go to the trouble of creating the new classes for new products.

There are still questions about whether it’s doing what Allison wants, and there are still improvements that could be made to this code, some of which we might agree on and some of which not. But my pair partner, Kitty, when asked if she’s OK with the code, does not object.

The experiment with “just refactoring” the messy method went amazingly well, doing what “Read by Refactoring” meant to me. (Thanks again, Dave. I must remember to read what you write, and maybe look for a talk by Arlo.) It felt like taking small and obvious steps, over and over, each one making things a little better. I like where we’ve come to.

Testing has been a concern. What “should” I have done? Well, I’d have felt less uncertain in a few places had I written more tests. I have been assured that if I installed a mutation testing tool and used it, I’d discover leaks in my tests and be motivated to improve them.

Both of those would probably be good things to do. Adding a new tool is a big deal to me: I don’t like doing it unless I’m pairing with someone who already knows how to do it. And writing tests is SO boring. So, often, like this time, I do less than might be ideal.

Does that mean I “should” do more tests? I don’t know. Every day we make decisions about how to spend our limited time, and every now and then we do well to look back, then look forward to how we might want to change in the future.

Right now, I’m happy with the results. Had you been here, I’m sure we’d have done something different, probably something even better. But you weren’t here, and Kitty and I are satisfied.

I’m not sure if this is the end of the Gilded Rose series or not. If it isn’t, it’s pretty close.

Here’s the code:

# 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?
    @item.sell_in = @item.sell_in - 1
    if brie?
      update_brie
    elsif tickets?
      update_ticket
    else
      decrement_quality_down_to_zero
      decrement_quality_down_to_zero if expired?
    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
    increment_quality_up_to_50 if expired?
  end

  def update_ticket
    increment_quality_up_to_50
    if @item.sell_in < 10
      increment_quality_up_to_50
    end
    if @item.sell_in < 5
      increment_quality_up_to_50
    end
    set_quality_zero if expired?
  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(42, brie.quality)
  end

  def test_ticket_does_not_increase_on_day_10_but_after_10
    gr = GildedRose.new([])
    tick = Item.new("Backstage passes to a TAFKAL80ETC concert", sell_in=11, quality=40)
    gr.update_item(tick)
    assert_equal(10, tick.sell_in)
    assert_equal(41, tick.quality)
    gr.update_item(tick)
    assert_equal( 9, tick.sell_in)
    assert_equal(43, tick.quality)
  end

  def test_ticket_does_not_increase_on_day_5_but_after_5
    gr = GildedRose.new([])
    tick = Item.new("Backstage passes to a TAFKAL80ETC concert", sell_in=6, quality=40)
    gr.update_item(tick)
    assert_equal(5, tick.sell_in)
    assert_equal(42, tick.quality)
    gr.update_item(tick)
    assert_equal( 4, tick.sell_in)
    assert_equal(45, tick.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