DEV Community

loading...
Cover image for A Walk Through the Gilded Rose Kata — Pt 3: From Faux-O to OO

A Walk Through the Gilded Rose Kata — Pt 3: From Faux-O to OO

Lomig
I love discovering new programming languages. Jack of all trades, master of none. Ruby dev :)
Updated on ・6 min read

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
Enter fullscreen mode Exit fullscreen mode

_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
Enter fullscreen mode Exit fullscreen mode

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
Enter fullscreen mode Exit fullscreen mode

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

Everyone gets a class

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
Enter fullscreen mode Exit fullscreen mode

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.

  • AgedBrie sees 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 CommonItem could 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
Enter fullscreen mode Exit fullscreen mode

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
Enter fullscreen mode Exit fullscreen mode

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!

Discussion (0)