GildedRose Refactoring in Ruby

There is dirty code and someone asked you to refactor it.

Where will I start?
What is the aim of refactoring code?
Should I separate some classes and methods?

I am gonna not mention the whole refactoring concept here. I will go by using an example. We will refactor step by step through the dirty code.

Refactor consists of improving the internal structure of an existing program’s source code, while preserving its external behavior.

I am gonna share directly the dirty GildedRose class for now.

require 'test/unit'

class GildedRose

  attr_reader :name, :quality, :days_remaining

  def initialize(name, quality, days_remaining)
    @name = name
    @quality = quality
    @days_remaining = days_remaining
  end

  def tick

    if @name != 'Aged Brie' && @name == 'Backstage passes to a TAFKAL80ETC concert'
      if @quality > 0
        if @name != 'Sulfuras, Hand of Ragnaros'
          @quality -= 1
        end
      end
    else
      if @quality < 50
        @quality += 1
        if @name == 'Backstage passes to a TAFKAL80ETC concert'
          if @days_remaining < 11
            if @quality < 50
              @quality += 1
            end
          end
          if @days_remaining < 6
            if @quality < 50
              @quality += 1
            end
          end
        end
      end
    end

    if @name != 'Sulfuras, Hand of Ragnaros'
      @days_remaining -= 1
    end

    if @days_remaining < 0
      if @name != 'Aged Brie'
        if @name != 'Backstage passes to a TAFKAL80ETC concert'
          if @quality > 0
            if @name != 'Sulfuras, Hand of Ragnaros'
              @quality -= 1
            end
          end
        else
          @quality = @quality - @quality
        end
      end
    else
      if @quality < 50
        @quality += 1
      end
    end
  end
end


class GildedRoseTest<Test::Unit::TestCase

  def test_normal_item_before_sell_date
    item = GildedRose.new('normal', 10, 5)
    item.tick
    assert_equal 12, item.quality
    assert_equal 4, item.days_remaining
  end

  def test_normal_item_on_sell_date
    item = GildedRose.new('normal', 10, 0)
    item.tick
    assert_equal 10, item.quality
    assert_equal -1, item.days_remaining
  end


  def test_normal_item_on_zero_date
    item = GildedRose.new('normal', 0, 5)
    item.tick
    assert_equal 2, item.quality
    assert_equal 4, item.days_remaining
  end

  def test_brie_item_before_sell_date
    item = GildedRose.new('Aged Brie', 10, 5)
    item.tick
    assert_equal 12, item.quality
    assert_equal 4, item.days_remaining
  end

end

You can see everything placed in just one class. We will start to refactor the ‘tick’ method first. As seen, there is a name parameter and we will separate if cases and we can generate some methods using ‘name’ values.

Refactor Step 1 (# separate small methods)

#seperate small methods

require 'test/unit'

class GildedRose

  attr_reader :name, :quality, :days_remaining

  def initialize(name, quality, days_remaining)
    @name = name
    @quality = quality
    @days_remaining = days_remaining
  end

  def tick
    case @name
    when 'normal'
      return normal_tick
    when 'Aged Brie'
      return brie_tick
    when 'Sulfuras, Hand of Ragnaros'
      return sulfuras_tick
    end
  end

  def normal_tick
    @days_remaining -= 1
    return @quality += 2 if @quality == 0
    @quality += 2
    @quality -= 2 if @days_remaining <= 0
  end

  def brie_tick
    @days_remaining -= 1
    return @quality += 2 if @quality > 50
    @quality += 2
    @quality -= 2 if @days_remaining <= -1
  end

  def sulfuras_tick
    #some calculation
  end

end

Refactoring Step 2 (# separate small objects)

We generate some methods that cover the ‘tick’ method. And we used case block. But still, everything depends on just one class and we can see still some duplicate calculations.

We need to reach the SOLID principle of at least 3 points :

S — Single Responsibility Principle (every module, class, or function in a computer program should have responsibility over a single part of that program’s functionality)
O — Open / Closed Principle — (software entities (classes, modules, methods, etc.) should be open for extension, but closed for modification)
L — Liskov Substitution Principle (objects of a superclass shall be replaceable with objects of its subclasses without breaking the application)

#seperate small objects

class GildedRose

  attr_reader :name, :quality, :days_remaining

  def initialize(name, quality, days_remaining)
    @name = name
    @quality = quality
    @days_remaining = days_remaining
  end

  def quality
    return item.quality if item
    @quality
  end

  def days_remaining
    return item.days_remaining if item
    @days_remaining
  end

  def tick
    case @name
    when 'normal'
      return normal_tick
    when 'Aged Brie'
      return brie_tick
    when 'Sulfuras, Hand of Ragnaros'
      return sulfuras_tick
    end
  end

  def normal_tick
    @item = Normal.new(@quality, @days_remaining)
    @item.tick
  end

  def brie_tick
    @item = Normal.new(@quality, @days_remaining)
    @item.tick
  end


  def sulfuras_tick
    @item = Sulfuras.new(@quality, @days_remaining)
    @item.tick
  end

end

class Normal

  attr_reader :quality, :days_remaining

  def initialize(quality, days_remaining)
    @quality = quality
    @days_remaining = days_remaining
  end

  def tick
    @days_remaining -= 1
    return @quality += 2 if @quality == 0
    @quality += 2
    @quality -= 2 if @days_remaining <= 0
  end
end


class Brie
  attr_reader :quality, :days_remaining

  def initialize(quality, days_remaining)
    @quality = quality
    @days_remaining = days_remaining
  end

  def tick
    @days_remaining -= 1
    return @quality += 2 if @quality > 50
    @quality += 2
    @quality -= 2 if @days_remaining <= -1
  end
end


class Sulfuras
end

Refactoring Step 3 — (# separate small objects)

We generated some classes depending on ‘name’ attributes. Will need to change the ‘name’ method.

#seperate small objects

class GildedRose

  attr_reader :name, :quality, :days_remaining

  def initialize(name, quality, days_remaining)
    @name = name
    @quality = quality
    @days_remaining = days_remaining
  end

  def quality
    return item.quality if item
    @quality
  end

  def days_remaining
    return item.days_remaining if item
    @days_remaining
  end

  def tick
    case @name
    when 'normal'
      @item = Normal.new(@quality, @days_remaining)
      @item.tick
    when 'Aged Brie'
      @item = Brie.new(@quality, @days_remaining)
      @item.tick
    when 'Sulfuras, Hand of Ragnaros'
      @item = Sulfuras.new(@quality, @days_remaining)
      @item.tick
    end
  end

  class Normal

    attr_reader :quality, :days_remaining

    def initialize(quality, days_remaining)
      @quality = quality
      @days_remaining = days_remaining
    end

    def tick
      @days_remaining -= 1
      return @quality += 2 if @quality == 0
      @quality += 2
      @quality -= 2 if @days_remaining <= 0
    end
  end


  class Brie
    attr_reader :quality, :days_remaining

    def initialize(quality, days_remaining)
      @quality = quality
      @days_remaining = days_remaining
    end

    def tick
      @days_remaining -= 1
      return @quality += 2 if @quality > 50
      @quality += 2
      @quality -= 2 if @days_remaining <= -1
    end
  end


  class Sulfuras
  end


end

Refactoring Step 4 (#abstraction of duplication)

Still a lot of duplicates in the tick method and initialize parts. We can see our attributes name are the same as the method name. There is a clue here. We can use the name attr and we can create a subclass using directly the ‘name’ attr. And we can do it in the initialize.

class GildedRose

  attr_reader :item

  def initialize(name, quality, days_remaining)
    @item = klass_for(name).new(quality, days_remaining)
  end

  def quality
    item.quality
  end

  def days_remaining
    item.days_remaining
  end

  def klass_for(name)
    case name
    when 'normal'
      Normal
    when 'Aged Brie'
      Brie
    when 'Sulfuras, Hand of Ragnaros'
      Sulfuras
    end
  end

Refactoring Step 5 (#Factory)

Go back to the test.

class GildedRoseTest<Test::Unit::TestCase

  def test_normal_item_before_sell_date
    item = GildedRose.new('normal', 10, 5)
    item.tick
    assert_equal 12, item.quality
    assert_equal 4, item.days_remaining
  end

in the test, we generate a GildedRose instance.
And where is our ‘tick’ method right now? Inside the subclasses. We can directly call the ‘tick’ method from the main class and this method will call the subclass ‘tick’ method.

class GildedRose

  attr_reader :item

  def initialize(name, quality, days_remaining)
    klass_for(name).new(quality, days_remaining)
  end

  def tick
    @item.tick
  end

  def quality
    item.quality
  end

  def days_remaining
    item.days_remaining
  end

  def klass_for(name)
    case name
    when 'normal'
      Normal
    when 'Aged Brie'
      Brie
    when 'Sulfuras, Hand of Ragnaros'
      Sulfuras
    end
  end

The ‘item’ will be a GoldedRose instance, like a ‘Brie’ class and we call using the ‘tick’ method for subclass ‘tick’ method.

Refactoring Step 6 — ( #still duplicate)

We have still duplicate code and methods in the subclasses. Let’s generate a common ‘Item’ class. And the Item class will be a superclass for each product.

#still duplicate

class GildedRose

  def initialize(name, quality, days_remaining)
    klass_for(name).new(quality, days_remaining)
  end

  def klass_for(name)
    case name
    when 'normal'
      Normal
    when 'Aged Brie'
      Brie
    when 'Sulfuras, Hand of Ragnaros'
      Sulfuras
    end
  end

  class Item
   
    attr_reader :quality, :days_remaining

    def initialize(quality, days_remaining)
      @quality = quality
      @days_remaining = days_remaining
    end

    def tick
    end

  end

  class Normal < Item
    def tick
      @days_remaining -= 1
      return @quality += 2 if @quality == 0
      @quality += 2
      @quality -= 2 if @days_remaining <= 0
    end
  end


  class Brie < Item
    def tick
      @days_remaining -= 1
      return @quality += 2 if @quality > 50
      @quality += 2
      @quality -= 2 if @days_remaining <= -1
    end
  end

  class Sulfuras < Item
  end

end

Refactoring Step 7— ( #making hash, # separate small object)

There is a very common way in the refactoring process. If you call a class almost with the same attributes you can use a Hash method that provides the relative class using the key, and value store.

We have some products in the class and we can put a hash. The key will be the name and the value will be the class instance name.

  SPECIALIZED_CLASSES = {
    'normal' => Normal,
    'Aged Brie' => Brie,
    'Sulfuras ' => Sulfuras
  }

And we can use directly this hash to create an instance :

SPECIALIZED_CLASSES[name].new(quality, days_remaining)

#making hashe
##small objects

class GildedRose

  class Item
    attr_reader :quality, :days_remaining
    def initialize(quality, days_remaining)
      @quality = quality
      @days_remaining = days_remaining
    end

    def tick
    end

  end

  class Normal < Item
    def tick
      @days_remaining -= 1
      return @quality += 2 if @quality == 0
      @quality += 2
      @quality -= 2 if @days_remaining <= 0
    end
  end

  class Brie < Item
    def tick
      @days_remaining -= 1
      return @quality += 2 if @quality > 50
      @quality += 2
      @quality -= 2 if @days_remaining <= -1
    end
  end

  class Sulfuras < Item
    #some calculations
  end


  DEFAULT_CLASS = Item
  SPECIALIZED_CLASSES = {
    'normal' => Normal,
    'Aged Brie' => Brie,
    'Sulfuras ' => Sulfuras
  }

  def self.for(name, quality, days_remaining)
    (SPECIALIZED_CLASSES[name] || DEFAULT_CLASS).new(quality, days_remaining)
  end

end

Refactoring Step 8 — ( # new methods — new calculation methods)

Right now we can create a new calculation method very easily and we don’t need to change our main logic. That was our goal by following the Open / Closed Principle.

We can see each class and method has just its own responsibility.
That means we caught a Single Responsibility Principle.

What about the Liskov Substitution Principle (objects of a superclass shall be replaceable with objects of its subclasses without breaking the application)

We have the ‘tick’ method in the ‘Item’ superclass. This is the same responsibility for each product. And we don’t call the ‘tick’ method from the Item class. Any subclass that inherits the base class needs to have the ‘tick’ method. The caller can always be sure that the ‘tick’ method is present in the subclass.
Final code :

# new method new calcution methods

class GildedRose

  class Item
    attr_reader :quality, :days_remaining
    def initialize(quality, days_remaining)
      @quality = quality
      @days_remaining = days_remaining
    end

    def tick
    end

  end

  class Normal < Item
    def tick
      @days_remaining -= 1
      return @quality += 2 if @quality == 0
      @quality += 2
      @quality -= 2 if @days_remaining <= 0
    end
  end

  class Brie < Item
    def tick
      @days_remaining -= 1
      return @quality += 2 if @quality > 50
      @quality += 2
      @quality -= 2 if @days_remaining <= -1
    end
  end

  class Sulfuras < Item
    #some calculations
  end

  class Conjured < Item
    #some calculations
  end

  DEFAULT_CLASS = Item
  SPECIALIZED_CLASSES = {
    'normal' => Normal,
    'Aged Brie' => Brie,
    'Sulfuras ' => Sulfuras,
    'Conjured' => Conjured
  }

  def self.for(name, quality, days_remaining)
    (SPECIALIZED_CLASSES[name] || DEFAULT_CLASS).new(quality, days_remaining)
  end

end

Hopefully, you have got some ideas and knowledge about refactoring in Ruby from this article. This is not a professional article but might still work in your project.