A Walk Through the Gilded Rose Kata — Pt 2: Duplication is your friend (at first)
Tags: ruby, refactorit, oop
Published: 2021-04-11 22:30:04

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.

```ruby
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.

```ruby
# 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:

```ruby
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`.

```ruby
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 😊

![Failing test - Aged Brie](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/bjv3cu4s4mzpflkfyfku.png)
 
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.
 
 
```ruby
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:

```ruby
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:

```ruby
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:

```ruby
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!):

```ruby
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".

![Everyone is special](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/rm6im4oht0wdt5se94nl.gif)

# 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: 

```ruby
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:

![case...when pattern](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/9vke1obsa7ob0h92j8cv.png)
 
```ruby
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 😊