16 steps to transform and simplify the code
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!
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
"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
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
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
"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
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
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
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
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
"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
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
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
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
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
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
"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
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!