A dozen gilded roses so far. Amazing how much practice and learning there can be in such a tiny example. Today I plan to move everything back into a single controlling update method, calling simple update methods. Here’s the wrapper.rb file, where all the work gets done:

# 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

My starting point will be to move the conjured item back inside the OriginalWrapper, and then to clean up its overly elaborate update method. At the time I created it, I liked trying to express everything with great generality, but I think it was premature. Getting everything put back together, but cleanly, should give us a look at the big picture and offer new ideas. Heck, we might even be satisfied.

In fairness, we should have been satified long ago. We’re really just exercising here, trying things, honing our skills and senses. The benefit to this program isn’t much, but it’s kind of fun learning in a small space like this.

I start by adding conjured to the “big” update method:

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

Now I need conjured? and update_conjured.

  def conjured?
    return item.name.downcase.include? 'conjured'
  end

I just borrowed this from the match_name function directly. I could have borrowed that function and perhaps I “should” have. At the time this seemed better, perhaps because it’s more atomic and I have in mind “re-discovering” the duplication once this whole thing is in one place. Or, maybe it was just a mistake. Or, maybe it was just right. Who knows? We never know, we make our best decision in the moment and feel free to adjust as we become aware of things.

Now for the update. We have that code in the ConjuredWrapper and we’ll move it in:

  def update_conjured
    @item.sell_in = @item.sell_in - 1
    @item.quality = [0,@item.quality - conjured_degradation].max
  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

I really hate this, but step one is to make it work. Now I can delete the ConjuredWrapper class and change our wrapper selection:

class GildedRose

  def initialize(items)
    @items = items
  end

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

  def update_item(item)
    OriginalWrapper.new(item)
    wrapped_item.update
  end

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

I’m taking a chance here, this was a pretty big edit, perhaps five minutes or more. I’m hoping the tests will run but kind of expect a bit of trouble making sure it’s all wired up correctly. I do expect that once it’s wired up, the tests will run. Here goes … OK, this was a bit too casual:

  def update_item(item)
    OriginalWrapper.new(item)
    wrapped_item.update
  end

I didn’t save to wrapped_item. This is better, perhaps:

  def update_item(item)
    OriginalWrapper.new(item).update
  end

  def conjured?
    return item.name.downcase.include? 'conjured'
  end

That needs to say @item. Once more …

This time all the conjured tests failed. I really should revert now, but I feel sure it’s a simple fix. I think I failed to account for the fact that we’re decrementing sell_in outside updates now:

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

Becomes …

  def update_conjured
    @item.quality = [0,@item.quality - conjured_degradation].max
  end

And we’re green. I started this exercise at 10 AM sharp, and it’s 10:25. That’s a long time to be red, but I probably didn’t even run my first test until … 10:16. So I’ve been red for 9 minutes. That’s still longer than I’d like, and there were 15 minutes of test-free code changing ahead of that.

There was surely a safer way of doing this change. Right now I’m not sure what it would have been, and we’re here now. An interesting exercise would be to revert and do it over but since I don’t see a truly different path, we’ll proceed from here.

Now let’s clean up the OriginalWrapper class a bit. It looks as follows, including a sort of random arrangement of methods:

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
    elsif conjured?
      update_conjured
    else
      decrement_quality_down_to_zero
      decrement_quality_down_to_zero if expired?
    end
  end

  def conjured?
    return @item.name.downcase.include? 'conjured'
  end

  def update_conjured
    @item.quality = [0,@item.quality - conjured_degradation].max
  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 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

There’s a lot there. I hope you just skimmed it. This sticks out for me:

  def update_conjured
    @item.quality = [0,@item.quality - conjured_degradation].max
  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

The whole deal is that conjured items degrade twice as fast as normal items. I was prematurely imagining linking conjured to the default degradation but never carried through with that. Then I folded the expired notion inside the degradation calculation.

I think we would do better, for now, to make the 2 explicit, and do our degradation the same as other cases, then refactor back after it’s simplified and written out longhand. But wait! We can’t just blithely go:

  @item.quality -= 2 if @item.quality > 0

That would fail if quality were odd. That horrid max trick might actually be a decent way to do it. Let me think … I could do this:

  @item.quality -= 1 if @item.quality > 0
  @item.quality -= 1 if @item.quality > 0
  @item.quality -= 1 if @item.quality > 0 if expired?
  @item.quality -= 1 if @item.quality > 0 if expired?

But that’s kind of yucchy, isn’t it? I could tolerate it when each line was there once, but twice? Really not good. How about this:

  def update_conjured
    @item.quality -=2
    @item.quality -=2 if expired?
    @item.quality = [0,@item.quality].max
  end

I rather hate that max usage. Let’s do this instead:

  def update_conjured
    @item.quality -=2
    @item.quality -=2 if expired?
    @item.quality = 0 if @item.quality < 0
  end

I’m not as fond of all these if statements as I might be. There are those who’ll tell you that if statements are indicators of trouble and that your objects aren’t doing their job. They’re probably right.

Quite possibly, instead of our Item having an integer quality, there should be some kind of simple Quality object that is inherently non-negative or something. Right now, I’m just going to treat these if statements as a code smell, but allow them to live for the time being.

Reviewing the class, I note the following patches of code, collected from here and there for your convenience:

    else
      decrement_quality_down_to_zero
      decrement_quality_down_to_zero if expired?
    end

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

  def update_conjured
    @item.quality -=2
    @item.quality -=2 if expired?
    @item.quality = 0 if @item.quality < 0
  end

Let’s give the decrement_quality_down_to_zero method a parameter, beef it up to deal with decrementing by more than one, and then use it in update conjured:

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

  def update_conjured
    decrement_quality_down_to_zero(2)
    decrement_quality_down_to_zero(2) if expired?
  end

  def decrement_quality_down_to_zero(increment)
    @item.quality -= increment
    @item.quality = 0 if @item.quality < 0
  end

Now of course that pair of method calls in the else clause should be extracted:

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

  def update_normal
    decrement_quality_down_to_zero(1)
    decrement_quality_down_to_zero(1) if expired?
  end

Would it be overkill to extract the sell_in line to a method called decrement_sell_in? It would fit the pattern better. We’ll hold off on that. What about this:

  def update
    return if sulfuras?
    @item.sell_in = @item.sell_in - 1
    case
    when brie?     then update_brie
    when tickets?  then update_ticket
    when conjured? then update_conjured
    else update_normal
    end
  end

And, no, I really want to extract that subtract, and also use -=:

  def update
    return if sulfuras?
    decrement_sell_in
    case
      when brie?     then update_brie
      when tickets?  then update_ticket
      when conjured? then update_conjured
      else update_normal
    end
  end

  def decrement_sell_in
    @item.sell_in -= 1
  end

Now we’re really down to tiny methods and tiny changes. I would like to express the double-degradation of conjured items a bit better. Their spec is that that degrade twice as fast as “normal” items. How about this:

class OriginalWrapper

  NormalDegradation = 1

...

  def update_normal
    decrement_quality_down_to_zero(NormalDegradation)
    decrement_quality_down_to_zero(NormalDegradation) if expired?
  end

...

  def update_conjured
    double_degrade = 2*NormalDegradation
    decrement_quality_down_to_zero(double_degrade)
    decrement_quality_down_to_zero(double_degrade) if expired?
  end

It’s 11:25, my tea is nearly empty, and so is my brain. I’ll arrange these methods a bit more sensibly and then call it quits, showing the final code down below. But first, let’s sum up.

Summing up (as promised)

Once we commit ourselves to what is now a case approach to the update, and was an if-then-else approach when we came in, we “should” fold our former approach of specialized classes back into the case structure: it’s just confusing to use both.

We could certainly convert our current case approach back to separate classes, and there still might be some clever use of lambda that would make things better but let’s get real: our 40-some line hard to read update method is now down to ten lines and they are, in my view, pretty clear:

  def update
    return if sulfuras?
    decrement_sell_in
    case
      when brie?     then update_brie
      when tickets?  then update_ticket
      when conjured? then update_conjured
      else update_normal
    end
  end

If we want to know more, we can look at the detailed methods, and they are all quite small and simple. We do have more lines now, something like 85 in our wrapper class. We could, and perhaps would, fold our update and its helper methods back down into the GildedRose class, which would save us some lines but might well make GildedRose more messy than it needs to be.

We have decent separation of the various types of item, but we certainly haven’t met the standard of a class per type. It’s easy to argue that each type of item deserves its own data type (class), but the net effect is going to be more lines of code and more duplication, unless we go to some kind of subclassing, which is pretty deep in the bag of tricks.

For now, I’m pretty happy with what we have. I’ll be interested to hear what you think.

See you next time. Shall we hope that a dozen roses is enough?

# 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 update_item(item)
    OriginalWrapper.new(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 OriginalWrapper

  NormalDegradation = 1

  def initialize(item)
    @item = item
  end

  def update
    return if sulfuras?
    decrement_sell_in
    case
      when brie?     then update_brie
      when ticket?  then update_ticket
      when conjured? then update_conjured
      else update_normal
    end
  end

  def update_normal
    decrement_quality_down_to_zero(NormalDegradation)
    decrement_quality_down_to_zero(NormalDegradation) if expired?
  end

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

  def update_brie
    increment_quality_up_to_50
    increment_quality_up_to_50 if expired?
  end

  def conjured?
    return @item.name.downcase.include? 'conjured'
  end

  def update_conjured
    double_degrade = 2*NormalDegradation
    decrement_quality_down_to_zero(double_degrade)
    decrement_quality_down_to_zero(double_degrade) if expired?
  end

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

  ## sulfurus does not update

  def ticket?
    @item.name == "Backstage passes to a TAFKAL80ETC concert"
  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

  ## utility methods

  def decrement_quality_down_to_zero(increment)
    @item.quality -= increment
    @item.quality = 0 if @item.quality < 0
  end

  def decrement_sell_in
    @item.sell_in -= 1
  end

  def expired?
    @item.sell_in < 0
  end

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

  def set_quality_zero
    @item.quality = 0
  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)
  end
end