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