I’ve heard of the Gilded Rose Kata by accident, looking for code exercises to train on. Unlike other katas, this kata was all about refactoring and design. The beautiful thing about this sort of exercises is that there’s no one and only clear solution. Instead, every design solution is debatable, has its pros and cons - that way solving a coding challenge becomes a debate about ideas rather than technicalities.

Once I’ve found this exercise, I decided to tackle it right away. Overall, it took me about an hour to achieve a solution I feel confident about, and another hour over engineering the solution with C# reflection. While wrapping up the code (making sure the style is coherent) I noticed some more changes I could have introduced that would make the code even cleaner. Since I already got I wanted from this solution, It made a lot of sense keeping it that way. This is not production code and any further expansions would’ve been mostly cosmetic.

I’ve cloned the kata from here (some background about the kata can be found here). The kata in any language other than C# can be found here. A reference to my solution can be found here.

Let’s get to business.

First Step - tests, tests, tests

The GildedRose code is a procedural, tangled mess. It is clear that playing around with this code as is will easily lead to bugs in the final implementation. In this Kata, we are guaranteed a very important detail that is that the code Correct. We can therefore add tests, capture the observed behavior, and know instantly whether we are deviating from it. Another big plus is that we may be tempted to change the public API of this code - we might make observation that will incentivize us to change how this code interacts with its environment. However, this is strict disallowed in this Kata because we are making a refactoring that should not reflect outside in any way. Tests help us achieve that as they capture and lock the behavior as well as the API of this code.

Tests allow us to understand very clearly what the code does what it does not, and more times than not it is a better indicator than the code itself. Actually, when writing the tests I didn’t even look directly at the UpdateQuality method but rather on the textual requirements of the system. Here’s an example

Second Step - removing clutter

It was the time to finally get to the refactoring. The first thing I changed was some clutter and mess along the UpdateQuality method. I changed the for loop into a foreach loop, I removed excessive nesting and conditionals, and tried to make the conditions in the if statements to be more concise. The last meaningful thing in this very shallow refactoring is that I was able to change the order of the if statements, and pulled up all the conditionals that were related to the item type. Later, I converted the messy if statements into if-else if-else statements as much as I could without changing the code too much. This will give a clear structure to the code and allow me to extract the nested code into its proper place in the future.

Third Step - redesign: utilizing polymorphism

Once I had a clear function - it was clear that the UpdateQuality method was doing way too much. However, it is clear that in an abstract level all that UpdateQuality does is go over a list of items and update their properties accoring to their type. Therefore, I am looking for a function structure that involves basically one loop and calls update on each individual file. We then reduce the problem to an individual UpdateQuality method per item. Here, there are many approaches on how we could slice up this big of a decision tree, varying from a more procedural and data driven approach, to a functional pattern-matchy approach all the way to object oriented polymorphism. It looked like we could get a clear separation of concerns if all the items will be able to update themselves, therefore polymorphism seems like a decent direction to go. Once the items are polymorphic instances, the UpdateQuality function doesn’t have to know the concrete type of the items it updates, removing all the dependencies and the business logic from the main method and putting it in polymorphic instances. I still needed to find a way to decide on the implementation for each input item, but I left that problem for later.

Since I couldn’t change the Item type I used an IItemUpdater interface, that its implementations will wrap the items and modify them according to the expected behavior. The item updater defined a method Update. After all this change, the UpdateQuality looked as follows:

Now its clear what this method does! All is left is to implement the GetUpdater method and I’m done.

Fourth Step – creating the Item updaters

After I’ve extracted all the updating logic from the main method to the updater implementations, I had to find a way to create the right updater for each item. Every item updates differently, while the items differ by name. I first created a mapping from names to updaters, that looks as follows:

That way, the GetUpdater only checks for every item whether it is in the above dictionary. If so, it creates the updater using reflection, and if not, it simply creates a regular item updater.

This flow is much clearer, but it violates the open-closed principle and the single responsibility principle – In order to add a new updater type, the main method (GetUpdater) needs to know about it. I have a dependency from the main method to the updater implementations.

Fifth step – removing dependencies

Right now, all the updaters have to explicitly referenced to be used in UpdateQuality method. In order to overcome this, I’ve decided to convert the compile-time dependencies to run-time dependencies, using reflection. First, I created a CustomItemUpdaterAttribute, that marks the updater classes and link to the corresponding item names.

I then decorated each custom updater class with its attribute

This allowed me to create the mapping dynamically, as follows:

After this change, in order to add a new custom item update behavior, all it takes is to create a class that implements the IItemUpdater interface and is decorated with the attribute I created, and voila! The updater is already in use by the UpdateQuality method. The dependencies moved from compile-time to run-time and the design does not violate the open-close principle any more.