Often getting legacy code into a state where you're comfortable making changes to it can be challenging. Legacy code can often have varying levels of quality throughout the codebase, as well as different levels of testing applied to it. I've taken over my fair share of old projects that have no tests, or haven't followed SOLID principles, and understanding how the code works and what it does can be a challenge.
The Gilded Rose Kata is an exercise in taking a system that calculates "quality" and expiry dates for items in a shop over time. When you start the Kata, you have a single method that calculates the changes in quality and expiry for the items. The business logic implemented is hard to understand, and features nested if statements with conditionals that feel inverted. You also need to add functionality to support a new item type to the shop to complete the Kata.
This post will describe how I refactored the code. All code and commits are available at https://github.com/rickerbh/GildedRose-Refactoring-Kata-Swift
When refactoring, you're typically looking to improve the internal structure of the code without changing the externally observable behaviour of the system. To ensure the existing behaviour didn't change whilst I was refactoring, I followed the advice in the readme that recommends "Text-Based Approval Testing". This is essentially an acceptance test in which the behaviour of the system is captured before you start refactoring, and then while refactoring you can compare the changed systems' output with the original output.
To implement this, I captured the text output from running the application in a file and stored it in the repo. I then implemented a test that would replicate running the application, but storing the results in memory. Then the test compared the results of my application with the output from the original application. As an aside here, when I first implemented this I did a comparison of the full body. I quickly found this to be a terrible solution, and then implemented a line-by-line comparison so I could easily tell what part of the calculations were going wrong. Also, I found an issue with the original project setup, so submitted an (accepted) PR to the original repo that corrected the store definintion.
I've recently read Working Effectively With Legacy Code by Michael Feathers and there was one approach in this book that seemed to jump out at me to help understand the Gilded Roses logic - Sprout Method. This is where you take a small block of existing functionality (e.g., an if statement), extract it out to it's own function, and give it a sensible name. Then, rather than seeing a block of code such as
if (items[i].name != "Sulfuras, Hand of Ragnaros") {
items[i].sellIn = items[i].sellIn - 1
}
you see something like
decrementSellDate(item[i])
Example of this here.
The behaviour of the system doesn't change, but suddenly it becomes more readable. Each "sprouted" method is also accompanied by a test. This allows you to also validate that your extracted code will then continue to work as expected, even with other changes taking place.
For this refactor, I continued to write tests and extract methods to get the giant block of if statements to a point where I could understand what they were doing.
After this, I considered the if statement itself. It used a bunch of negation statements to control the flow of execution through the application e.g., if (items[i].name != "Aged Brie" && items[i].name != "Backstage passes to a TAFKAL80ETC concert"). Personally, I find this style of if statement creates a higher congnitive load that "positive" statement, so I restructured it to use statements more like if (items[i].name == "Aged Brie" || items[i].name == "Backstage passes to a TAFKAL80ETC concert"). Maybe that's a personal thing, but I find it simpler to understand - being explicit about what the predicate matches, rather than "everything but these items". During this activity, having the acceptance tests run was crucial to ensure the system behaved the same, while the statements were changing.
After this, I extracted one more method for managing expiry dates. I felt that I had a good understanding of how the existing system worked and the logic was clear in the codebase. Time for some bigger changes.
One of the key restrictions in the Gilded Rose Kata is that you're not allowed to alter the Items class or the Items property. Not having this restriction would have made the refactor relatively trivial as you could just create different subclasses that represent the behavour of each of the types of items. To do this however, would have involved altering the Items property to instantiate the different item types, or turning the Items class into a factory that would return items of different types. So, I needed a way to move the rules for specific types of items away from the main calculation flow, so that new item types could be introduced without impacting the existing processes.
I decided to add an ItemBehaviour class that would act as a superclass for each of the items to encapsulate the rules/logic for that specific item. The goal was to separate out the process of incrementing a day in the store, and how each item changes as the days increment. Then I created subclasses (Brie, Sulfuras, BackStagePass) to override the default behaviour for each item that has unique logic. To surface this behaviour, a BehaviourFactory class was implemented that would return an ItemBehaviour based on the items name (not the approach I would have liked to implement, but not being allowed to change the Item class is quite a restriction).
The behaviour was then integrated into the main application flow by replacing the if-statement/logic that takes place before the date change with a single call to updateQualityPreDateChange.
Next, the functionality to decrement the sales date was extracted from the sprouted method, and placed into the specific item behaviour classes. And finally, the changes to process expired items was extracted out into the item behaviour classes.
The main body of the store updating function has been dramatically simplified. It now maps through all the items, retrieves the behaviour for an item, and calls the functions to advance the item through a day. Simples.
let _ = items.map { item in
let itemBehaviour = BehaviourFactory.getBehaviour(item)
itemBehaviour.updateQualityPreDateChange(item)
itemBehaviour.decrementSellDate(item)
itemBehaviour.processExpiredItem(item)
}
I believe that's significantly easier to understand than the original code.
Now that the item behaviour is decoupled from the main day-end processing, adding a new item type is trivial. The new type gets added and then integrated into the factory. The only other thing that needs to happen is the old acceptance test needs to be updated, as this item didn't exist in the original set. Ideally at this point the acceptance test would be updated (or removed) as the system behaviour has moved on, but for the purposes of this exercise I excluded the new item from the test.
Legacy System maintenance is hard, but not impossible. You can always get a piece of business logic to be able to be tested, and restructure messy code so it's simple and understandable. Techniques such as sprout method make gaining initial understanding and making code a bit more clear relatively easy to perform. Working Effectively With Legacy Code was a super-valuable resource, and covers many, many techniques to get an unwieldy codebase under control, both in terms of gaining understanding as well as getting code testable.
Personally, I found this kata a valuable experience in getting an existing system under control, whilst maintaining existing behaviour. I recommend if you have a spare hour or so give it a go yourself.