Dave Nicolette mentioned on Twitter that he was wondering whether a Mob Programming Gilded Rose kata, using Arlo Belshee’s ‘Read by Refactoring’ would make a good video. That gave me an idea.

It would be a good idea for Dave to do that, and if he does, I’ll put a link here. I confess I’ve never seen Arlo talk about “Read by Refactoring” but I’m hoping the name says it all, and in any case it increased my desire to refactor this hideous thing even though, from a business perspective, we probably shouldn’t.

My purpose is to have fun, of course. But in a real legacy situation, we might not be as lucky as we’ve been with Gilded Rose, where a simple change of approach sets us up for the future without really addressing the convoluted legacy code. So let’s pretend that we’ve done what we could outside, and really need to dive in and understand this baby. We’ll do it by extracting and naming things.

Anyway, here’s Wond … no, here’s our update 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
    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.quality < 50
          item.quality = item.quality + 1
        end
      elsif item.name == "Backstage passes to a TAFKAL80ETC concert"
        item.quality = 0
      else
        if item.quality > 0
          if item.name != "Sulfuras, Hand of Ragnaros"
            item.quality = item.quality - 1
          end
        end
      end
    end 
  end

That code is so ugly I almost can’t eat my morning banana. Let’s see what we can extract that might help. I think I’ll go really crazy on this, extracting even single lines if I think it can help understanding.

Take a look at this and try to see something that deserves extraction and naming. It’s really daunting because the code is so horrid. Really we just need to start somewhere and keep going. I’m choosing this line:

    if item.sell_in < 0

What does that mean? I think it means “item has expired”. I change it this way:

    if item_has_expired(item)
      if item.name == "Aged Brie"
        if item.quality < 50
          item.quality = item.quality + 1
        end
      elsif item.name == "Backstage passes to a TAFKAL80ETC concert"
        item.quality = 0
      else
        if item.quality > 0
          if item.name != "Sulfuras, Hand of Ragnaros"
            item.quality = item.quality - 1
          end
        end
      end
    end 
...

  def item_has_expired(item)
    return item.sell_in < 0
  end

Oddly, that’s a bit better. Let’s just keep looking randomly. How about this, under “Aged Brie”?

        if item.quality < 50
          item.quality = item.quality + 1
        end

Well, that’s “increment quality up to 50”, isn’t it? Now we have:

    if item_has_expired(item)
      if item.name == "Aged Brie"
        increment_quality_up_to_50(item)
      elsif item.name == "Backstage passes to a TAFKAL80ETC concert"
        item.quality = 0
      else
        if item.quality > 0
          if item.name != "Sulfuras, Hand of Ragnaros"
            item.quality = item.quality - 1
          end
        end
      end
    end 

...

  def increment_quality_up_to_50(item)
    if item.quality < 50
      item.quality = item.quality + 1
    end
  end

This bit …

      elsif item.name == "Backstage passes to a TAFKAL80ETC concert"
        item.quality = 0

… becomes …

      elsif tickets?(item)
        set_quality_zero(item)
...

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

  def set_quality_zero(item)
    item.quality = 0
  end

That time I remembered the Ruby convention that methods returning truthy values end in question mark. I’ll change my other method name and show you what we’ve got:

  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
    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 expired?(item)
      if item.name == "Aged Brie"
        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

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

  def set_quality_zero(item)
    item.quality = 0
  end

  def expired?(item)
    item.sell_in < 0
  end

  def increment_quality_up_to_50(item)
    if item.quality < 50
      item.quality = item.quality + 1
    end
  end

Then I refactored increment_quality_up_to_50:

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

I should mention that after each of these changes I’m running the tests, both the microtests and the text test. I confess I’m feeling a bit less than perfectly confident, because I’ve not inspected the text test to be sure it covers all the edge cases, and I know our microtests don’t.

If this were less of an exercise and more live, I’d be working to beef up the tests as needed. Most of these changes have been “obviously right”, except perhaps this last one, where I changed if quality < 50 to unless quality >= 50. That’s the one that really stabbed at me a bit. Still pretty clearly right but I’d like to see a test.

OK, you shamed me into it. Here’s the relevant code:

    if expired?(item)
      if item.name == "Aged Brie"
        increment_quality_up_to_50(item)

This will increment Brie’s quality after expiration up to and including 50. Let’s just write the darn microtest. How hard can it be?

  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

Sure enough, that runs. I hope you feel better now. I know I do.

What else can we do? I still hate this code a lot. Let’s work on this first bit:

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

Those two main clauses are independent, the not-Brie, not-ticket branch and the else branch. So the else branch is is_Brie or is_ticket. Are you with me? So that whole bit between the arrows is update Brie or ticket. Extract it.

  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
    else
      update_brie_or_ticket(item)
    end
    if item.name != "Sulfuras, Hand of Ragnaros"
      item.sell_in = item.sell_in - 1
    end
    if expired?(item)
      if item.name == "Aged Brie"
        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

  def update_brie_or_ticket(item)
    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

Tests still run. Notice those two < 50 bits? Those are increment_quality_up_to_50 again. Replace.

  def update_brie_or_ticket(item)
    if item.quality < 50
      item.quality = item.quality + 1
      if item.name == "Backstage passes to a TAFKAL80ETC concert"
        if item.sell_in < 11
          increment_quality_up_to_50(item)
        end
        if item.sell_in < 6
          increment_quality_up_to_50(item)
        end
      end
    end
  end

Oh, and there’s what should be a call to tickets?. Replace it.

  def update_brie_or_ticket(item)
    if item.quality < 50
      item.quality = item.quality + 1
      if tickets?(item)
        if item.sell_in < 11
          increment_quality_up_to_50(item)
        end
        if item.sell_in < 6
          increment_quality_up_to_50(item)
        end
      end
    end
  end

We missed an increment_quality_up_to_50 possibility. That’s going to look interesting once we do it:


  def update_brie_or_ticket(item)
    if item.quality < 50
      increment_quality_up_to_50(item)
      if tickets?(item)
        if item.sell_in < 11
          increment_quality_up_to_50(item)
        end
        if item.sell_in < 6
          increment_quality_up_to_50(item)
        end
      end
    end
  end

We have a redundant check for < 50 in there now. Since all the increments are checked, we can remove that outer if:

  def update_brie_or_ticket(item)
    increment_quality_up_to_50(item)
    if tickets?(item)
      if item.sell_in < 11
        increment_quality_up_to_50(item)
      end
      if item.sell_in < 6
        increment_quality_up_to_50(item)
      end
    end
  end

Tests are still running fine. As I look at this, I rather wish I could get rid of the or, and have a separate update for Brie and for tickets. That’s controlled here:

    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
    else
      update_brie_or_ticket(item)
    end

We can do this trivially:

    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 item.name == "Aged Brie"
      update_brie_or_ticket(item)
    elsif tickets?(item)
      update_brie_or_ticket(item)
    end

Now I can create the two separate updates (and build a new checker: brie?)

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

  def update_brie(item)
    increment_quality_up_to_50(item)
  end

  def update_ticket(item)
    increment_quality_up_to_50(item)
    if item.sell_in < 11
      increment_quality_up_to_50(item)
    end
    if item.sell_in < 6
      increment_quality_up_to_50(item)
    end
  end

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

  def brie?(item)
    item.name == "Aged Brie"
  end

OK. I started at 8:50 this morning. It’s 10:06 as I write this sentence. Less than an hour and a half and we’ve actually begun to get this under control.

I’m going to call it a day and push this article after summing up.

Impressions

I started with a random idea (thanks, Dave!) of noticing bits of code that had meaning, and extracting methods that expressed that meaning. The result, so far, is interesting. We have more lines of code: git sees 52 insertions and 23 deletions. The insertions are all at least three lines, I reckon.

But by my lights, the main update method is becoming more clear, because it expresses what it’s doing, rather than just doing it. Some folks don’t like the little tiny methods with allegedly readable names, but I do. It’s probably my Smalltalk background that has made me comfortable with that.

Preferences aside, our update method is starting to tell a story explicitly rather than just being a mass of weird expressions. I like that.

We’ll continue this next time. Right now I want a bowl of Graham Crunch.

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 = @item
    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
    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 
  end

  def update_brie(item)
    increment_quality_up_to_50(item)
  end

  def update_ticket(item)
    increment_quality_up_to_50(item)
    if item.sell_in < 11
      increment_quality_up_to_50(item)
    end
    if item.sell_in < 6
      increment_quality_up_to_50(item)
    end
  end

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

  def brie?(item)
    item.name == "Aged Brie"
  end

  def set_quality_zero(item)
    item.quality = 0
  end

  def expired?(item)
    item.sell_in < 0
  end

  def increment_quality_up_to_50(item)
      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

end