Refactoring the Gilded Rose kata

The Gilded Rose kata is a famous refactoring challenge that really stretches your ability to understand and manipulate complicated logic. It can be found here.

This article will outline the steps needed to simplify the logic, without moving further into extracting methods, variables or classes.

Spoiler warning - if you intend to do the kata yourself it might be better to try it first before reading on.

def update_quality(self):
    for item in self.items:
        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
        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
                    if item.sell_in < 6:
                        if item.quality < 50:
                            item.quality = item.quality + 1
        if item.name != "Sulfuras, Hand of Ragnaros":
            item.sell_in = item.sell_in - 1
        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
                else:
                    item.quality = item.quality - item.quality
            else:
                if item.quality < 50:
                    item.quality = item.quality + 1

So here is what we are faced with. It's hard to make sense of, but giving it a once-over reveals that there's a list of items that we are iterating over, changing item.quality and item.sell_in based on the item.name. At this point it's not at all obvious what the rules for ding this are, or how we would go about changing them or adding new ones.

Manual refactoring is very dependent on having unit tests in place to ensure that we don't break anything, so we'll be running our suite of tests after every step to ensure that everything still works.

Sometimes you just have to start refactoring in order to get a feel for the structure of the code and the destination you should be aiming for. Let's roll up our sleeves and get started!

The for block is split into three if conditions - Let's look at the last one first. In the nested condition it checks if item.name != "Aged Brie". In general it's easier to understand a condition if the test is positive rather than negative. We will therefore start by inverting the condition, yielding this:

if item.sell_in < 0:
    if item.name == "Aged Brie":
        if item.quality < 50:
            item.quality = item.quality + 1
    else:
        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
        else:
            item.quality = item.quality - item.quality

Let's go ahead and invert the "Backstage passes" condition here too, for the same reason.

if item.sell_in < 0:
    if item.name == "Aged Brie":
        if item.quality < 50:
            item.quality = item.quality + 1
    else:
        if item.name == "Backstage passes to a TAFKAL80ETC concert":
            item.quality = item.quality - item.quality
        else:
            if item.quality > 0:
                if item.name != "Sulfuras, Hand of Ragnaros":
                    item.quality = item.quality - 1

An elif is easier to read than a nested if inside an else condition, so let's change this to an elif.

if item.sell_in < 0:
    if item.name == "Aged Brie":
        if item.quality < 50:
            item.quality = item.quality + 1
    elif item.name == "Backstage passes to a TAFKAL80ETC concert":
        item.quality = item.quality - item.quality
    else:
        if item.quality > 0:
            if item.name != "Sulfuras, Hand of Ragnaros":
                item.quality = item.quality - 1

This is starting to look a bit switch-like, since we are comparing the item.name to several different values in succession. These are quite easy to understand so let's see if we can massage the code a bit further to look like one. To do this we will swap the nesting on the "Sulfuras" and item.quality > 0 conditional.

if item.sell_in < 0:
    if item.name == "Aged Brie":
        if item.quality < 50:
            item.quality = item.quality + 1
    elif item.name == "Backstage passes to a TAFKAL80ETC concert":
        item.quality = item.quality - item.quality
    else:
        if item.name != "Sulfuras, Hand of Ragnaros":
            if item.quality > 0:
                item.quality = item.quality - 1

If we now invert the "Sulfuras" test to make it positive, we end up with this:

if item.sell_in < 0:
    if item.name == "Aged Brie":
        if item.quality < 50:
            item.quality = item.quality + 1
    elif item.name == "Backstage passes to a TAFKAL80ETC concert":
        item.quality = item.quality - item.quality
    else:
        if item.name == "Sulfuras, Hand of Ragnaros":
            pass
        else:
            if item.quality > 0:
                item.quality = item.quality - 1

Normally you wouldn't introduce an empty if body like this, but in this case it helps with our goal of turning the larger conditional into a switch-like statement. The next thing to do is to tidy this up into an elif again.

if item.sell_in < 0:
    if item.name == "Aged Brie":
        if item.quality < 50:
            item.quality = item.quality + 1
    elif item.name == "Backstage passes to a TAFKAL80ETC concert":
        item.quality = item.quality - item.quality
    elif item.name == "Sulfuras, Hand of Ragnaros":
        pass
    else:
        if item.quality > 0:
            item.quality = item.quality - 1

This is now much clearer - we can see what the code does for each of the different values of item.name. It's difficult to see what to do next with this bottom conditional, so let's move to the top one. Again this would be easier to understand as a positive condition, so let's invert it, using De Morgan's law that (not A and not B) is equivalent to not (A or B).

Before:

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
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
            if item.sell_in < 6:
                if item.quality < 50:
                    item.quality = item.quality + 1

After:

if item.name == "Aged Brie" or item.name == "Backstage passes to a TAFKAL80ETC concert":
    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
            if item.sell_in < 6:
                if item.quality < 50:
                    item.quality = item.quality + 1
else:
    if item.quality > 0:
        if item.name != "Sulfuras, Hand of Ragnaros":
            item.quality = item.quality - 1

Ah ha! This is also starting to look like it could be converted into a switch-like statement. We can split the top or condition into two, by duplicating the code like this:

if item.name == "Aged Brie":
    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
            if item.sell_in < 6:
                if item.quality < 50:
                    item.quality = item.quality + 1
elif item.name == "Backstage passes to a TAFKAL80ETC concert":
    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
            if item.sell_in < 6:
                if item.quality < 50:
                    item.quality = item.quality + 1
else:
    if item.quality > 0:
        if item.name != "Sulfuras, Hand of Ragnaros":
            item.quality = item.quality - 1

The previous step actually made the code longer, introducing duplication, so we wouldn't normally do it. However, now we can see that there's some redundant code that can be deleted. In the first block we know that the item.name == "Aged Brie", so everything inside the "Baskstage passes" conditional can go. Similarly in the second block we don't need the extra "Backstage passes" test since we know that it is true, so it can be removed.

if item.name == "Aged Brie":
    if item.quality < 50:
        item.quality = item.quality + 1
elif item.name == "Backstage passes to a TAFKAL80ETC concert":
    if item.quality < 50:
        item.quality = item.quality + 1
        if item.sell_in < 11:
            if item.quality < 50:
                item.quality = item.quality + 1
        if item.sell_in < 6:
            if item.quality < 50:
                item.quality = item.quality + 1
else:
    if item.quality > 0:
        if item.name != "Sulfuras, Hand of Ragnaros":
            item.quality = item.quality - 1

We can now repeat the same trick with the "Sulfuras" conditional that we did previously, since this code is identical to that we saw after step 3. This time we'll do it in one step, including merging it into an elif.

if item.name == "Aged Brie":
    if item.quality < 50:
        item.quality = item.quality + 1
elif item.name == "Backstage passes to a TAFKAL80ETC concert":
    if item.quality < 50:
        item.quality = item.quality + 1
        if item.sell_in < 11:
            if item.quality < 50:
                item.quality = item.quality + 1
        if item.sell_in < 6:
            if item.quality < 50:
                item.quality = item.quality + 1
elif item.name == "Sulfuras, Hand of Ragnaros":
    pass
else:
    if item.quality > 0:
        item.quality = item.quality - 1

It's time to take stock of where we've got to. Here's the whole piece of code - the only change we've made here is to replace all of the increments and decrements to item.quality and item.sell_in with augmented assignments.

def update_quality(self):
    for item in self.items:
        if item.name == "Aged Brie":
            if item.quality < 50:
                item.quality += 1
        elif item.name == "Backstage passes to a TAFKAL80ETC concert":
            if item.quality < 50:
                item.quality += 1
                if item.sell_in < 11:
                    if item.quality < 50:
                        item.quality += 1
                if item.sell_in < 6:
                    if item.quality < 50:
                        item.quality += 1
        elif item.name == "Sulfuras, Hand of Ragnaros":
            pass
        else:
            if item.quality > 0:
                item.quality -= 1
        if item.name != "Sulfuras, Hand of Ragnaros":
            item.sell_in -= 1
        if item.sell_in < 0:
            if item.name == "Aged Brie":
                if item.quality < 50:
                    item.quality += 1
            elif item.name == "Backstage passes to a TAFKAL80ETC concert":
                item.quality = item.quality - item.quality
            elif item.name == "Sulfuras, Hand of Ragnaros":
                pass
            else:
                if item.quality > 0:
                    item.quality -= 1

Looking over the whole function we can now see that no updates get made when the item.name == "Sulfuras, Hand of Ragnaros". Let's extract this out as a guard clause.

def update_quality(self):
    for item in self.items:
        if item.name == "Sulfuras, Hand of Ragnaros":
            continue
        if item.name == "Aged Brie":
            if item.quality < 50:
                item.quality += 1
        elif item.name == "Backstage passes to a TAFKAL80ETC concert":
            if item.quality < 50:
                item.quality += 1
                if item.sell_in < 11:
                    if item.quality < 50:
                        item.quality += 1
                if item.sell_in < 6:
                    if item.quality < 50:
                        item.quality += 1
        else:
            if item.quality > 0:
                item.quality -= 1
        item.sell_in -= 1
        if item.sell_in < 0:
            if item.name == "Aged Brie":
                if item.quality < 50:
                    item.quality += 1
            elif item.name == "Backstage passes to a TAFKAL80ETC concert":
                item.quality = item.quality - item.quality
            else:
                if item.quality > 0:
                    item.quality -= 1

Now we can see that there is an update to item.sell_in in the middle of this function. It's sitting in between two sets of updates to item.quality, and it would be better to update one variable at a time. Let's move the update to the end of the function. We'll have to alter the item.sell_in conditional as we move past.

def update_quality(self):
    for item in self.items:
        if item.name == "Sulfuras, Hand of Ragnaros":
            continue
        if item.name == "Aged Brie":
            if item.quality < 50:
                item.quality += 1
        elif item.name == "Backstage passes to a TAFKAL80ETC concert":
            if item.quality < 50:
                item.quality += 1
                if item.sell_in < 11:
                    if item.quality < 50:
                        item.quality += 1
                if item.sell_in < 6:
                    if item.quality < 50:
                        item.quality += 1
        else:
            if item.quality > 0:
                item.quality -= 1
        if item.sell_in < 1:
            if item.name == "Aged Brie":
                if item.quality < 50:
                    item.quality += 1
            elif item.name == "Backstage passes to a TAFKAL80ETC concert":
                item.quality = item.quality - item.quality
            else:
                if item.quality > 0:
                    item.quality -= 1
        item.sell_in -= 1

Now we have two if conditionals sitting next to each other that look very similar in structure, but aren't quite the same. If we swap the nesting of conditionals in the second one, moving the item.sell_in < 1 test inside we get the following:

def update_quality(self):
    for item in self.items:
        if item.name == "Sulfuras, Hand of Ragnaros":
            continue
        if item.name == "Aged Brie":
            if item.quality < 50:
                item.quality += 1
        elif item.name == "Backstage passes to a TAFKAL80ETC concert":
            if item.quality < 50:
                item.quality += 1
                if item.sell_in < 11:
                    if item.quality < 50:
                        item.quality += 1
                if item.sell_in < 6:
                    if item.quality < 50:
                        item.quality += 1
        else:
            if item.quality > 0:
                item.quality -= 1
        if item.name == "Aged Brie":
            if item.sell_in < 1:
                if item.quality < 50:
                    item.quality += 1
        elif item.name == "Backstage passes to a TAFKAL80ETC concert":
            if item.sell_in < 1:
                item.quality = item.quality - item.quality
        else:
            if item.sell_in < 1:
                if item.quality > 0:
                    item.quality -= 1
        item.sell_in -= 1

Now the if's look identical, and we can merge them together.

def update_quality(self):
    for item in self.items:
        if item.name == "Sulfuras, Hand of Ragnaros":
            continue
        if item.name == "Aged Brie":
            if item.quality < 50:
                item.quality += 1
            if item.sell_in < 1:
                if item.quality < 50:
                    item.quality += 1
        elif item.name == "Backstage passes to a TAFKAL80ETC concert":
            if item.quality < 50:
                item.quality += 1
                if item.sell_in < 11:
                    if item.quality < 50:
                        item.quality += 1
                if item.sell_in < 6:
                    if item.quality < 50:
                        item.quality += 1
            if item.sell_in < 1:
                item.quality = item.quality - item.quality
        else:
            if item.quality > 0:
                item.quality -= 1
            if item.sell_in < 1:
                if item.quality > 0:
                    item.quality -= 1

        item.sell_in -= 1

This is looking much more understandable, but can still be improved. There is an opportunity to reduce some nesting. In the "Backstage passes" section the nested ifs that check item.sell_in will only update item.quality if item.quality < 50. They can therefore be hoisted out of their containing condition.

def update_quality(self):
    for item in self.items:
        if item.name == "Sulfuras, Hand of Ragnaros":
            continue
        if item.name == "Aged Brie":
            if item.quality < 50:
                item.quality += 1
            if item.sell_in < 1:
                if item.quality < 50:
                    item.quality += 1
        elif item.name == "Backstage passes to a TAFKAL80ETC concert":
            if item.quality < 50:
                item.quality += 1
            if item.sell_in < 11:
                if item.quality < 50:
                    item.quality += 1
            if item.sell_in < 6:
                if item.quality < 50:
                    item.quality += 1
            if item.sell_in < 1:
                item.quality = item.quality - item.quality
        else:
            if item.quality > 0:
                item.quality -= 1
            if item.sell_in < 1:
                if item.quality > 0:
                    item.quality -= 1

        item.sell_in -= 1

Let's make a final pass and reduce nesting a little by merging the nested if conditionals. Also item.quality - item.quality is equivalent to 0.

def update_quality(self):
    for item in self.items:
        if item.name == "Sulfuras, Hand of Ragnaros":
            continue
        if item.name == "Aged Brie":
            if item.quality < 50:
                item.quality += 1
            if item.sell_in < 1 and item.quality < 50:
                item.quality += 1
        elif item.name == "Backstage passes to a TAFKAL80ETC concert":
            if item.quality < 50:
                item.quality += 1
            if item.sell_in < 11 and item.quality < 50:
                item.quality += 1
            if item.sell_in < 6 and item.quality < 50:
                item.quality += 1
            if item.sell_in < 1:
                item.quality = 0
        else:
            if item.quality > 0:
                item.quality -= 1
            if item.sell_in < 1 and item.quality > 0:
                item.quality -= 1
        item.sell_in -= 1

There we go! Now it's much clearer what the function does. There's still more tidying up that could be done by extracting magic numbers and string literals. The next step after that might be extracting functions like the increment to item.quality only if it is less than 50. We could also move to using polymorphism based on the item.name.

Now that we've seen how long it takes to do this manually - here's how you can do the same thing with Sourcery!

Refactoring Gilded Rose with Sourcery

Refactoring the Gilded Rose kata

See Sourcery in action

Product

Integrations

Resources

Company

COMMUNITY