16 steps to transform and simplify the code
Jul 10, 2019
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!