It’s noon on Tuesday. I’ve whiled away the morning. Let’s try to make a bit more progress on this code.

Many of my pals are saying just to subclass the Item and be done with it, on the grounds that it doesn’t change the Item class. Then they want to populate the items collection with the subclasses. I think the latter is a clear violation of the goblin’s concerns and possibly the former is.

More to the point, if we require ourselves to leave those things alone, it makes the problem a bit more difficult, and since this is a learning exercise, that’s probably a good thing.

I do have an alternative in mind. We have this loop at the center of our program:

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

I propose to produce a series of wrapper classes that can be used roughly like this:

  def update_quality()
    @items.each do |item|
      wrapper_class = appropriateWrapperClass(item)
      wrapped_item = wrapper_class.new(item)
      wrapped_item.update
    end
  end

We get the appropriate wrapper for our item (by checking its name, of course), then we tell that wrapper to do the necessary updating. Each wrapper will have just the precise code we need to do the job for that particular kind of item.

The trick will be to be sure to put enough testing in place to be confident that we’ve not broken anything.

I think we can start rather simply, however.

Well, I wasn’t quite right about how simple it would be. But it was straightforward, and it goes like this. Here’s the complete Gilded Rose class now:

# 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 appropriateWrapperClass(item)
    return OriginalWrapper
  end

  def update_item(item)
    wrapper_class = appropriateWrapperClass(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

I put my wrapper logic into update_item rather than inside the update loop. For now, that makes more sense, I think, because otherwise update_quality would have control structure plus computation in it, and that violates the Composed Method pattern.

The appropriateWrapper method just returns OriginalWrapper, and I’ve moved a bunch of stuff in there directly:

# wrapper classes

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

  def conjured_degradation(item)
    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_conjured_item(item)
    item.sell_in = item.sell_in - 1
    item.quality = [0,item.quality - conjured_degradation(item)].max
  end
  
  def match_name(item,name)
    return item.name.downcase.include? name.downcase
  end

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

end

Basically that’s just the old update method, tweaked to compile inside OriginalWrapper, plus the methods it calls. The conjured item is left in there for now, but of course we’ll be pulling that out, along with the other item types, as we move forward.

My unit tests run, as does the text test.

We’re green. Let’s commit these files.

Discussion

So, is it clear what we’re up to here? We’re going to create a series of simple wrapper classes that each know how to handle one type of shop Item. As we loop to update the items, we’ll select the appropriate wrapper, instantiate it with that base item as its member variable, then tell it to update. The wrapper compute the appropriate values and stuff them back into the base item. We get the advantage of polymorphism, but we don’t have to worry about the goblin and his hangups.

Our work from now forward, for a while, will be to beef up the unit or text test a bit to support converting some chosen item type to the new scheme, build the new wrapper class, cause it to be selected by appropriateWrapperClass, and plug it into the system. Having done that, we can either delete some code from OriginalWrapper, or just wait until we’re done and delete the whole thing. The latter is probably safer, the former is probably more gratifying, as we slash that horrible method, making it suffer as it truly deserves, until its cries of anguish echo …

Excuse me, I got a bit emotional. Anyway, soon enough we’ll be finished with OriginalWrapper.

Options

There are other ways to have done this. Perhaps the most modern and stylish would have been to create a bunch of neat lambda functions for the update, and select the right one and apply it. I think that would be cooler, but it would also be a bit trickier for me, because while I use lambdas a lot in Lua, I’ve not used them at all in Ruby (other than in the various looping statements of course). So if I decide to do that at all, I’ll do it after the more object-oriented style refactoring is done.

There are other cute ways to have gone as well. Because I’m the one here at the keyboard, I picked the one that was closest to the program’s existing design, and simple enough that I could probably do it without a pair or mob. And, since it seems to be working … possibly it was a good enough decision.

Summing up

This whole exercise, including writing the article, took less than an hour. Our system design is a bit better, and we’re green. This is an example of how we can do a “large” refactoring in small steps, as we get the opportunity.

Next time, we’ll move our new conjured item method into a wrapper class, which will set the code up for new shop items should any arise.

See you then!