Posted inc# kata

The Gilded Rose Refactoring Kata

No Comments

The Gilded Rose refactoring Kata is a brilliant exercise based on the World of Warcraft inn The Gilded Rose:

Countless hours were spent exploring Azeroth as a kid, this Kata makes me feel at home. I’ve even got on the soundtrack whilst writing this.
Here’s the spec:

Hi and welcome to team Gilded Rose. As you know, we are a small inn with a prime location in a prominent city ran by a friendly innkeeper named Allison. We also buy and sell only the finest goods. Unfortunately, our goods are constantly degrading in Quality as they approach their sell by date.

We have a system in place that updates our inventory for us. It was developed by a no-nonsense type named Leeroy, who has moved on to new adventures. Your task is to add the new feature to our system so that we can begin selling a new category of items. First an introduction to our system:

  • All items have a SellIn value which denotes the number of days we have to sell the items
  • 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

Pretty simple, right? Well this is where it gets interesting:

  • 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

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

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 as those belong to the goblin in the corner who will insta-rage and one-shot you as he doesn’t believe in shared code ownership (you can make the UpdateQuality method and Items property static if you like, we’ll cover for you).

Just for clarification, an item can never have its Quality increase above 50, however “Sulfuras” is a legendary item and as such its Quality is 80 and it never alters.

https://github.com/emilybache/GildedRose-Refactoring-Kata/blob/main/GildedRoseRequirements.md

The best way to start is to run the existing tests:

[Test]
public void foo()
{
    IList<Item> Items = new List<Item> { new Item { Name = "foo", SellIn = 0, Quality = 0 } };
    GildedRose app = new GildedRose(Items);
    app.UpdateQuality();
    Assert.AreEqual("fixme", Items[0].Name);
}

It fails… Let’s get stuck in. Now this is just a case of changing the “fixme” for “foo”, but I’m going to delete this test.

This is the production code, it’s mad:

    public class GildedRose
    {
        IList<Item> Items;
        public GildedRose(IList<Item> Items)
        {
            this.Items = Items;
        }

        public void UpdateQuality()
        {
            for (var i = 0; i < Items.Count; i++)
            {
                if (Items[i].Name != "Aged Brie" && Items[i].Name != "Backstage passes to a TAFKAL80ETC concert")
                {
                    if (Items[i].Quality > 0)
                    {
                        if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
                        {
                            Items[i].Quality = Items[i].Quality - 1;
                        }
                    }
                }
                else
                {
                    if (Items[i].Quality < 50)
                    {
                        Items[i].Quality = Items[i].Quality + 1;

                        if (Items[i].Name == "Backstage passes to a TAFKAL80ETC concert")
                        {
                            if (Items[i].SellIn < 11)
                            {
                                if (Items[i].Quality < 50)
                                {
                                    Items[i].Quality = Items[i].Quality + 1;
                                }
                            }

                            if (Items[i].SellIn < 6)
                            {
                                if (Items[i].Quality < 50)
                                {
                                    Items[i].Quality = Items[i].Quality + 1;
                                }
                            }
                        }
                    }
                }

                if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
                {
                    Items[i].SellIn = Items[i].SellIn - 1;
                }

                if (Items[i].SellIn < 0)
                {
                    if (Items[i].Name != "Aged Brie")
                    {
                        if (Items[i].Name != "Backstage passes to a TAFKAL80ETC concert")
                        {
                            if (Items[i].Quality > 0)
                            {
                                if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
                                {
                                    Items[i].Quality = Items[i].Quality - 1;
                                }
                            }
                        }
                        else
                        {
                            Items[i].Quality = Items[i].Quality - Items[i].Quality;
                        }
                    }
                    else
                    {
                        if (Items[i].Quality < 50)
                        {
                            Items[i].Quality = Items[i].Quality + 1;
                        }
                    }
                }
            }
        }
    }
}

I want to add some basic tests to ensure the quality/price-in values decrease after each day:

[Test]
public void Should_Degrade_Quality()
{
    var elixirOfMongoose = new Item { Name = "Elixir of the Mongoose", SellIn = 5, Quality = 7 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();
    Assert.AreEqual(6, elixirOfMongoose.Quality);
}

[Test]
public void Should_Decrease_SellIn()
{
    var elixirOfMongoose = new Item { Name = "Elixir of the Mongoose", SellIn = 5, Quality = 7 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();
    Assert.AreEqual(4, elixirOfMongoose.SellIn);
}

Next, I want to add some unit tests to cover the scenarios specified in the spec:

  • 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
https://github.com/emilybache/GildedRose-Refactoring-Kata/blob/main/GildedRoseRequirements.md
[Test]
public void Should_Decrease_Quality_Twice_As_Fast_When_SellIn_0()
{
    var elixirOfMongoose = new Item { Name = "Elixir of the Mongoose", SellIn = 0, Quality = 7 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();
    Assert.AreEqual(5, elixirOfMongoose.Quality);
}

[Test]
public void Should_Not_Set_Quality_To_Negative()
{
    var elixirOfMongoose = new Item { Name = "Elixir of the Mongoose", SellIn = 0, Quality = 1 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();
    Assert.AreEqual(0, elixirOfMongoose.Quality);
}

[Test]
public void Aged_Brie_Should_Increase_In_Quality()
{
    var elixirOfMongoose = new Item { Name = "Aged Brie", SellIn = 4, Quality = 5 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(6, elixirOfMongoose.Quality);
}


[Test]
public void Should_Not_Increase_Quality_Of_Item_If_50()
{
    var elixirOfMongoose = new Item { Name = "Aged Brie", SellIn = 4, Quality = 50 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(50, elixirOfMongoose.Quality);
}

[Test]
public void Sulfuras_Stats_Should_Remain_The_Same()
{
    var elixirOfMongoose = new Item { Name = "Sulfuras, Hand of Ragnaros", SellIn = 4, Quality = 5 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(5, elixirOfMongoose.Quality);
    Assert.AreEqual(4, elixirOfMongoose.SellIn);
}


[Test]
public void Backstage_Passes_Should_Increase_In_Quality()
{
    var elixirOfMongoose = new Item
        { Name = "Backstage passes to a TAFKAL80ETC concert", SellIn = 12, Quality = 5 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(6, elixirOfMongoose.Quality);
}

[Test]
public void Backstage_Passes_Should_Increase_Quality_By_2_When_Less_Than_10_Days()
{
    var elixirOfMongoose = new Item
        { Name = "Backstage passes to a TAFKAL80ETC concert", SellIn = 9, Quality = 5 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(7, elixirOfMongoose.Quality);
}

[Test]
public void Backstage_Passes_Should_Increase_Quality_By_3_When_Less_Than_5_Days()
{
    var elixirOfMongoose = new Item
        { Name = "Backstage passes to a TAFKAL80ETC concert", SellIn = 4, Quality = 5 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(8, elixirOfMongoose.Quality);
}

[Test]
public void Backstage_Passes_Should_Set_Quality_To_0_After_Concert()
{
    var elixirOfMongoose = new Item
        { Name = "Backstage passes to a TAFKAL80ETC concert", SellIn = 0, Quality = 5 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(0, elixirOfMongoose.Quality);
}

Before I start refactoring I’ll run a code coverage report to see if there are any lines I’ve missed, there might be some edge cases.

Ah, it looks like I’ve missed a test. See that else statement starting on line 90. So I need a test that covers when it’s past its SellIn date and is “Aged Brie” then it should continue to increase in quality.

Hmmm, this following test fails because it’s expecting the quality to be 12. This means that when “Aged Brie” is out of date, it increases quality twice as fast. 

[Test]
public void Aged_Brie_Should_Increase_In_Quality_When_SellIn_Is_0()
{
    var elixirOfMongoose = new Item { Name = "Aged Brie", SellIn = 0, Quality = 10 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();
    Assert.AreEqual(11, elixirOfMongoose.Quality);
}

There’s nothing in the spec that states that should happen, so this is either a bug or just not written down. I’ll update the test so it expects the value the production code is outputting just so I don’t break anything. I’ll have to question this one with the innkeeper Allison.

Now I feel much more comfortable about refactoring UpdateQuality.

There’s a tonne of nested if statements and repeating conditionals in the code, this violates both the Single Responsibility Principle and the Open/Closed principle. The code is handling multiple scenarios/behaviours and is not open for extension.

All the code is doing is incrementing the quality and the buyIn of an item. Most of the logic is around quality. I’m going to create a factory which’ll create specific quality updaters for each item.

I’ll define a new interface IQualityUpdater

public interface IQualityUpdater
{

    public void UpdateQuality(Item item);
}

And create an example updater. Notice how the following AgedBrieQualityUpdater is easily testable; ripe for some unit tests.

public class AgedBrieQualityUpdater : IQualityUpdater
{
    public void UpdateQuality(Item item)
    {
        item.SellIn--;
        if (item.Quality < 50)
        {
            item.Quality++;
            if (item.SellIn < 0) // this is the edge case
            {
                item.Quality++;
            }
        }
    }
}

Next, I’ll create the factory.

public class QualityUpdaterFactory
{

    public IQualityUpdater GetQualityUpdater(string name)
    {
        if (name == "Aged Brie")
        {
            return new AgedBrieQualityUpdater();
        }

        return null;
    }
}

Next, I need to update the GildedRose class to use this factory. Of course, I want to do this incrementally, so I’ll add an updater, run the tests, and then remove any code relating to it.

  public class GildedRose
    {
        IList<Item> Items;
        QualityUpdaterFactory _factory;
        
        public GildedRose(IList<Item> Items)
        {
            this.Items = Items;
            this._factory = new QualityUpdaterFactory();
        }


        public void UpdateQuality()
        {
            for (var i = 0; i < Items.Count; i++)
            {
                // added this
                var updater = _factory.GetQualityUpdater(Items[i].Name);
                
                if (updater != null)
                {
                    updater.UpdateQuality(Items[i]);
                    continue; // we don't want the following code to execute 
                }
                if (Items[i].Name != "Backstage passes to a TAFKAL80ETC concert")
                {
                    if (Items[i].Quality > 0)
                    {
                        if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
                        {
                            Items[i].Quality = Items[i].Quality - 1;
                        }
                    }
                ... the code continues

So in the above code, I have only done the updater for “Aged Brie”, so it’ll return null if it’s not which means the existing production code will continue to execute.

I’m going to add the rest of the updaters now and see where we’re at.

Now the UpdateQuality method looks like this:

public void UpdateQuality()
{
    foreach (var item in Items)
    {
        var updater = _factory.GetQualityUpdater(item.Name);
        updater.UpdateQuality(item);
    }
}

Much, much better.

Want to see the complicated code for the SulfurasQualityUpdater? Well, here it is:

public class SulfurasQualityUpdater : IQualityUpdater
{
    public void UpdateQuality(Item item)
    {
        // it does NOTHING!
    }
}

Having SulfurasQualityUpdater do nothing whilst implementing IQualityUpdater means it violates the Interface Segregation Principle:

classes that implement interfaces should not be forced to implement methods they do not use.

So in the factory, I’ll return null in this case and conditionally call update.

Next, let’s try to add the new feature and see how much of a pain it is. If it’s a pain then we’ll consider some more refactoring. PDD, pain-driven development is a good indicator of when you should/shouldn’t refactor, if it’s a pain to add new features, then it probably needs some work. You don’t want to abstract everything because it can make the code overly complicated. Likewise, you don’t want to *not* abstract anything and have everything concrete because it’ll make the code difficult to extend (as seen in this Kata).

It shouldn’t be a pain though, all we need to do is add a new quality updater.

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

First, let’s add some tests, I did one test and then wrote enough code to get that test to pass, following TDD principles. But for the sake of the blog, I’ll show all the tests:

[Test]
public void Conjured_Item_Should_Degrade_Twice_As_Fast()
{
    var elixirOfMongoose = new Item
        { Name = "Conjured Mana Cake", SellIn = 6, Quality = 5 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(3, elixirOfMongoose.Quality);
}

[Test]
public void Conjured_Item_Should_Drop_4_In_Quality_If_SellIn_Past()
{
    var elixirOfMongoose = new Item
        { Name = "Conjured Mana Cake", SellIn = 0, Quality = 5 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(1, elixirOfMongoose.Quality);
}

[Test]
public void Conjured_Item_Quality_Should_Not_Drop_Below_0()
{
    var elixirOfMongoose = new Item
        { Name = "Conjured Mana Cake", SellIn = 0, Quality = 3 };
    var items = new List<Item> { elixirOfMongoose };
    var app = new GildedRose(items);
    app.UpdateQuality();

    Assert.AreEqual(0, elixirOfMongoose.Quality);
}

The new updater:

public class ConjuredItemUpdater : IQualityUpdater
{
    public void UpdateQuality(Item item)
    {
        item.SellIn--;
        var decrementValue = (item.SellIn >= 0) ? 2 : 4;
        item.Quality = Math.Max(0, item.Quality - decrementValue);
    }
}

Then all that’s needed is to add another line to the factory.

That was very simple to implement.

There’s just one issue with this code, it violates the single responsibility principle. The updaters are also updating the SellIn property.

So I could add a new method to the interface and rename the interface to IItemUpdater and have something like this:

public class ConjuredItemUpdater : IItemUpdater
{
    public void UpdateQuality(Item item)
    {
        var decrementValue = (item.SellIn >= 0) ? 2 : 4;
        item.Quality = Math.Max(0, item.Quality - decrementValue);
    }

    public void UpdateSellIn(Item item)
    {
        item.SellIn--;
    }
}

But it kind of feels a little redundant for everything to override UpdateSellIn when they’re all doing the same thing — apart from the Sulfuras, which does nothing — decrementing the SellIn.

Maybe I need an abstract class instead. Something like this:

public abstract class ItemUpdater
{

    public abstract void UpdateQuality(Item item);

    public virtual void UpdateSellIn(Item item)
    {
        item.SellIn--;
    }
}

So now a new item updater wouldn’t need to worry about updating SellIn unless it required some different behaviour.

Here’s how our ConjuredItemUpdater looks now. The only difference is the override keyword and it no longer decrements the sellIn property.

public class ConjuredItemUpdater : ItemUpdater
{
    public override void UpdateQuality(Item item)
    {
        var decrementValue = (item.SellIn >= 0) ? 2 : 4;
        item.Quality = Math.Max(0, item.Quality - decrementValue);
    }
}

Now I’m not particularly happy about the GildedRose class having to know to call the UpdateSellIn method then the UpdateQuality method:

public void UpdateQuality()
{
    foreach (var item in Items)
    {
        var updater = _factory.GetQualityUpdater(item.Name);
        updater.UpdateSellIn(item);
        updater.UpdateQuality(item);
    }
}

I’ve created a DailyItemUpdater class and updated the factory to be static. Now the DailyItemUpdater is responsible for calling UpdateSellIn and UpdateQuality:

public static class DailyItemUpdater
{
    public static void Update(Item item)
    {
        var updater = QualityUpdaterFactory.Create(item.Name);
        updater.UpdateSellIn(item);
        updater.UpdateQuality(item);
    }
}

Now the GildedRose class looks like this:

public class GildedRose
{
    private readonly IList<Item> _items;

    public GildedRose(IList<Item> Items)
    {
        this._items = Items;
    }


    public void UpdateQuality()
    {
        foreach (var item in _items)
        {
            DailyItemUpdater.Update(item);
        }
    }
}

Couldn’t get much simpler than that.

Now if the Gilded Rose decided to stock any more items it should just be a case of creating a new class ItemUpdater and add that line to the factory!

Comments

No comments yet. Why don’t you start the discussion?

Leave a Reply

Your email address will not be published. Required fields are marked *