Lomig

Posted on Apr 11, 2021 • Edited on Apr 13, 2021

A Walk Through the Gilded Rose Kata — Pt 2: Duplication is your friend (at first)

#ruby #refactorit #oop

So, we left our kata at the end of the last post with some tests, but still no clue about how to handle the entanglement in the gross conditional that is at the core of the GildedRose app.

If you tried this kata yourself, or if you try to relate with some difficult pieces of code you encountered, you perhaps tried to understand a bit of what happens, and tiny fragment by tiny fragment, patiently untangle the code.

As I said, I will not; I cannot. It is a conditional! Yuck!
What I understand and like, though, are guard clauses.
Guard clauses are simple, they are nice, and most importantly to me, they are flat.

A guard clause?

For people unfamiliar with the concept, a guard clause is a pattern that would require to leave a method very early and return easy results, or special cases results, for the method to focus on its main algorithm.

Everything is better and clearer with an example! Imagine a method that would divide an integer by another one.

def divide(dividend:, divisor:)
  dividend.to_f / divisor
end

That is splendid, but if divisor is 0, there will be an issue. We want to return the string "error" when a user tries such a calculus.

# Conditional
def divide(dividend:, divisor:)
  if !divisor.zero?
    dividend.to_f / divisor
  else
    "error"
  end
end

# Guard clause
def divide(dividend:, divisor:)
  return "error" if divisor.zero?

  dividend.to_f / divisor
end

It works because return stops the execution of the method, making any else useless.

But how can we apply this concept to my messy method at hand?

Excluding a first special case from the execution

Instead of trying to untangle what I have, I will exclude something. Anything. Let us take a look at the code:

def update_quality()
    @items.each do |item|
      if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
        # <Moar code here!>
      end
    end
  end

There seem to be a case where item.name equals Aged Brie; I decide (at random, any other would work, it was just the first to appear here) it will be my special case.

I just want to return something in that case. What do I want to return? I don't know yet, but is it a problem? Unit Tests have my back in any case!

⚠️ In the Guard Clause example above, I used return to stop the execution of the method. Here, I will want to stop the execution of a block. Unlike some other languages, Ruby is nice enough to provide a substitute for return in blocks: next.

def aged_brie_update(item)
  # Here will be what we want to do with our Aged Brie!
end

def update_quality()
    @items.each do |item|
      next aged_brie_update(item) if item.name == "Aged Brie"

      if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
        # <Moar code here!>
      end
    end
  end

Again, you can see I do not try to untangle the conditional. If the item is the Aged Brie, I will be able to deal with it outside of the conditional. If it is not, the conditional will take over. That way, the only tests that should fail are the one related to the Aged Brie: obviously, we do not do anything with it 😊

So, test by test, I work towards green. I do not really care about special refactoring, even though we can argue I have my coding style. It seems simple enough, far away from the problems of our #update_quality method.

What do we want?

Each time, it ages;
Each time, it gains 1 quality;
If item.sell_in is overdue, each time it gains an extra quality;
It cannot go above 50 quality.

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

My tests being green again, I can take for granted that I did not break anything.

You may think that I made the execution path even more complicated, and the GildedRose class even longer, but bear with me: do not forget I am a stupid man, and I would do anything to punch an else in the guts.

Repeat...

I take my code:

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

def update_quality()
  @items.each do |item|
    next aged_brie_update(item) if item.name == "Aged Brie"

    if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
      # <Moar code here!>
    end
  end
end

I extract a special case:

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

def backstage_update(item)
  #What should we do with the backstage pass?
end

def update_quality()
  @items.each do |item|
    next aged_brie_update(item) if item.name == "Aged Brie"
    next backstage_update(item) if item.name == "Backstage passes to a TAFKAL80ETC concert"

    if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
      # <Moar code here!>
    end
  end
end

My tests are red for the Backstage passes, and I make them green:

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

def backstage_update(item)
  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
  item.quality = 50 if item.quality > 50
end

def update_quality()
  @items.each do |item|
    next aged_brie_update(item) if item.name == "Aged Brie"
    next backstage_update(item) if item.name == "Backstage passes to a TAFKAL80ETC concert"

    if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
      # <Moar code here!>
    end
  end
end

And rinse!

Extraction by extraction, guard clause by guard clause, I work toward a state where I cannot find any special case anymore.

At this stage, my code looks like this (notice the Sulfuras method that is empty 😀 — as soon as I extracted this special case, the tests were already green!):

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

def backstage_update(item)
  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
  item.quality = 50 if item.quality > 50
end

def sulfuras_update(item)
end

def update_quality()
  @items.each do |item|
    next aged_brie_update(item) if item.name == "Aged Brie"
    next backstage_update(item) if item.name == "Backstage passes to a TAFKAL80ETC concert"
    next sulfuras_update(item)  if item.name == "Sulfuras, Hand of Ragnaros"

    if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
      # <Moar code here!>
    end
  end
end

As I do not have any special case anymore, what does the conditional do?
Well, in fact we did not deal with all the special cases: We also need to deal with the "not special case", when my product is neither cheese, concert ticket, nor Sulfuras.

I think this is something humans tend to forget: whenever we talk about X special cases, in reality we speak about X + 1 special cases, counting "not special".

Final extraction

We remove the now useless conditional at last, then make the tests for any "Normal item" green, and this is the result — the complete code, this time:

class GildedRose

  def initialize(items)
    @items = items
  end

  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

  def backstage_update(item)
    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
    item.quality = 50 if item.quality > 50
  end

  def sulfuras_update(item)
  end

  def common_update(item)
    item.sell_in -= 1

    item.quality -= 1
    item.quality -= 1 if item.sell_in < 0
    item.quality = 0 if item.quality < 0
  end

  def update_quality()
    @items.each do |item|
      next aged_brie_update(item) if item.name == "Aged Brie"
      next backstage_update(item) if item.name == "Backstage passes to a TAFKAL80ETC concert"
      next sulfuras_update(item)  if item.name == "Sulfuras, Hand of Ragnaros"

      common_update(item)
    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

Do I see the light?

Well, do not worry, I have more tricks in my bag. This being said, we could argue that already it looks better, and it would be easier to add our "Conjured items" new product.

Before ending this second part (there will be more), we can at least remove our guard clauses here. The signal is this pattern:

def update_quality()
  @items.each do |item|
    case item.name
    when "Aged Brie"
      aged_brie_update(item)
    when "Backstage passes to a TAFKAL80ETC concert"
      backstage_update(item)
    when "Sulfuras, Hand of Ragnaros"
      sulfuras_update(item)
    else
      common_update(item)
    end
  end
end

And I will let you ponder this change (I said I hated conditionals, right?) until part 3!

Thanks for reading 😊

DEV Community

A Walk Through the Gilded Rose Kata — Pt 2: Duplication is your friend (at first)

A guard clause?

Excluding a first special case from the execution

Repeat...

And rinse!

Final extraction

Do I see the light?

Top comments (0)

Read next

Docker Autoscaling: Dynamically Adjust Containers Based on Demand

A Pleasant Work Environment = Better Productivity

Docker with Helm: Simplifying Kubernetes Deployment and Management

Docker and Kubernetes Integration: The Ultimate Solution for Containerized Applications