A few days ago, I have been made aware of the Gilded Rose Kata, with quite a promising introduction — well, promising to me: a few years back it aroused Sandi Metz' interest.

I love Sandi Metz. I just love her. She is one of the smartest people around when it comes to coding, especially when dealing with what OO should be like; her books and talks are always an absolute blast, and I can only hope that one day I will be able to see programming as she does.

That could only mean that it was an interesting problem, and I wondered if I would be able to tackle it in an elegant way... or perhaps better, in a way Sandi would endorse. (Spoiler: I have been told our solutions and even our thought processes were quite similar, which is a great honor. This being said, I read her books often enough not to have any merits: monkey sees, monkey does.)

Aside from being a full-grownup developer, I teach to baby ones at Le Wagon, a high-quality product-oriented ruby-on-rails-based bootcamp, and I have learned a few things doing so: to understand a concept, result is quite meaningless, it is the thought process which has value. I can teach how to test a codebase, I can show good tests which will be understood by students, if they don't understand why we test and how we choose which test to write and when, not a lot is gained: people will know how to theoretically create a testing suite, but will not be able to do so in real life.

This got me to this idea: what about explaining how I solved this kata, step by step? I am no wise man and I am sure there are a lot of people with a better solution, or a simpler, or with different ways of seing the matter at hand, but I think that there may be things to get and to be inspired by when looking into someone else's mind. And maybe other people than my students may find interest in what I did.

The Kata

If you want to try it for yourself, you may find it here.

Allison, proud owner of the "Gilded Rose Inn" in Stormwind, meets lots of wannabe adventurers with whom she trades.
There are a lot of products to inventory, and so she has used for the past few months a software to keep track of her goods, with great success.

• Every item ages 1 day each day
• An item loses 1 quality each day (but cannot be below 0)
• Every item has a use-by-date and may spoil faster after it

Of course, this is a very basic set of rules, and there are exceptions. You can read the full criteria list, but it implies products like cheese which obviously gains quality each passing day, or concert tickets whose demand would go higher and higher each day up until the concert is over, and then their quality would be non existent.

The objective is to add a new kind of product: Conjured items, with their new special set of rules. They are magical, you see, created by spell from thin air, and they spoil way faster.

The Code

The code is a frightening 40-something long conditional, here below:

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

I have a confession to make: I hate conditionals. I use them in guard clauses fine enough, I use an if if I cannot do otherwise, but each time I see an else, I want to punch it in the face. I will do whatever in my power to get rid of it. I hate them. They look stupid.
...and so you can imagine what I felt in front of this piece of code! It was like opening the door of the restroom and meeting face to face a sick-to-his-stomach Beelzebub doing his business.

But... it works. It just does what it is supposed to. I do not need to change it, not even to look at it, I could just add an encapsulating if checking for my sweet conjured items, else the abomination above, and it would work. That would defeat the point, though. I feel I am urged to do more, and to be honest... I want to do more. I want to punch those conditionals in the face.

I'll have to refactor this method. It does make sense in a business point of view: we cannot keep accumulating conditionals on conditionals if we are asked to add more new items. I checked swiftly if I could integrate my conjured items "inside" the mess, but I could not grasp the extend of it.

And so I bailed out. Maybe it was the way to go though — maybe there was a pattern to be seen — but with my tiny mind, I was not able to grasp it. Don't hesitate to poke me if there was a way, if you feel my approach is flawed.

It does not mean I was defeated by the kata though! Oh, no. If I cannot understand the pattern, I shall change it for a pattern I can understand. But first, I need to be sure I will not break anything.

Do not break anything

People tend to mean "it should still return the same result" by that. Of course it plays a huge part of "not breaking anything", but in reality, the rule is a little broader.

I do not know what is interconnected with the piece of code I am working on and so I do not feel free to change everything as I see fit. I have to keep the interface as it is whether I find it relevant or not. Later would be the time to propose some change if needed, but now, I have to be sure I do not break anything, inside, but also outside of my code.

First design decision: the scope

• I will not change the Item class;
• The GildedRose class has to keep accepting an array of those Item objects, and respond to the #update_quality message.

Everything else, I feel at liberty to change. The problem is that liberty is not what stops us to do drastic changes; fear is. Fear that we tampered with the expected results once our refactoring took place.

Here come the tests

Beginners often ask why TDD is used, or even, is effective. One of the answers is: "because it allows for refactoring without any fear of messing up". Well, that is, if you did not screw your tests.

When you refactor existing code, it is even more vital if you feel you need to extensively change the codebase. I will not, I shall not change a single line before being sure I covered every use case with tests. I fear regressions more than initial bugs.

Fortunately, this kata is provided with a simple script which displays the evolution of some items over the course of days.

I just have to run this script before any code modification on a sufficient amount of days, save the result, and then compare it to any run once I changed my code!

I have what I need: a source of truth.

describe GildedRose do
  before(:all) { `ruby spec/texttest_fixture.rb 100 > test.txt` }
  after(:all)  { `rm test.txt` }

  describe "#update_quality" do
    it "has no regression" do
      expected = "spec/100_updates_expected_results.txt"
      actual = "test.txt"

      expect(IO.read(actual)).to eq IO.read(expected)
    end
  end
end

This being said, it is not as handy as nice unit tests I can solve one by one with readable error message, so I will need to create tests anyway — This source of truth (I arbitrarily chose 100 days, because why not?) will only be here to confirm other tests.

I ended with a very long test file, with lots of cases:

• Some inferred from the written rules provided by Allison
• Some provided by different attempts with the source of truth

They are not pretty tests, in a sense that there sure are duplicates and useless ones; I would take a more thoughtful approach to testing if it was TDD with my own code, but as I handle legacy entangled code, I prefer to be safe than sorry.

For the sake of those articles, the only thing to remember is: if ever I break something, a test will catch it.

And now, we can try and find where to start to untangle this mess... In a second article, in order to be kind on your eyes and your attention.

See you soon :)