This blog entry is the second part of the GildedRose kata that I had taken up several months ago. It’s a great kata to help study and consider clean design principles; it can be found here. Whatever your level you will definitely get something out of it. I previously did a first attempt at this kata, and made a blog entry on it.
Funnily enough, this kata is all about dealing with legacy code. In a blog entry to follow this one, I’m going to give a quick introduction to a tool that can help us understand spaghetti code (Flog).
Looking at the initial iteration of code I had written for this kata, I understood that the tests I had written were not ideal. Mainly because the instantiation of special objects was actually made from within the test; this is great if we want to unit test the classes of these special objects, but not ideal if we actually want to think about what the GildedRose class is actually useful for.
How we left things previously.
All special items are attributed a class of their own, which inherits from the Item class, although it also has new functionality.
Here is a reminder of what we got to:
class GildedRose
include UpdateOperators
def initialize(items)
@items = items
end
attr_accessor :items
def update_quality
items.each do |item|
item.respond_to?(:update_self) ? item.update_self : default_update(item)
end
end
end
# /*/ Parent class
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
# /*/ Subclasses
class Sulfuras < Item
def quality
@quality = 80
end
def update_self
self.quality
self.sell_in
end
end
class BackstagePass < Item
include UpdateOperators
def update_self
if self.sell_in <= 0
self.quality = 0
elsif self.sell_in > 10
increment_quality
decrease_sell_in
elsif self.sell_in <= 5
increment_quality(3)
decrease_sell_in
elsif (self.sell_in <= 10)
increment_quality(2)
decrease_sell_in
end
end
end
class Brie < Item
include UpdateOperators
def update_self
if still_sellable(self)
increment_quality
decrease_sell_in
else
increment_quality(2) and decrease_sell_in unless max_quality
end
end
end
All special items hold an instance method for updating themselves.
It means that when an item is of type Brie, it knows how to process its own update, as that functionality is an intrinsic part of its identity.
As a reminder, we are not allowed to touch the Item class, which looks like this:
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
And here is an example of tests for a special item:
# Normal item test example:
it "does not change the name" do
items = [Item.new("foo", 0, 0)]
GildedRose.new(items).update_quality()
expect(items[0].name).to eq "foo"
end
# Special item test example:
it "Brie increases in quality as sell-by-date approaches" do
items = [Brie.new("Aged Brie", 8, 1)]
expect(items[0].quality).to eq 1
expect(items[0].sell_in).to eq 8
GildedRose.new(items).update_quality()
expect(items[0].quality).to eq 2
expect(items[0].sell_in).to eq 7
GildedRose.new(items).update_quality()
expect(items[0].sell_in).to eq 6
expect(items[0].quality).to eq 3
end
As unit tests, these work well, we know we can create an instance of Brie and that this Brie object will have the necessary behaviour which knows how to update itself. Though in order for the GildedRose class to actually be worth existing, it should have a special function.
Changing the tests
What the class should really do is process an item that is inputted, and determine which special item class should be called to apply the relevant update method. The objects inputted into the GildedRose class will all be of type Item, but from these items, the GildedRose should be able to tell us what particular type of item we’re dealing with (this make sense in regards the fact that Item is the parent class, and all special items inheriting children of Item). GildedRose is a factory of objects, it receives items, but can then differentiate between different special items. We should therefore have a dictionary/catalogue of sorts, wherein we can store our special items. We’ll see why this is a good idea momentarily.
Changing the tests, we get all of the failures we expect:
it "Brie increases in quality as sell-by-date approaches" do
items = [Item.new("Aged Brie", 8, 1)]
expect(items[0].quality).to eq 1
expect(items[0].sell_in).to eq 8
GildedRose.new(items).update_quality()
expect(items[0].quality).to eq 2
expect(items[0].sell_in).to eq 7
GildedRose.new(items).update_quality()
expect(items[0].sell_in).to eq 6
expect(items[0].quality).to eq 3
end
I personally like this approach of considering that everything that passes through is an Item first, and then potentially a special item if it’s found within the dictionary, as it really demonstrates well the hierarchy of inheritance: Item first (parent class), then possibly a special item (child class). The behaviour that we are expecting here of the Item object is not behaviour that we have programmed into the Item class (remember, we’re not allowed to change any code inside that class). What we can do, therefore is check the name of the inputted item, and see whether it matches any names belonging to special items. To fast forward I’m skipping test, and putting the special items straight into a hash.
The problem with the previous iteration of this problem is principally with the tests, in that we are creating an instance of the special objects from within the tests, which is counter to what we should actually be doing.
It also make us consider whether the GildedRose class actually has any particular usage, since all of the logic for updating quality of special items is given to the special classes themselves.
The answer to this is to consider the GildedRose as a factory of objects. When an item, or multiple items, pass through the GildedRose, this “processing” class should decide, or designate which TYPE of item they are: an item is either a special item, or a default item. In this way, we can figure that the GildedRose class is actually a factory for items, and it should be able to recognise an object that is passed in and call it along with its behaviours….
At this point, we already know that we have multiple special items, so why not put them inside a dictionary or catalogue of sorts. The immediate benefit of this is that if down the line we were to have a new special item to consider, we can simply add the item into this dictionary without modifying any of the business logic immediately inside of the code.
Therein, the responsibility of the GildedRose is only to consider incoming objects and check whether they are “special items”, against the catalogue. This is the open/close principle, that says it’s best to “arrange code so that adding new behaviour does not require that you edit existing code.”
Before doing this, we make use of the tests and replace all instantiation of special items with Item, ensuring that all items, aswell as having the characteristics of special items are also items by default. The special item classes will allow us to make the special update functionality.
But the items are only ever inputted as default items.
Switching the functionality of the GildedRose class
After changing the tests and watching them fail, we can refactor the code of the GildedRose from this:
class GildedRose
include UpdateOperators
def initialize(items)
@items = items
end
attr_accessor :items
def update_quality
items.each do |item|
item.respond_to?(:update_self) ? item.update_self : default_update(item)
end
end
end
to this:
class GildedRose
include UpdateOperators
ITEM_CLASSES = {
"Aged Brie" => Brie,
"Sulfuras, Hand of Ragnaros" => Sulfuras,
"Backstage passes to a TAFKAL80ETC concert" => BackstagePass,
"Conjured Item" => ConjuredItem,
}
def initialize(items)
@items = items
end
attr_accessor :items
def update_quality
@items.each do |item|
special_item = ITEM_CLASSES[item.name]&.new(item.name, item.sell_in, item.quality)
if special_item
special_item.update_self
item.quality = special_item.quality
item.sell_in = special_item.sell_in
else
default_update(item)
end
end
end
end
It looks longer, but now the class does what it should actually be doing, checking the incoming items against a catalogue that can easily be updated.
You may notice that within the conditional of the method we assign the “item.quality” and “item.sell_in” to that we get from the special item, this is because we are actually considering every inputted object to first be an item, and potentially a special item also.
We could even make an extra abstraction by taking the asignement of the special item values in the update_quality method aimed at overwriting the Item values, like this:
def update_quality
@items.each do |item|
special_item = ITEM_CLASSES[item.name]&.new(item.name, item.sell_in, item.quality)
if special_item
special_item.update_self
item.quality = special_item.quality
item.sell_in = special_item.sell_in
else
default_update(item)
end
end
end
to this:
def synthesize_values_of(item, special_item)
item.quality = special_item.quality
item.sell_in = special_item.sell_in
end
def update_quality
@items.each do |item|
special_item = ITEM_CLASSES[item.name]&.new(item.name, item.sell_in, item.quality)
if special_item
special_item.update_self
synthesize_values_of(item, special_item)
else
default_update(item)
end
end
end
We can then hide away the synthesize_values_of method into theUpdateOperators module.
Limitations
It might be an idea to unpack some of the things inside UpdateOperators module, as currently it seems to be getting pretty cumbersome. But for now I think this is a pretty satisfactory solution, though I will be curious to check in again in several months, to perhaps see if I a new iteration could be even better.
The final result of the code is over here.