Over the years I had to deal with applications and system that have a long history of already being "legacy".
On top of that I met with clients/product owners that never want you to spend time refactoring. Some of them will even be aware that things like automated tests exist and will specifically tell you to not write them.
On one hand I can understand them. They do not understand code (even when they think so), they don't understand how vulnerable it is, but they understand how expensive it can be.
Doing an MVP, greenfield project often comes with restrictions on time and budget, so those kind of projects are the most vulnerable, and people who want them done, are usually in need of convincing that tests are important, and we should not write code, that requires refactoring immediately after it is done. That means that we need more time, and if we can't have that, we need to rethink our partnership.
Same goes for legacy projects, with terrible or bad codebases "that just work". They gather more technical debt than money, and there is never time for refactoring, specially since none (or not enough) of the code is covered with tests. Refactoring will be too time consuming when they are no tests to make sure that you are not just breaking stuff, instead of making it better, right?
Who is this for
This short post is aimed at developers mostly, and particularly at those, who put up with the behaviour of clients described above. I've historically had some successes and failures when it comes to convincing people that refactoring is needed, I had to persuade clients that had some technical knowledge, that tests are actually beneficial and time spent refactoring is time spend on making more money.
The point of this
Based on a Guilded Rose Kata I want to present a situation, that you can either use as an example, or learn from or at least take some motivation from.
The Kata
Guilded Rose Kata is a rather popular programming exercise, where you are given a bad piece of code "that just works", with no tests. You also get the general description of how everything works and what are you supposed to do (and what you cannot do). Here is the whole description:
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 concertWe 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).
The process
If you are dropped in this kind of situation in real life, and see this code (using ruby here):
class GildedRose
def initialize(items)
@items = items
end
def update_quality()
@items.each do |item|
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
end
end
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
end
end
if item.sell_in < 6
if item.quality < 50
item.quality = item.quality + 1
end
end
end
end
end
if item.name != "Sulfuras, Hand of Ragnaros"
item.sell_in = item.sell_in - 1
end
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
end
end
else
item.quality = item.quality - item.quality
end
else
if item.quality < 50
item.quality = item.quality + 1
end
end
end
end
end
end
class Item
attr_accessor :name, :sell_in, :quality
def initialize(name, sell_in, quality)
@name = name
@sell_in = sell_in
@quality = quality
end
def to_s()
"#{@name}, #{@sell_in}, #{@quality}"
end
end
This is obviously rather hard to follow. Mapping this to real world description of the system is not easy to follow.
Ruby is often praised (or hated) for its "english-like" syntax, so mapping almost a bullet point description to if code shouldn't be too hard, but the amount of nesting and conditionals do make me lose track of things. Most lines are not "stand-alone" they don't necessarily make sense on their own, and I have to constantly start reading the whole method again, or go back to the description.
We want something that "flows", code that makes it easy to add more features or change existing ones. Client wants that too, cause that means in the future, he has to spend less money on you.
It's a simple rule in IT, that money people will always understand:
The better the code, the less time it takes to add to it or to change it
This is where we have to start the discussion. The money side of things. The time it takes.
When discussing refactoring, never, ever approach it from the angle of perfectionist or even the POV of a engineer. Try to take a far more pragmatic, business and materialistic approach.
Don't say:
This code is so bad it makes me feel bad about being a programmer
Say:
This code is so bad, that it will cost you more money if you ever want to change anything.
There is no:
This does not follow any of the best practices
There is just:
This is so insufficient that it will scare away customers cause it will work worse and worse. All decent programmers will demand more money to work on it as time goes on
Always focus on money. Clients and product owners don't care if you think about them, but they do care if you think about their money, and how to make them have more of it.
This is mine main advice if you have to talk to clients that have limited budget. Don't focus on technology, don't try to show them how great of a engineer you are, don't explain how good code should be written, they don't care. Tell them why stuff is so expensive, why small changes cost them a lot of money, and what can you do to help them save it now and in the future.
Remember that coding is often mostly reading, rather than writing. Clients and non-tech people will often not be aware of it. If they are made conscious that first you have to understand the code you need to change, they might get a bit different perspective, and understand that if code is bad, it means it is hard to read, understand and change. Harder means more time consuming. More time == more money
This is how you can convince them to spend a few hours on refactoring that in the future will save them days. Time is money, so focus on those two things. How long will a feature take if code looks like that, and how long will it take, if you change it to look the other way.
Only once you get the green light on that, you can start talking about tests.
People that pay you will mostly care about time. How long will a task take? Why so long, and just make it faster, cut some corners!
They often think (sometimes subconsciously), that programmers only care about writing some awesome technological code. They might consider developers as people who don't think about money. You need to make sure they know, that you care about how much they spend on you. Once they know that you are aware of the financial side of things, they might trust your judgement more.
How to talk about tests
This is the most difficult thing, cause if you are in a position where you have to talk about tests with the client, you are already in a bad spot.
It is a technical detail, something clients, and business people in general should not worry about. It should be implied, that we only deliver code that we think is good, and good code is tested.
However, we have seen some other situations. Mostly in cases, where the client is a former technical person (or still slightly technical) and they know what tests are, but think they just come with more time, and provide little value.
How do you argue with that, when there is still a lot of programmers that think the same?
There are 4 main things to try, and if those fail, you can either accept your damned faith, or look for a different project/job/client.
- Tests are a mark of something being actually done. If you write tests for a feature (which is easier than writing the whole feature), you get a unit of measurement of that something actually being done. Also the more of those you write, the better you get with estimations (mapping out how feature relates to tests and that to implementation estimation is a topic for a different time, but in general, it comes with practice).
- When refactoring, it not only makes you more confident in making changes, it actually secures the safety of those changes. If you convinced them to refactoring, it is crucial to make sure, they understand, that refactor aims to create a good code out of a bad one, and good code is tested. When you are confident in changes and safe in making them, they take less time.
- Regression is something that can cost a lot of money. Seemingly unrelated change, breaks something that seems completely disjointed from it. Well known tale in IT, one that we can avoid to spend money on, if we have good tests.
- If they ever change programmer, they will have easier, quicker time onboarding them into the code that is well tested. That will mean less money spend on time needed for them to understand the codebase and make the same mistakes someone with no tests would do.
In general all the above aim to drive home one main point:
Tests save time, not add more of it
The better the testing suite, the easier and safer the changes to the actual code are. This means that code will not break as often and will be easier to expand on. Easier is faster. Faster is cheaper.
MONEY
Look like this on your webacm calls with the client
Make sure you mention it a lot.
The code
Lets actually solve the kata, ok? I will show you my process of testing and refactoring those kind of "features", "problems", or whatever we want to call it.
First of I map the requirements to a spec, in this case, RSpec and Ruby make it super easy and clean, since it is like reading english (almost).
Here is how I mapped the requirements to the spec (I start with just the descriptions, without the code, I only did a simplistic test for the Item class since I can't change it anyway due to requirements):
Initial spec
describe GildedRose do
it 'does not change the name' do
items = [Item.new('foo', 0, 0)]
GildedRose.new(items).update_quality
expect(items[0].name).to eq 'foo'
end
describe 'Item' do
it 'should have a name, sell_in, and quality' do
item = Item.new('foo', 0, 0)
expect(item.name).to eq 'foo'
expect(item.sell_in).to eq 0
expect(item.quality).to eq 0
end
it '#to_s' do
item = Item.new('foo', 0, 0)
expect(item.to_s).to eq 'foo, 0, 0'
end
end
context 'with quality' do
it 'degrades twice as fast if sell_in has passed (negative value)' do
end
it 'cannot be negative' do
end
it 'gets higher with passage of time for Aged Brie' do
end
it 'cannot be higher than 50' do
end
it 'is always 80 for Sulfuras' do
end
it 'decreases by 1 for all other items' do
end
context 'with Backstage passes' do
it 'increases by 2 when sell_in is 10 or less' do
end
it 'increases by 3 when sell_in is 5 or less' do
end
it 'drops to 0 after the concert' do
end
end
context 'with conjured items' do
it 'degrades twice as fast as normal items' do
end
end
end
context 'with sell_in' do
it 'never decreases for Sulfuras' do
end
it 'decreases by 1 for all other items' do
end
end
end
That makes it easy to track all the requirements. When I see that I haven't missed anything I start doing the actual test, no changes to the code at this point.
First full spec
Here I have the contents of the test, it has 16 examples, 14 pass, 2 fail, since they are specs for the new feature not yet implemented.
With that in hand I can see that this messy code does indeed work. I still have no clear idea how to change it to add the new stuff and not blow things up, but at least I can easily experiment.
describe GildedRose do
it 'does not change the name' do
items = [Item.new('foo', 0, 0)]
GildedRose.new(items).update_quality
expect(items[0].name).to eq 'foo'
end
describe 'Item' do
it 'should have a name, sell_in, and quality' do
item = Item.new('foo', 0, 0)
expect(item.name).to eq 'foo'
expect(item.sell_in).to eq 0
expect(item.quality).to eq 0
end
it '#to_s' do
item = Item.new('foo', 0, 0)
expect(item.to_s).to eq 'foo, 0, 0'
end
end
context 'with quality' do
it 'degrades twice as fast if sell_in has passed (negative value)' do
items = [Item.new('foo', -1, 10)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 8
end
it 'cannot be negative' do
items = [Item.new('foo', 0, 0)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 0
end
it 'gets higher with passage of time for Aged Brie' do
items = [Item.new('Aged Brie', 0, 0)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 2
end
it 'cannot be higher than 50' do
items = [Item.new('Aged Brie', 0, 50)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 50
end
it 'is always 80 for Sulfuras' do
items = [Item.new('Sulfuras, Hand of Ragnaros', 0, 80)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 80
end
it 'decreases by 1 for all other items' do
items = [Item.new('foo', 0, 1)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 0
end
context 'with Backstage passes' do
it 'increases by 2 when sell_in is 10 or less' do
items = [Item.new('Backstage passes to a TAFKAL80ETC concert', 10, 10)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 12
end
it 'increases by 3 when sell_in is 5 or less' do
items = [Item.new('Backstage passes to a TAFKAL80ETC concert', 5, 10)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 13
end
it 'drops to 0 after the concert' do
items = [Item.new('Backstage passes to a TAFKAL80ETC concert', 0, 10)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 0
end
end
context 'with conjured items' do
it 'degrades twice as fast as normal items' do
items = [Item.new('Conjured Mana Cake', 1, 10)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 8
end
it 'degrades twice as fast also when sell_in has passed' do
items = [Item.new('Conjured Mana Cake', 0, 10)]
GildedRose.new(items).update_quality
expect(items[0].quality).to eq 6
end
end
end
context 'with sell_in' do
it 'never decreases for Sulfuras' do
items = [Item.new('Sulfuras, Hand of Ragnaros', 0, 80)]
GildedRose.new(items).update_quality
expect(items[0].sell_in).to eq 0
end
it 'decreases by 1 for all other items' do
items = [Item.new('foo', 1, 1)]
GildedRose.new(items).update_quality
expect(items[0].sell_in).to eq 0
end
end
end
So with that, the refactor begins (only showing the part of the code that matter for brevity)
First thing is running rubocop to make it a bit cleaner, but it still leaves me with a mess. My first attempt is anything to make it more readable. Since most of the business logic is based on the name quality of the Item I see an opportunity to redo the if statements to case.
At least for the most part. Anyway, this is what the first version looks like:
def update_quality
@items.each do |item|
item.sell_in -= 1 unless item.name == 'Sulfuras, Hand of Ragnaros'
change_quality(item)
end
end
private
def change_quality(item)
case item.name
when 'Aged Brie'
item.quality += 1
when 'Backstage passes to a TAFKAL80ETC concert'
if item.sell_in.negative? || item.sell_in.zero?
item.quality = 0
return
end
item.quality += case item.sell_in
when 1..5
3
when 6..10
2
else
1
end
when 'Sulfuras, Hand of Ragnaros'
item.quality = 80
return
when 'Conjured Mana Cake'
item.quality -= item.sell_in.negative? ? 4 : 2
else
item.quality -= item.sell_in.negative? ? 2 : 1
end
item.quality = 50 if item.quality > 50
item.quality = 0 if item.quality.negative?
end
It is still messy, it still has a lot of if statements, but at least at first glance you can find places that hold the whole logic for a specific item. With nested ifs, that was harder to follow.
This first version of refactoring takes around 10-15 minutes. I did it by simply deleting all the ifs, and following the spec step by step, handling each case. That made it super easy to map the spec requirements to the case code and make the code pass.
However at the end of the day, it is barely better than the original. Adding new features seems like adding just more when statements, but there is still some ending logic that complicates stuff, and we end up nesting cases inside and there is still some leftover conditionals.
I myself am not a big opponent of conditional statements, there is a lot of programmers in Ruby that just grin and twist when they see an if. They get angry when they remember 102432 design patterns that could be used instead of an if, but I am too stupid for that, so I don't mind them from time to time, especially when using ternary operators, that just ensure simple 0/1 kind of result.
To adhere to this obsession though, and due to my own dislike of the case implementation, I will try something else.
I have been experimenting with Elixir programming language and it has a feature called pattern matching. Not just some feature by the way, it plays a major role in that language. Ruby has had it too for some time, although it is not as advanced and mature as in Elixir (and will never be due to the nature of both languages), but hey, it works, and seems to be a match made in heaven for cases like this.
Final refactor
This refactor took another 15 minutes, I also added one more spec, cause refactoring with the specs made me realise I need to handle one more case (see? more value of refactoring and testing).
This shows the whole code this time.
class GildedRose
def initialize(items)
@items = items
end
def update_quality
@items.each do |item|
update_item(item, item.sell_in, item.name)
end
end
private
def update_item(item, *pattern)
case pattern
in [_, 'Sulfuras, Hand of Ragnaros']
set_quality(item, 80)
in [_, 'Aged Brie']
age_item(item)
increase_quality(item, 1)
in [6..10, 'Backstage passes to a TAFKAL80ETC concert']
age_item(item)
increase_quality(item, 2)
in [1..5, 'Backstage passes to a TAFKAL80ETC concert']
age_item(item)
increase_quality(item, 3)
in [Integer => sell_in, 'Backstage passes to a TAFKAL80ETC concert']
age_item(item)
sell_in.negative? || sell_in.zero? ? set_quality(item, 0) : increase_quality(item, 1)
in [_, 'Conjured Mana Cake']
age_item(item)
item.sell_in.negative? ? degrade_quality(item, 4) : degrade_quality(item, 2)
in [Integer, String]
age_item(item)
item.sell_in.negative? ? degrade_quality(item, 2) : degrade_quality(item, 1)
end
end
def degrade_quality(item, level)
item.quality = [item.quality - level, 0].max
end
def increase_quality(item, level)
item.quality = [item.quality + level, 50].min
end
def set_quality(item, quality)
item.quality = quality
end
def age_item(item)
item.sell_in -= 1
end
end
class Item
attr_accessor :name, :sell_in, :quality
def initialize(name, sell_in, quality)
@name = name
@sell_in = sell_in
@quality = quality
end
def to_s
"#{@name}, #{@sell_in}, #{@quality}"
end
end
So the whole kata took about an hour. We ended up with far more readable code, that makes it easy to expand with more business logic and handles all previous cases and a new one.
To add more "edge cases" we simply pattern match them, use the methods to either set the quality to bypass the max and minimum validations of quality and always use the same age method if needed. Any fundamental changes would be easy to make too, like changing the way things age (just modify the simplistic method, that already has been used in all needed cases).
What about AI?
You might be scared of the incoming singularity, that will assimilate the human race under the new management of the machines. Google, Meta and some other companies are in a race to achieve that, since it makes their lines go up on some business charts. Seemingly forgetting that once they make AI replace everyone at their jobs, there won't be too many people to afford their products, no people to take data from to sell.
Some programmers are afraid of it too. What if some nasty client with a bad codebase, just feeds this code to AI and asks it to refactor it?
This is what awaits us... but not just yet
As a test I did that. I used GitHub Copilot, ChatGPT and if you used those tools for trying to refactor some code, you probably know what they did.
They just moved the strings and magic numbers to CONSTANTS and moved almost every single line into a separate method to just name the actions.
I will not try to explain how little value does this provide, and why is it inferior to the existing code, cause it would be a waste of everyone's time.
Conclusion
Given the "good-looks" nature of Ruby, you could even show the two versions of the code to a non-technical client. Anyone with eyesight would see a total mess in the nested ifs version, and the relative harmony in the pattern match, where each line seems to be looking quite similar. Same goes for RSpec, where the contexts and it descriptions seem to describe the entire domain, that you can verify with the product owner/whoever, showing them the value of it, when the descriptive text actually validates the inner workings of the code.
In the end, if you use the word money a lot in those difficult conversations, you should get some more trust from the people you talk to, cause they like to know, they are working with people, not programming machines. That is something AI will never give them, and a feature that makes flesh-and-blood programmers still relevant in the world racing to singularity. We can map the business requirements to both code and real life. We can figure out ways to save money, and it just so happens that saving money usually comes down to having a good technology, that is achieved through iteration and following best practices, like testing in case of Ruby.
