What I’ve learned by doing The Gilded Rose Kata (4 refactoring tips)
A while ago I found great presentation on code refactoring called “All the little things” from Sandi Metz. The presentation was based on an exercise called The Gilded Rose Kata. It inspired me to play with the kata and here are some after thoughts. For those of you that like to get your hands dirty, I’ve also included a few code examples to help you get started with your own kata exercise.
What is the The Gilded Rose Kata?
Let me first start with explanation of what a code kata actually is. It’s an exercise which helps programmers improve their skills through practice and repetition.
The Gilded Rose Kata is all about two classes Item and GildedRose that you should refactor. Item has name, sell_in and quality attributes. GildedRose class has update_quality method responsible for decreasing sell_in and updating the quality attributes for each item.
The code is messy and has a lot of if statements that need to be resolved. Rules… hmm, they are pretty clear. Let’s get more familiar with them before we jump in any further.
The Gilded Rose Refactoring Kata
Here is the full description of The Gilded Rose Kata I found in Bobby Johnson’s repository:
Hi and welcome to team Gilded Rose. As you know, we are a small inn with a prime location in a prominent city ran by a friendly innkeeper named Allison. We also buy and sell only the finest goods. Unfortunately, our goods are constantly degrading in quality as they approach their sell by date. We have a system in place that updates our inventory for us. It was developed by a no-nonsense type named Leeroy, who has moved on to new adventures. Your task is to add the new feature to our system so that we can begin selling a new category of items. First an introduction to our system:
- All items have a SellIn value which denotes the number of days we have to sell the item
- All items have a Quality value which denotes how valuable the item is
- At the end of each day our system lowers both values for every item
Pretty simple, right? Well this is where it gets interesting:
- Once the sell by date has passed, Quality degrades twice as fast
- The Quality of an item is never negative
- “Aged Brie” actually increases in Quality the older it gets
- The Quality of an item is never more than 50
- “Sulfuras”, being a legendary item, never has to be sold or decreases in Quality
- “Backstage passes”, like aged brie, increases in Quality as it’s SellIn value approaches; Quality increases by 2 when there are 10 days or less and by 3 when there are 5 days or less but Quality drops to 0 after the concert
We have recently signed a supplier of conjured items. This requires an update to our system:
- “Conjured” items degrade in Quality twice as fast as normal items
Feel free to make any changes to the UpdateQuality method and add any new code as long as everything still works correctly. However, do not alter the Item class or Items property as those belong to the goblin in the corner who will insta-rage and one-shot you as he doesn’t believe in shared code ownership (you can make the UpdateQuality method and Items property static if you like, we’ll cover for you).
Just for clarification, an item can never have its Quality increase above 50, however “Sulfuras” is a legendary item and as such its Quality is 80 and it never alters.
Let’s play with The Gilded Rose Kata
I was looking for example in ruby and I found one in Emily Bache’s repository. Here is the code we need to refactor.
The first thing I had to do before rewriting the above code was to prepare a test suite to ensure my changes wouldn’t break the item rules. I simply added rspec and wrote the tests. There are plenty of them, if you want you can check out the specs here.
I was pretty sure every rule was covered in test suite so I made my first attempt to refactor the code. After I had done some work improving the code and I was still facing a green test suite a thought came to my mind.
They call it Golden Master
We used to run dojo workshop at Lunar and we used a clever technique called Golden Master Testing to record the behaviour of the program. We recorded bunch of input examples and output results from the program we wanted to refactor. The recorded data was used to check if the refactored code behaves in the same way. It’s great when you have to deal with legacy code and you don’t have test suite. At least if you can prepare seed input for the program and collect outputs. I wrote script texttest_fixture.rb that creates all kinds of items and runs the update_quality method for a given number of days. Below you will find the output for 2 days.
$ ruby texttest_fixture.rb 2 OMGHAI! -------- day 0 -------- name, sellIn, quality +5 Dexterity Vest, 10, 20 Aged Brie, 2, 0 Elixir of the Mongoose, 5, 7 Sulfuras, Hand of Ragnaros, 0, 80 Sulfuras, Hand of Ragnaros, -1, 80 Backstage passes to a TAFKAL80ETC concert, 15, 20 Backstage passes to a TAFKAL80ETC concert, 10, 49 Backstage passes to a TAFKAL80ETC concert, 5, 49 -------- day 1 -------- name, sellIn, quality +5 Dexterity Vest, 9, 19 Aged Brie, 1, 1 Elixir of the Mongoose, 4, 6 Sulfuras, Hand of Ragnaros, 0, 80 Sulfuras, Hand of Ragnaros, -1, 80 Backstage passes to a TAFKAL80ETC concert, 14, 21 Backstage passes to a TAFKAL80ETC concert, 9, 50 Backstage passes to a TAFKAL80ETC concert, 4, 50
Of course in our case more reasonable amount of days would be higher so we can cover more possible cases. I wrote a golded_master_spec.rb file that executes texttest_fixture.rb file for 100 days and generates nice readable it examples like below:
$ rspec spec/golden_master_spec.rb Golden Master for GildedRose match line 0: OMGHAI! should equal OMGHAI! match line 1: -------- day 0 -------- should equal -------- day 0 -------- match line 2: name, sellIn, quality should equal name, sellIn, quality match line 3: +5 Dexterity Vest, 10, 20 should equal +5 Dexterity Vest, 10, 20 match line 4: Aged Brie, 2, 0 should equal Aged Brie, 2, 0 match line 5: Elixir of the Mongoose, 5, 7 should equal Elixir of the Mongoose, 5, 7 match line 6: Sulfuras, Hand of Ragnaros, 0, 80 should equal Sulfuras, Hand of Ragnaros, 0, 80 match line 7: Sulfuras, Hand of Ragnaros, -1, 80 should equal Sulfuras, Hand of Ragnaros, -1, 80 match line 8: Backstage passes to a TAFKAL80ETC concert, 15, 20 should equal Backstage passes to a TAFKAL80ETC concert, 15, 20 match line 9: Backstage passes to a TAFKAL80ETC concert, 10, 49 should equal Backstage passes to a TAFKAL80ETC concert, 10, 49 match line 10: Backstage passes to a TAFKAL80ETC concert, 5, 49 should equal Backstage passes to a TAFKAL80ETC concert, 5, 49 match line 11: should equal
Golden Master to the rescue!
It turned out the golden master tests were failing on my refactored code. It means I made a mistake somewhere. My previously written specs were green but it seems like I didn’t cover everything. What was that? I checked the lines where the golden master tests were failing. I realized that I missed the case when an item with high quality=49 can’t reach quality greater than 50 but it should be able to reach a max quality of 50.
The rule for “Backstage passes” item says:
“Backstage passes”, like aged brie, increases in Quality as it’s SellIn value approaches; Quality increases by 2 when there are 10 days or less and by 3 when there are 5 days or less but Quality drops to 0 after the concert
I added missing tests to my rspec test suite and fixed the refactored code to make it pass.
What I’ve learned
Don’t trust myself too much. Don’t trust the tests I wrote. Always look for a way to prove if I’m wrong. The Golden Master Testing technique helped me with that.
What else have learned? There are plenty of little things pointed by Sandi Metz that helped me with refactoring code step by step.
- Make smaller things – it’s obvious when you see so many if statements then you know it’s not good to leave them like that. They’re hard to read, hard to understand.
- Duplication is far cheaper than the wrong abstraction – don’t be afraid to duplicate code. You are learning how to refactor the code and the abstraction is not yet clear until you understand exactly what your program does. Just don’t get stuck with wrong abstraction.
- Keep SOLID principles in mind – we would like to have an easy way to add a new Item with different rules. It would be great to have the code open for extension in that case. And even better to have the code closed for modification at the same time so there won’t be the need to change existing code when adding a new item.
- Things get worse always before they get better – intermediate steps during refactoring may look like they make things more complicated until you reach the point when you can get rid of complexity.
I did a second attempt to refactor code and I extracted a few smaller classes. I made the tests pass and I had a lot of fun with that.
Now it’s your turn
I prepared The Gilded Rose Kata repository with a ready to go test suite. If you want to tackle the exercise, you can clone it and switch to the “ready-to-start-exercise” branch .
$ git clone firstname.lastname@example.org:ArturT/GildedRose-Refactoring-Kata.git $ cd GildedRose-Refactoring-Kata $ git checkout ready-to-start-exercise $ cd ruby $ bundle install # run tests prepared by me $ rspec spec/gilded_rose_spec.rb # run golden master tests $ rspec spec/golden_master_spec.rb
This way you can run tests to ensure changes you make in the gilded_rose.rb file won’t break the test suite.
In the repository you will also find my first attempt and second with refactored code. Don’t open gilded_rose_refactored_1.rb and gilded_rose_refactored_2.rb unless you like spoilers!
All the little things
If you played with kata already then watch Sandi Metz’s video and check how she did the refactor. Hope you like it!