The Gilded Rose Refactoring Kata:
my personal refactoring work on the application
In this blog post I’m going to write about my first attempt to refactor the Gilded Rose application. The Gilded Rose is a refactoring Kata and the purpose of it is to enhance your refactoring skills (if you have a look at the application you’ll soon find out why the application hugely needs to be refactored…).
The Kata comes in different languages and I chose the one in Java language. To know more about the Gilded Rose application and how it works you can read this.
First look at the application
The first thought I had when I saw the application was: WOOH! The application is made of 2 classes (the Gilded Rose class and the Item class). The Gilded Rose class has a method updateQuality() which is a masterpiece of way-too-long-all-nice-and-wavy method, at first glance. It then contains a lot of nested if-statements, repetitions, very long and complicated hard-coded strings, and small operations that are not explanatory or clear about what they’re doing at all… already good material to work on.
The class Item has 3 fields, name, sellIn and quality, which define each of the item types. It also implements a method toString() which is not used anywhere, really.
The application comes with a GildedRoseTest class with one failing test as well, ready to be ‘populated’ with further tests.
Requirements/constraints
The application is already up-and-running based on some requirements and specific behaviour for each of the item types the Inn sells (you can have a look at the description of the application in the link I’ve provided above.)
According to the Kata, it is possible to change the updateQuality() method in GildedRose class or to add more code to the application. The only 2 constraints are:
- do not alter the
Itemclass - do not alter the
Itemsproperty.
First step: understanding how the application works
The first thing I immediately thought I should do was try to understand how the application worked and what was happening in each line of the method updateQuality(). So I analysed each line and added a comment above it (the below shows just part of the result):
Second step: adding more tests
Once I had properly understood the method, I started writing tests for each of the behaviour expected from every single item each time the method was called (each item type updates its quality day by day in its peculiar way, generally depending on the sell-by-day value; in a single case the quality doesn’t alter at all.)
Surprisingly, each of the tests I created passed. This was the proof that the method — although not at all respecting the general rules of good and clean coding… — was really working!
Third step: starting refactoring
Now that I had a good suite of tests I felt confident enough to change the code and make it better. I have to admit that the start hasn’t been easy at all, at the contrary quite slow and ‘intimidating’: I wasn’t quite sure on how to start (although I tried to keep in mind Uncle Bob’s blog post about the Transformation Priority.)
The first thing that I actually changed was the for-loop so that instead of setting the 3 expressions ( initialization, termination and increment expressions) it would just iterate for each of the items:
for (Item item : items) {
// do something
}Although this isn’t a huge refactoring, it already adds more readability and clarity to the code as shown below:
if (item.name.equals("Aged Brie") {
// do something
}instead of:
if (items[i].name.equals("Aged Brie") {
}The next step wasn’t easy at all. As I mentioned above, the original code has plenty of repetitions, so next I tried to work on eliminating some duplications by extracting them into a method.
So, for example I extracted if (item.quality < 50) into something like:
private boolean isUnderHighestQualityValue(Item item) {
return item.quality < 50;
}, and all other similar repetitions (like the operation item.quality + 1 and so forth.)
I then replaced the names of the items in the code in the form of strings (some of them definitely long and complicated!!) with constants.
All of the above refactorings might seem premature at this early stage (maybe I should have done those at the very end of the refactoring process) but, as I said, I felt rather confused by the complexity and the unclarity of the original code that — I admit — I started with the most easy and straightforward refactorings…
Fourth step: things get more complicated
At this point the updateQuality() function already seemed more polished and clear to me! But of course I couldn’t not notice all the many if and else keywords everywhere in the method…
I noticed that some of the items were repeated in the code with different if conditions. That was a sign, a code smell that something was wrong, and that maybe all of those conditions could be grouped into just one single line — or a single method — for each of the items, and each item referred to just once in the method. How it comes that a line like if(!item.name.equals("AgedBrie") appeared twice in line 12 and line 44?
Intuitively I thought I had to ‘group together’ all the conditions for each of the items and avoid having a bunch of duplicated if- and else-statements in the method updateQuality().
** I feel like I have ‘cheated’ and skipped a lot of steps here as I have moved forward more according to ‘intuition’ and attempts rather than following each of the steps of the refactoring process as described by Uncle Bob’s blog post, but yeah, this is how I proceed… :)**
So the result of my next refactoring was something like this (please note I’m not providing the actual code here as, unfortunately, I didn’t commit this change — damn; besides, Christmas Holidays have been in-between and I’ve forgotten how I actually wrote my code…….):
public void updateQuality() {
for(Item item : items) {
if (item.quality < 50 && item.quality > 0) { //quality cannot
be above 50 or
below 0
if (item.name.equals(AGED_BRIE)) {
doSomethingToSellByDayValue;
doSomethingToQualityValue;
} else if (item.name.equals(ANOTHER_ITEM)) {
doSomethingToSellByDayValue; //different output
from above
doSomethingToQualityValue; //different output
from above
}
// more else-if statements with different outputs
for each of the remaining items
}
}
}Although the code above worked just fine (more or less,) it showed one big issue: it didn’t respect the Open-Closed Principle. If I needed to add a further item I had to add a new behaviour for the new item, different from those of the items before (remember, items update their quality value differently.) The function wasn’t flexible enough. Besides, the quality for the Sulfuras item had a different condition: it had to be 80.
Because of all the above, the function still
- wasn’t flexible enough for changes
- was too long
- showed some avoidable duplication
- had too many
if-statements(and the list could potentially still increase…)
Fifth step: BIG REFACTORING
At this stage something was clear and obvious to me: all the above conditions just confirmed to me that I needed to use polymorphism.
In order to achieve this I applied a class refactoring and created a class for each of the item types. Then I defined a method updateState() in an interface which I called CustomisedItem and then implemented that method differently for each of the item classes, depending on their expected behaviour:
public void updateQuality() {
for (Item item : items) {
customisedItem(item).updateState();
// some code that controls the quality state
}
}private CustomisedItem customisedItem(Item item) {
return new CustomisedItemFactory(item).customiseItem(item);
}
As shown above, I also used a Factory Class which I called CustomisedItemFactory and where the method customiseItem() returned a new CustomisedItem depending on the Item object passed to the method as argument:
public class CustomisedItemFactory {
private final static Map<String, CustomisedItem>
ITEM_TYPES_LIST = new HashMap<>();
public final static String SULFURAS =
"Sulfuras, Hand of Ragnaros";
public final static String BRIE = "Aged Brie";
public final static String BACKSTAGE_PASSES_ITEM =
"Backstage passes to a TAFKAL80ETC concert";
public final static String CONJURED_ITEM = "Conjured";
public CustomisedItemFactory(Item item) {
ITEM_TYPES_LIST.put(SULFURAS, new Sulfuras());
ITEM_TYPES_LIST.put(BRIE, new AgedBrie(item));
ITEM_TYPES_LIST.put(BACKSTAGE_PASSES_ITEM, new
BackstagePassesItem(item));
ITEM_TYPES_LIST.put(CONJURED_ITEM, new ConjuredItem(item));
}
public CustomisedItem customiseItem(Item item) {
if (isStandardItem(item)) {
return new StandardItem(item);
}
return ITEM_TYPES_LIST.get(item.name);
}
private boolean isStandardItem(Item item) {
return !ITEM_TYPES_LIST.keySet().contains(item.name);
}
}Final result
So, below you can see how my class GildedRose eventually looked like:
class GildedRose {
private static final int LOWEST_QUALITY_VALUE_POSSIBLE = 0;
Item[] items;
public GildedRose(Item[] items) {
this.items = items;
}
public void updateQuality() {
for (Item item : items) {
customisedItem(item).updateState();
if (hasReachedLowestQualityValue(item)) {
item.quality = LOWEST_QUALITY_VALUE_POSSIBLE;
} else if (hasReachedHighestQualityValue(item)) {
item.quality =
QualityValues.highestValuePossible(item);
}
}
}
private CustomisedItem customisedItem(Item item) {
return new CustomisedItemFactory(item).customiseItem(item);
}
private boolean hasReachedLowestQualityValue(Item item) {
return item.quality < LOWEST_QUALITY_VALUE_POSSIBLE;
}
private boolean hasReachedHighestQualityValue(Item item) {
return item.quality >
QualityValues.highestValuePossible(item);
}
}The lines of code in bold represent how the quality value for each of the items is altered after each call. All of the items cannot decrease their quality below value 0. Most of the items (Sulfuras excluded) can increase their quality value up to 50; Sulfuras has quality 80 (and it cannot change.)
To achieve this kind of differentiation between the 2 possible values above I created a final class QualityValues which defines the 2 possible highest quality values in a static method highestValuePossible(). This way all items can update their quality in their peculiar way in the same method updateQuality().
Conclusion
I’m quite satisfied with the work I did although I feel like I missed some of the intermediate refactoring steps (as I already mentioned above.)
I had to rush a little when describing each step of my work here so please forgive me if this blog post is not accurate or clear enough! To have a more clear picture of all the code you can have a look at my Github repository.
I haven’t done a great work when it comes to making commits and pushing them to Github. A more accurate work in this sense would have been helpful to describe in more details and to clearly show what I did step by step… next time ;)
