I left you in the last post with a GildedRose class which I find — arguably — in a much healthier state than when we first met. But I still feel that something can be done.
The same way I recognized a pattern calling for case ... when ... end, there is another one that screams at us right here.
Where there is a message...
An old teacher of mine had a saying (well I imagine that any real OO expert has a saying of the sort, but let me imagine my old teacher as a wise hermit sharing his knowledge to the worthy in a cryptic way, please): "Prefix means message".
Well, I am French, so it's a little awkward as we tend not to order words as others; here in English we should say "Suffix means message".
Let's simplify our code to see the obvious:
def aged_brie_update(item)
end
def backstage_update(item)
end
def sulfuras_update(item)
end
def common_update(item)
end
_update is a message; a message asks for an object to be called upon.
...there is a class!
People tend to think about classes as those well-thought pieces of creativity which require time, crafting, and the right model.
It's true for some of them, but not all of them. We should prefer small objects exchanging messages compared to a big class that indeed mimics a Real Life Object, but does too many things and knows too many things; those small objects often just emerge from your code and your design.
#update is a message, sent to a class, I will let this class design itself.
I will go from:
def aged_brie_update(item)
item.sell_in -= 1
item.quality +=1
item.quality +=1 if item.sell_in < 0
item.quality = 50 if item.quality > 50
end
to:
class AgedBrie
attr_reader :item
def initialize(item)
@item = item
end
def update
item.sell_in -= 1
item.quality +=1
item.quality +=1 if item.sell_in < 0
item.quality = 50 if item.quality > 50
end
end
Nothing fancy, now that we can see it.
Obviously, I shall change its call in our #update_quality method
- from:
aged_brie_update(item) - to:
AgedBrie.new(item).update
Applying the same recipe everywhere
This should be trivial, now.
There is a special case, though: Sulfuras and its empty method. You may have asked yourself why I kept it for so long.
I do not care if a method is empty. Empty is good. Doing nothing is good. I did not follow a typical journey as a developer; in college, my first approach to coding was through math and functional languages. There, we have things like "identity functions" that returns what is passed to them without any change. Isolated, it is profoundly useless, but thrown in other concepts, it becomes one of the most powerful concept there is.
Nothing is something. I like nothing. If I can suffer the case ... when in this code, I am sure you can bear empty methods. Especially as now it sort of pays off: I am creating classes representing types of items. As I kept the method, I kept the concept of sending a message to Sulfuras, and I can see now that it's a type of items as any of the others. Throwing the method as useless earlier would have kept this knowledge from me.
Refactoring tips
Don't be too keen on removing duplication of code, empty methods, silly things... too early. You may miss the right abstraction for your algorithm, and create unnecessary pain for your future self.
In any case, oops, I did it again: I am making my class longer and more complex that before my refactoring. As before, I will not care that much. I respect what my old professor said from his grotto.
Here is the code in its current state:
class GildedRose
def initialize(items)
@items = items
end
def update_quality
@items.each do |item|
case item.name
when "Aged Brie"
AgedBrie.new(item).update
when "Backstage passes to a TAFKAL80ETC concert"
Backstage.new(item).update
when "Sulfuras, Hand of Ragnaros"
Sulfuras.new(item).update
else
Common.new(item).update
end
end
end
class Common
attr_reader :item
def initialize(item)
@item = item
end
def update
item.sell_in -= 1
item.quality -= 1
item.quality -= 1 if item.sell_in < 0
item.quality = 0 if item.quality < 0
end
end
class AgedBrie
attr_reader :item
def initialize(item)
@item = item
end
def update
item.sell_in -= 1
item.quality +=1
item.quality +=1 if item.sell_in < 0
item.quality = 50 if item.quality > 50
end
end
class Backstage
attr_reader :item
def initialize(item)
@item = item
end
def update
item.sell_in -= 1
item.quality += 1
item.quality += 1 if item.sell_in < 10
item.quality += 1 if item.sell_in < 5
item.quality = 0 if item.sell_in < 0
item.quality = 50 if item.quality > 50
end
end
class Sulfuras
attr_reader :item
def initialize(item)
@item = item
end
def update; end
end
end
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
We can see some repeating patterns though as our newly created classes cover:
- the same concept (a kind of trade good)
- with the same signature (accept an
Item, respond to#update) - that can be substituted without causing an error
You guessed it, it is time for inheritance!
And the not so special case becomes a parent
To keep my sanity, I decided that the parent object should be my CommonItem. In reality, any of those new classes could fit. That is exactly the reason why I am not that weary to use such a dangerous (and hated, it seems) concept as inheritance — please refer to my bullet points above.
I also decided that if all those objects should be siblings, they should try and behave the same way as much as possible.
An item, it cannot have a negative quality, nor have a quality above 50. I had special cases for that when required, but never cared to streamline the process.
-
AgedBriesees its quality go up, so I never cared it could not go below 0. But it's true anyway. It's true for every items. - In the same way, I did not care that a
CommonItemcould never have its quality above 50, but it's a limit for every items nonetheless.
I implemented a simple bound helper function on which every of those items can rely:
def limit_quality
return item.quality = 0 if item.quality < 0
item.quality = 50 if item.quality > 50
end
A few moments later, once everything inherits from its rightful parent, we get this magnificent result:
class GildedRose
def initialize(items)
@items = items
end
def update_quality
@items.each do |item|
case item.name
when "Aged Brie"
AgedBrie.new(item).update
when "Backstage passes to a TAFKAL80ETC concert"
Backstage.new(item).update
when "Sulfuras, Hand of Ragnaros"
Sulfuras.new(item).update
else
CommonItem.new(item).update
end
end
end
class CommonItem
attr_reader :item
def initialize(item)
@item = item
end
def update
item.sell_in -= 1
item.quality -= 1
item.quality -= 1 if item.sell_in < 0
limit_quality
end
private
def limit_quality
return item.quality = 0 if item.quality < 0
item.quality = 50 if item.quality > 50
end
end
class AgedBrie < CommonItem
def update
item.sell_in -= 1
item.quality +=1
item.quality +=1 if item.sell_in < 0
limit_quality
end
end
class Backstage < CommonItem
def update
item.sell_in -= 1
return item.quality = 0 if item.sell_in < 0
item.quality += 1
item.quality += 1 if item.sell_in < 10
item.quality += 1 if item.sell_in < 5
limit_quality
end
end
class Sulfuras < CommonItem
def update; end
end
end
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
We are nearly down the road, and tomorrow, we will see how to finally use those classes to our advantage, and justify their existence in our grand refactoring scheme.
Because, yup, we had a goal, and it is still on the radar: make this GildedRose class easier to understand and maintain, and let us add a functionality.
Have a nice day!
Top comments (0)