In the last part, we left GildedRose in a very awkward position.
- It is not a huge mess anymore (Good!)
- It is not a huge conditional mess anymore (Even better!)
- It is not a huge procedural conditional mess anymore...
... or is it?
It looks more OO, but in reality, is a mimicry: I just camouflaged bits of procedural code in objects.
Unfortunately, in my real life:
- lots of the OO code I see looks like what we had in part 1;
- most of the OO code I see looks like what we first created with smaller methods in part 2;
- some well-behaved kids try OO and get to those small objects we created in part 3;
- ... and we are still in the middle of Faux-O. Oh, we did a HUGE step forward, it is just that we are not there yet.
But before continuing our Journey, a quite unrelated side-step, for you to understand why I did a part of what you will see in future code samples.
Good OO looks like Functional Programming
This is a little exaggerated, but not by much. Chaining messages, when done properly, can feel like composing functions.
You may have been told rules like "No more than 2 dots on a line".
I feel this rule stupid. The functional developper inside of my stomach shifts uneasily when hearing such sayings.
The heart is at the right place, I understand what the people mean, but I am not sure it means the same thing for them as it does for me.
When the returned object of each message is different, we should not go above 2. It would mean that we send messages to objects we should not be aware of in the first place.
But what about something like:
str = "a STRING is like a spring!"
str.downcase
.gsub(/(s.ring)/, 'beautiful \1')
.capitalize
# => A beautiful string is like a beautiful spring!
Each method is called on the same object; it does not count as "three dots"!
I want all my classes to behave like String and allow me to chain calls as I see fit.
To that avail, a simple rule: "A public method which does not return a meaningful result should return self"
So you will see some self coming in the mix soon enough 😊
But let us go back to our main subject. What does GildedRose? It responds to 1 message, and that is it.
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
self
end
Let us see it from another angle: GildedRose creates some classes and propagates a message to those classes.
Oh. God. I fear GildedRose is in reality... a kind of Factory.
No, not the gross factories from the Java world: The pure, aesthetic factories promoted by Object Oriented Programming.
Now that we considered GildedRose that way, impossible to go back: we need to refine it and shape it like the factory it is.
From a standard class to a factory
@items is an Array of Item instances. I would prefer it to be an Array of CommonItem. My problem is: which child of CommonItem?
Well, it depends on the item name.
Let just code some helper for that!
(In reality, the code is already there, can you see it?)
private
def class_from(name:)
case name
when "Aged Brie"
AgedBrie
when "Backstage passes to a TAFKAL80ETC concert"
Backstage
when "Sulfuras, Hand of Ragnaros"
Sulfuras
else
CommonItem
end
end
Given an Item, I can now instantiate the right object with a simple class_from(name: item.name).new!
We just can now refactor our code around this idea.
class GildedRose
def initialize(items)
# I do not store Item instances anymore, but instances of CommonItem
@items = items.map { |item| class_from(name: item.name).new(item) }
end
def update_quality
# item is now an instance of CommonItem.
# it can just be sent the "update" message!
@items.each { |item| item.update }
self
end
private
def class_from(name:)
case name
when "Aged Brie"
AgedBrie
when "Backstage passes to a TAFKAL80ETC concert"
Backstage
when "Sulfuras, Hand of Ragnaros"
Sulfuras
else
CommonItem
end
end
end
I do not know for you, but for me, it starts to look really elegant. And simple.
But let us go a little further. The content of #class_from is a real pain. Come on, you know me by now: it is a dreaded conditional! Even worse: it's a configuration conditional 🤢
Luckily for us, we know perfectly well a data structure that can handle a simple key/value configuration: we shall use an Hash.
And here is what the (final?) code looks like:
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
self
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
self
end
end
class Backstage < CommonItem
def update
item.sell_in -= 1
item.quality = 0 and return self 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
self
end
end
class Sulfuras < CommonItem
def update
self
end
end
class GildedRose
ITEM_CLASSES = {
"Aged Brie" => AgedBrie,
"Sulfuras, Hand of Ragnaros" => Sulfuras,
"Backstage passes to a TAFKAL80ETC concert" => Backstage
}
def initialize(items)
@items = items.map { |item| class_from(name: item.name).new(item) }
end
def update_quality
@items.each { |item| item.update }
self
end
private
def class_from(name:)
ITEM_CLASSES[name] || CommonItem
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
Technically speaking, the refactoring is now finished.
- We have a very lean class
- Its responsibility is well defined
- We can add a functionality very easily
The last part will deal with this new functionality, and a sprinkle of refactoring: this time, we will try to accommodate my tiny brain with quality of life changes.
Bye for now :)
Top comments (0)