4
\$\begingroup\$

The Gilded Rose Kata is a refactoring challenge, in which some frankly disgusting code is thrown at the developer, and they're told to make a small change to it. For reference, the (truncated) requirements and the original code are given here:

  • 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

  • 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
  • Feel free to make any changes to the UpdateQuality method and add any new code as long as everything still works correctly. However, do not alter the Item class or Items property

  • 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

And the code to be refactored is as follows:

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

So, the first thing I did was add some specs:

require File.join(File.dirname(__FILE__), 'gilded_rose')
require 'rspec'
require 'rspec/autorun'

RSpec::Matchers.define :update_to do |sell_in, quality|
  match do |item|
    new_item = update(actual)
    expect(new_item.sell_in).to eq sell_in
    expect(new_item.quality).to eq quality
  end
  failure_message do |actual|
    new_item = update(actual)
    "Expected #{actual} to end up with sell_in=#{sell_in} and quality=#{quality}. Got #{new_item}."
  end

  def update(item)
    item.clone.tap{|i| GildedRose.new([i]).update_quality}
  end
end

describe GildedRose do
  describe "#update_quality" do
    it "does not change the name" do
      items = [Item.new("foo", 0, 0)]
      GildedRose.new(items).update_quality
      expect(items[0].name).to eq "foo"
    end

    it "decreases the quality of an in-date item by 1" do
      expect(Item.new("foo", 10, 10)).to update_to 9,9
      expect(Item.new("foo", 1, 10)).to update_to 0,9
    end

    it "decreases the quality of an out-of-date item by 2" do
      expect(Item.new("foo", 0, 10)).to update_to -1,8
    end

    it "does not drop quality below 0" do
      expect(Item.new("foo", 0, 0)).to update_to -1,0
    end

    it "does not decrease the quality or sell by of sulfuras" do
      expect(Item.new("Sulfuras, Hand of Ragnaros", 10, 10)).to update_to 10,10
    end

    it "increases the quality of in-date aged brie" do
      expect(Item.new("Aged Brie", 10, 10)).to update_to 9,11
    end

    it "rapidly increases the quality of out-of-date aged brie" do
      expect(Item.new("Aged Brie", 0, 10)).to update_to -1, 12
    end

    it "does not increase the quality of any item over 50" do
      items = [Item.new("Aged Brie", 10, 50), Item.new("Aged Brie", 0, 49), Item.new("Backstage passes to a TAFKAL80ETC concert", 9, 49)]
      GildedRose.new(items).update_quality
      items.each do |item|
        expect(item.quality).to eq 50
      end
    end

    it "increases the quality of a backstage pass more than 10 days before its sell by date" do
      expect(Item.new("Backstage passes to a TAFKAL80ETC concert", 11, 10)).to update_to 10,11
    end

    it "rapidly increases the quality of a backstage pass between 6 and 10 days before its sell by date" do
      expect(Item.new("Backstage passes to a TAFKAL80ETC concert", 10, 10)).to update_to 9,12
      expect(Item.new("Backstage passes to a TAFKAL80ETC concert", 6, 10)).to update_to 5,12
    end

    it "very rapidly increases the quality of a backstage pass between 1 and 5 days before its sell by date" do
      expect(Item.new("Backstage passes to a TAFKAL80ETC concert", 5, 10)).to update_to 4,13
      expect(Item.new("Backstage passes to a TAFKAL80ETC concert", 1, 10)).to update_to 0,13
    end

    it "drops the quality of a backstage pass to 0 beyond its sell by date" do
      expect(Item.new("Backstage passes to a TAFKAL80ETC concert", 0, 10)).to update_to -1,0
      expect(Item.new("Backstage passes to a TAFKAL80ETC concert", -3, 10)).to update_to -4,0
    end

    it "rapidly decreases the quality of a conjured item" do
      expect(Item.new("Conjured mana cake", 10, 10)).to update_to 9,8
      expect(Item.new("Conjured mana cake", 0, 10)).to update_to -1,6
    end

  end

end

And then the next thing I did was introduce everyone's favourite buzzword: dependency injection, refactoring the original main class into this:

require 'bundler/setup'
require 'active_support/inflector'

Dir.glob(File.join(File.dirname(__FILE__), 'mechanics', '*')).each{|f| require f}

class GildedRose

  def initialize(items)
    @items = items
    mechanics = load_mechanics
    @items.each do |item|
      mechanics.each{|m| item.extend(m) if m.matches?(item)}
    end
  end

  def update_quality
    @items.each(&:update)
  end

  private
  def load_mechanics
    Dir.glob(File.join('mechanics', '*')).map{|mechanic_file| load_mechanic_from(mechanic_file)}.sort_by(&:priority)
  end

  def load_mechanic_from(file_name)
    constant_name = file_name.chomp(File.extname(file_name)).camelize
    begin
      constant_name.constantize
    rescue NameError
      raise LoadError, "Expected #{file_name} to define #{constant_name}"
    end
  end

end

Each item now has some number of mechanics glommed onto it when the GildedRose class is instantiated. Each mechanic then defines the minimal amount of behaviour necessary for the item to do its job.

mechanics/base.rb:

module Mechanics
  module Base
    def self.matches?(item)
      true
    end
    def self.priority
      0
    end
    private
    def age
      self.sell_in -= 1
    end
    def degrade
      self.quality -= self.sell_in < 0 ? 2 : 1
    end
    def clamp_quality
      self.quality = [self.quality, 0].max
      self.quality = [self.quality, 50].min
    end
    public
    def update
      age
      degrade
      clamp_quality
    end
  end
end

mechanics/legendary.rb:

module Mechanics
  module Legendary
    def self.matches?(item)
      item.name == "Sulfuras, Hand of Ragnaros"
    end
    def self.priority
      1
    end
    public
    def update
    end
  end
end

mechanics/time_limited.rb:

module Mechanics
  module TimeLimited
    def self.priority
      2
    end
    def self.matches?(item)
      item.name == "Backstage passes to a TAFKAL80ETC concert"
    end
    private
    def degrade
      case
      when self.sell_in < 0
        self.quality = 0
      when self.sell_in < 5
        self.quality += 3
      when self.sell_in < 10
        self.quality += 2
      else
        self.quality += 1
      end
    end
  end
end

mechanics/vintage.rb:

module Mechanics
  module Vintage
    public
    def self.matches?(item)
      item.name == "Aged Brie"
    end
    def self.priority
      2
    end
    private
    def degrade
      self.quality += self.sell_in < 0 ? 2 : 1
    end
  end
end

At this point, it's pretty easy to make the change to allow conjured items to be implemented, by creating the file mechanics/conjured.rb with the content:

 module Mechanics
   module Conjured
    def self.priority
      2
    end
    def self.matches?(item)
      item.name =~ /conjured/i
    end
    private
    def degrade
      self.quality -= self.sell_in < 0 ? 4 : 2
    end
   end
 end

At this point, all the specs passed and I considered the challenge complete (save for a bunch of shuffling behaviour round to generally make things neater), which brings me to the review objectives:

  • Is this coherent, or does it just look like the ramblings of a madman? The primary goal here is to make something that's more maintainable, and I've a little worry that by adding in all this autoloady stuff I've actually made it harder to tell what's going on.
  • I've never used rspec before. Have I made any stylistic blunders? Are there any features I should have been using but didn't?
  • Dependency injection is something I've read about, but never seen implemented. Is there a cleaner way of doing it here? priority seems a bit kludgey, but I like what it lets me do; I can start layering mechanics on top of each other to make blessed, enchanted, masterwork, etc items if I want to.

Of course, feel free to comment on anything and everything, but the above is what I'm interested in right now. The code's all been pushed to GitHub, so if you want to comment on my commit messages feel free to do that, too.

\$\endgroup\$
  • 1
    \$\begingroup\$ i will try to elaborate on this later if no one else answers, but yeah, your suspicion is correct and your refactoring is over-engineered and not easy to follow. One small and easy thing: choose clear names. I don't see the word 'Mechanic' anywhere in the problem specs, so why is it the primary name in your solution? \$\endgroup\$ – Jonah May 9 '16 at 17:23
  • \$\begingroup\$ Your GitHub changes would be easier to comment on if it were in the form of a pull request. \$\endgroup\$ – Kenrick Chien May 13 '16 at 14:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.