How to refactor GildedRose-Refactoring-Kata with Simple Factory Pattern and Strategy Pattern

I found an interesting project — GildedRose-Refactoring-Kata on GitHub. It provides a starting code for refactoring exercise in a bunch of programming languages. The principle is we should create enough unit tests that can cover all cases(especially for the edge cases), then start to refactor the code. Let us try to do some exercises.

The original project has a huge method that contains lots of if else blocks to update the Item properties. Regarding the refactoring, there is a limit based on the requirement - "do not alter the Item class or Items property". That is kind of trap because my first thought is to use the simple factory pattern, which means we need to modify the Item class as an abstract class, then create respectively derived classes for different items. Anyway, let us see what we could do if we are allowed to modify the Item class.

Simple Factory Pattern

The Simple Factory Pattern might be one of the most widely used patterns. There is a factory object for creating some other objects so we do not need to use new keyword to frequently create different objects in our code. The objects created by the factory should have the same parent class but they perform different tasks according to their types. It is easy to use and it works for lots of scenarios. The "easiest" does not mean it is not good enough. We could find the balance between the delivery and the time-cost.

For this case, the Item class would be the base class. I can add an abstract method to calculate the SellIn and Quality:

public abstract class Item
{
    public string Name { get; set; }
    public int SellIn { get; set; }
    public int Quality { get; set; }
​
    protected Item(string name, int sellIn, int quality)
    {
        Name = name;
        SellIn = sellIn;
        Quality = quality;
    }
    public abstract void Update();
}

The Item class now is abstract so it cannot be instantiated directly. Now we can create derived classes to update these properties for different types. For simplicity, I will create a StandardItem type and an AgedBrie type as examples, as shown below:

public class StandardItem : Item
{
    public StandardItem(string name, int sellIn, int quality) 
        : base(name, sellIn, quality) { }
    public override void Update()
    {
        SellIn = SellIn - 1;
        Quality = Quality - 2;
        if (Quality < 0)
        {
            Quality = 0;
        }
        else if(Quality > 50)
        {
            Quality = 50;
        }
    }
}
​
public class AgedBrie : Item
{
    public AgedBrie(string name, int sellIn, int quality) 
        : base(name, sellIn, quality)
    {
    }
​
    public override void Update()
    {
        SellIn = SellIn - 1;
        Quality = Quality + 1;
        if (Quality < 0)
        {
            Quality = 0;
        }
        else if (Quality > 50)
        {
            Quality = 50;
        }
        //The above validation can be refactored as well
    }
}

But here we still see some duplicated code when retrieving the value of the Quality. So the better way is to move these validations to the getter method of the Quality property in the Item base class, or we could have a default implementation in the base class.

By creating these derived classes, we can extract the logic from the big GildedRose class to the derived classes. Next we need to have a factory to create them.

public class ItemFactory
{
    public static Item CreateItem(string name, int sellIn, int quality)
    {
        switch (name)
        {
            case "Aged Brie":
                return new AgedBrie(name, sellIn, quality);
            ...
            default:
                return new StandardItem(name, sellIn, quality);
        }
    }
}

The next step is to update the Main method in Program class:

IList<Item> Items = new List<Item>{
    //Call the factory to create the derived classes:
    ItemFactory.CreateItem("+5 Dexterity Vest", 10, 20),
...

We use the factory to create the items. Now update the UpdateQuality method in the GildedRose class:

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

Because each derived item has its own method to update the properties, so we can get the correct values.

But, what if we cannot modify the Item class?

One variation is using the interface to extend the behavior of the class. We do not need to add the Update method in the Item class. Instead, we can create a new interface which provides the Update method. So the derived classes could implement this interface to update its properties:

public interface IUpdate
{
    void UpdateSellIn();
    void UpdateQuality();
}
​
public class AgedBrie : Item, IUpdate
{
    ...
}

The derived classes should also implement the IUpdate interface. So the basic logic does not change much, just need to change the UpdateQuality method respectively:

public void UpdateQuality()
{
    foreach (var item in Items)
    {
        ((IUpdate)item).UpdateSellIn();
        ((IUpdate)item).UpdateQuality();
    }
}

For some C# interviews, one common question is “the differences between the abstract class and the interface”. Basically, the typical answer could be:

• The abstract class can have access specifier for functions but the interface cannot. In other words, you can use private, public, etc for the internal functions but in the interface, all functions are public by default.
• The abstract class can have default implementations but the interface cannot.
• The abstract class can have fields and constants but the interface cannot have fields.
• The abstract class can have non-abstract methods but the interface cannot.
• The abstract class can have the constructor but the interface cannot.
• One class can only derive from one abstract class but it can implement multiple interfaces.
• Both of them cannot be instantiated.
• …

Note: Please keep in mind that “the interface cannot have implementation” is not correct today because the interface supports default implementations from C# 8.0! Also, there are more changes to the interface since C# 8.0: The interface in C# 8.0 can have private, static, protected, virtual members. Maybe this interview question could be updated. FYI: Default implementations in interfaces and Tutorial: Update interfaces with default interface methods in C# 8.0.

So when should we use the abstract class or the interface? From my understanding, the answer is “it depends”. The abstract class is used to define the actual identity of the objects which have logical inheritance relations but the interface is more likely used to extend the behaviors of the objects. In the dependency injection mode, we mainly use interfaces.

That’s kind of digression. Let us return to the refactoring. To be honest, the variation of the factory pattern is not good enough — because it makes confusing regarding the object and the behavior. Actually we should make an abstraction of the method that is used to calculate the properties. But the item itself should be the same. Let us move on.

Strategy Pattern

There are categories of the design patterns:

• Creational Patterns — to create various objects
• Structural Patterns — to assemble objects into larger structures
• Behavioral Patterns — more focus on the algorithms and the assignment of responsibilities between objects.

For this practice, we could use one of the Behavioral Patterns — Strategy Pattern to decouple the calculation logic and the objects. This pattern allows us to have multiple algorithms without changing the original Item class.

First, we need to define an interface that declares a method to execute a strategy:

public interface IStrategy
{
    (int sellIn, int quality) Update(Item item)
}

If we use C# 8.0, we can have the default implementation here, eg. to reset the value of SellIn and validate the value of Quality.

Then we will define the concrete strategies:

public class StandardStrategy : IStrategy
{
    public (int sellIn, int quality) Update(Item item)
    {
        var quality = item.Quality - 2;
        ...
        return (item.SellIn - 1, quality);
    }
}
​
​
public class AgedBrieStrategy : IStrategy
{
    public (int sellIn, int quality) Update(Item item)
    {
        var quality = item.Quality + 1;
        ...
        return (item.SellIn - 1, quality);
    }
}

Then we need to create a Context class to define the interface of interest to clients:

public class StrategyContext
{
    private readonly IStrategy _strategy = null;
​
    public StrategyContext(string name)
    {
        switch (name)
        {
            case "Aged Brie":
                _strategy = new AgedBrieStrategy();
                break;
            ...
            default:
                _strategy = new StandardStrategy();
                break;
        }
    }
​
    public (int sellIn, int quality) Update(Item item)
    {
        return _strategy.Update(item);
    }
}

Next we can apply the strategies in the UpdateQuality method in GildedRose class:

public void UpdateQuality()
{
    foreach (var item in Items)
    {
        var context = new StrategyContext(item.Name);
        var result = context.Update(item);
        item.SellIn = result.sellIn;
        item.Quality = result.quality;
    }
}

This time we did not change the Item class and the Main method in Program class. If you think the new keyword is not beautiful here, you can continue to use the Factory Pattern to decouple the logic used to create the correct strategy to another part. But now we have already decoupled the calculation behavior from the object itself to another class.

Summary

My repo can be found here: https://github.com/yanxiaodi/GildedRose-Refactoring-Kata-Solutions. Because I did not create enough unit tests so there is no guarantee for the results. The demo is to show how to apply these design patterns, not the specific implementation. When we start to refactor the code, the most important thing is to create enough unit tests to make sure the refactoring would not break the current functions. The original project already has a unit test sample so we can add more tests to cover those **edge cases**. It is not easy to make 100% cover but it is worth doing.

Obviously, I do not think someone would write code like the original project. One thing I always think about is when we have a new requirement, how should we design it by using some complex design pattern or just if else? In other words, sometimes the client might just ask you to implement the function as soon as possible because of the budget limit without any care regarding your underlying implements. Let us say we have two options:

• Developer A is a newbie who can complete the function in 2 hours but just using if else. However, it indeed works as expected.
• Developer B is more experienced but needs more time to design the pattern so the time-cost would be 4 hours. But it would be easy to add more features in the future.

How would you choose?