Introduction

I recently did a pairing session with two experienced programmers. As an exercise I was given the task of looking a piece of code and figuring out what to do within an hour or so.

I am writing this blog post because although I implicitly espoused some ideas of the single responsibility principle(SRP), I don’t believe that in the time I was given my answer fully embodied the main tenets of SOLID, along with some of the basic principles that OOP leverages.

The term SOLID refers to the five fundamental principles to write better maintainble code. It can be used to evaluate the health of a codebase and architectural approach.

This post will explore the first one: Single Responsibility Priniciple, by looking at a famous kata, ‘The Gilded Rose’. The idea of SRP is to keep classes small and maintanable. It also makes them easier to understand.

Additionally, I want to explore something that came up in the discussion of the pairing session. One of the programmers brought up the idea of using classes to break up the given code (that will follow).

Had I had more time, I might have liked to leverage statefulness of classes, in order to adher to the separation of concerns design principle, but more obviously to assign the responsibility of self to the respective classes of certain objects.

After all, the idea of the single responsibility principle is that class should only be given one reason to change, or one responsibility, so that in itself should drive us to adhering to separation of concerns, which in turn allows us to make things modular, and easily extendible in future iterations of the same code-base.

The idea of SRP is in my opinion similar in essence to the philosophy of Unix tools:

“A tool in Unix should do one thing, and one thing well.”

Ok, so now to the code.

The following code snippet is not safe for sensitive audiences, viewer discretion is advised:

class GildedRose

  def initialize(items)
    @items = items
  end

  def update_quality()
    @items.each do |item|
      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
          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.name != "Backstage passes to a TAFKAL80ETC concert"
            if item.quality > 0
              if item.name != "Sulfuras, Hand of Ragnaros"
                item.quality = item.quality - 1
              end
            end
          else
            item.quality = item.quality - item.quality
          end
        else
          if item.quality < 50
            item.quality = item.quality + 1
          end
        end
      end
    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

Instructions

The instructions for this kata are in the README of the code I have on Github here.

What we are allowed to do with the code basically boils down to this:

• We can’t touch the Item class which (off limits)
• We can’t touch the items (off limits)

Asides from these, we can do anything we like, so long as the following criteria holds:

• All items have a SellIn value which denotes the number of days we have to sell the item
• All items have a Quality value which denotes how valuable the item is
• At the end of each day our system lowers both values for every item

Pretty simple, right? Well this is where it gets interesting:

• Once the sell by date has passed, Quality degrades twice as fast
• The Quality of an item is never negative
• “Aged Brie” actually increases in Quality the older it gets
• The Quality of an item is never more than 50
• “Sulfuras”, being a legendary item, never has to be sold or decreases in Quality
• “Backstage passes”, like aged brie, increases in Quality as its SellIn value approaches;

Quality increases by 2 when there are 10 days or less and by 3 when there are 5 days or less but Quality drops to 0 after the concert

We have recently signed a supplier of conjured items. This requires an update to our system:

• “Conjured” items degrade in Quality twice as fast as normal items

Testing

When looking at the code initially I gasped, because of the heavy nesting, and I immediately considered how we could begin to extract some of the unsavoury code in place.

It turned out I was jumping a step ahead, although I should have known better. At Makers, we were drilled with the importance of TDD, and its a process I still try to espouse since finishing. Here however, we write the tests backwards, since the code base in its original form only shows one test. So we should support the rest of the functionality with specs that will make refactoring less risky.

Since the one test that is given passes (at least after a typo fix), and after writing out all the tests to support the given criteria above, we could say that the code is stuck in the green stage of the red-green-refactor cyle, which is the root cause of the heavy nesting. It looks like someone has hacked away in order to make the tests pass, and in the process created a monolith of unscalable legacy code. And it is our job to fix this. An explanation of “Red Green Refactor” is given under “4.Ruby Test Driven Development” in my documentation compilation.

…

For the sake of brevity, lets assume that a test suite has been written to support the code in question. To see the test-suite you can jump to the solution to this kata that I have on Github here.

Refactoring

Now we can jump to how we would refactor this.

In my initial impression I began working on cleaning and DRYing the code, I extracted the strings out into a constant, like so:

class GildedRose

  PRODUCT = {
    cheese: "Aged Brie" ,
    ticket: "Backstage passes to a TAFKAL80ETC concert",
    sulfuras: "Sulfuras, Hand of Ragnaros"
  }
  def initialize(items)
    @items = items
  end

  def update_quality()
    @items.each do |item|
      if item.name != PRODUCT[:cheese] and item.name != PRODUCT[:ticket]
        if item.quality > 0
          if item.name != PRODUCT[:sulfuras]
            item.quality = item.quality - 1
          end
        end
      else

#    [...]

What I was trying to improve here were the magic strings inside the long conditional. As mentioned above, I was jumping ahead of myself.

Doing this ultimately raises the question around the overall design of a possible solution; this is something I discovered later on in the process. The question it raises is the following: is it the responsibility of the GildedRose class to know what sorts of objects/items can be passed through? Or is that something that the user would be responsible for?

The trouble is this: if we create a constant to store the names of special objects, and in turn use that hash constant to call an instance of those special objects respectively, then we might lose the ability to iterate over the same instance of a called object mutliple times, and have that object remember its new state after an update.

So I did the following:

• After writing a test suite, I removed all the code from inside the conditional in order to first focus on a “normal” item

• I then paused all the tests for objects that had special names, so that I could proceed incrementally - similarly to a TDD process (refer to my “Docs Compilation/4.Ruby/Test Driven Development”)

• Only the tests refering to a “normal” item would pass

• I then extracted the update logic into helper methods, and put these inside a module, an example is decrementing item.quality

So the first step looked like this:

class GildedRose

  def initialize(items)
    @items = items
  end

   def decrease_quality(item)
     item.quality -=  1
   end

  def update_quality()
    @items.each do |item|
      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"
            decrease_quality(item)
          end
        end
      else
# [...]

But it still does not really deal with the problem we’ll face with the different types of items. I realised that I could completely take out those magic strings completely for now.

The module for storing update methods (which are really helper method) looks like this:

module UpdateOperators

  DEFAULT_INCREMENT = 1

  def decrease_quality(item, quality_increase = DEFAULT_INCREMENT)
    item.quality -= (DEFAULT_INCREMENT * quality_increase) 
    item.quality = [item.quality, 0].max
  end

  def decrease_sell_in(item = self)
    item.sell_in -= 1
  end

  def expired?(item)
    item.quality == 0
  end

  def still_sellable(item)
    item.sell_in > 0
  end

  def default_update(item, quality_increase = DEFAULT_INCREMENT)

    return if expired?(item)
    if still_sellable(item)
      decrease_quality(item, quality_increase);
      decrease_sell_in(item)
    else
      decrease_quality(item, DEFAULT_INCREMENT * 2)
    end
  end

end

• For the sake of brevity I’ve jumped a few steps, but already we have all the basic update logic stored in one place, and it now sort of follows the open close principle, open for extension, but closed for modification; and we have achieved this by making our methods as small as possible.

• We also choose idiomatic naming for the methods, so that when I come back to this later, or if someone else were to look at the code, they would know precisely what each method’s functionality is, simply by looking at the name.

• We have a constant set at the top. This means that if one day, for whatever reason, the quality of a product decreases at a different speed generally, then we are able to change that default increment value and in turn affect all of the items instantaneously (we would however have to alter the tests to accomodate such a change, so perhaps this does not really adher to the O/C priniciple just yet).

• What we achieve in having this module also, is one of the principles of OOP: abstraction. The GildedRose class does not need to know HOW to update the items, it just needs to do it; the responsibility of the procedure can be attributed to the module therefore, which stores this functionality in a namespace for us. (See my previous blog post on how we can decide whether to use a class or a module).

Now that we have this module, our GildedRose class can simply look like this:

class GildedRose

include UpdateOperators

  def initialize(items)
    @items = items
  end

  attr_accessor :items

  def update_quality
    items.each do |item|
      default_update(item)
    end
  end

end

• We include the module containing all the “background logic” elsewhere, and the GildedRose class is responsible only for updating the items. We are also still adhering to the rules, which state that we cannot change either the Item class or items attribute.

Though if we turn the tests for special items back on, they will fail.

Introducing classes

This is where I created new classes for managing the statefulness of special items, all of which are inherited from the parent class Item:

# /*/ 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

• We include the module containing all the update methods there aswell, since we can inherit the same functionality for all of these classes.

• We can also add to the UpdateOperators module (therein abstracting, or tucking away) the new methods required, such as max_quality, which checks whether the max quality has been reached for a Brie object, returning true if it is so; or increment_quality that increases the quality instead of decreasing it for a Brie object.

I made an important decision here. I decided that a special object would have the responsibility of knowing how to update itself, which still adhers to SRP, since the class only has ONE reason to change ( i.e. update_self ).

Before adding anything to the GildedRose class, which is the entry point through which pass these objects, I anticipated that I could change the tests to no longer be given an instance of Item.new, now we would be passing in the specific items like Brie.new, Sulfuras.new etc.

After adding this to the tests, I began to turn the tests back on, one by one - of course, they fail, since we can only increment by a value of one.

Though now, the special classes know about themselves and how to update themseleves, so we can add some simple logic to GildedRose class:

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

When iterating through the objects, we ask whether the block variabe item in the array we are given knows how to update itself with:

item.respond_to?(:update_self) ? item.update_self : default_update(item)

• We’re basically saying, do you know how to update yourself? If you do, then do so, otherwise, we’ll take care of it.

Limitations

I’ll post the preliminary solution below, however I have encountered a limitation in this first draft.

Although we have eliminated the magic strings, and numbers, it may be the choice of the store owner to not have to distinguish between Item.new() and Brie.new themselves. They may simply want to always pass in an Item.new("Brie", 10, 10) instead of Brie.new(("Brie", 10, 10).

One way I anticipate handling this is going back to our initial reaction to the code, using a hash constant that we can gradually update:

class GildedRose

  ITEM_CLASSES = {
    "Aged Brie" : Brie,
    "Sulfuras, Hand of Ragnaros" : Sulfuras,
    "Backstage passes to a TAFKAL80ETC concert" : BackstagePasses,
  }

  def initialize(items)
    @items = items
  end

  def update_quality
    @items.each do |item|
      item_class = ITEM_CLASSES[item.name] || Item <1>
      special_item = item_class.new(item.name, item.sell_in, item.quality)
      special_item.update_self || default_update(item)
    end
  end
end

1. When we iterate through the elements of a given array, we are saying: “Do you recognise the given item name inside our catalogue of special items? If you do then create an instance of that item, otherwise just create a basic Item object.

In this way we say, we could simply update the constant with any new special items after first creating a class for it. We would only be dealing with the Item class.

The drawback I have yet to explore, is whether we would be able to have the instance of a special item remember its updated state, without having to re-insert manually the new quality value of a given item.

Please let me know in the comments below if you have any suggestions, ideas, or questions.

Resources

• My preliminary refactor on Github

• The original kata here

• Resources for later: SOLID Design Principles - Dependency Injection

• To consider after second draft: Lomig walkthrough