Agile Legacy Code

Primary Menu

How Not To Get Fired For A Refactoring

I get to the office some minutes before 9AM to check the e-mail before starting what I thought was going to be a very productive and very relaxing Friday. The release went live that night but for the first time in months all the check-ins were done weeks in advance to give the testers all the time they needed. No bugs were filed during the testing phase. The day was going to be a fun day.

I was wrong

At 9:08 a super-high manager enters the office and ask what was happening in production. He looks alarmed. His face says: “We are losing thousands of euros every minute”.

My boss asks me if we changed anything in the functionality that wasn’t working and I replied that we just fixed a minor bug and that the change couldn’t possibly be the source of the error. But as I stepped through the check-in history my heart stopped for a moment. I did not JUST fixed a minor bug, I also cleaned up the function a little bit.

That refactoring could be the cause of thousands of euros lost.

What I did

I had this piece of functionality that was super ugly, it was inside a monolithic method and I was having difficulty understanding what it did. First of all I did an Extract Method refactoring to separate the ugliness and then I started analyzing it. It looked something like the code below, just with some more comments and conditions. I simplified it a little for this post.

privateList<BoxItem> UglyOrderBy(List<CompleteBox> boxes)

{

varresult = newList<BoxItem>();

// Put first those with FlgDefault equals to "S"

foreach (varx in boxes.Where(cas => cas.FlgDefault == "S"))

{

result.Add(newBoxItem(x.Description, x.Code, x.SubCode));

}

foreach (varx in boxes.Where(cas => cas.FlgDefault == "N"))

{

result.Add(newBoxItem(x.Description, x.Code, x.SubCode));

}

return result;

}

Basically, the two for are there to order the list based on the value of FlgDefault (I know it can only have two values: “S” and “N”). So I decided to change it using Linq to improve its readability:

privatestaticList<BoxItem> NiceOrderBy(List<CompleteBox> boxes)

{

return

boxes.

OrderByDescending(cas => cas.FlgDefault == "S").

Select(x => newBoxItem(x.Description, x.Code, x.SubCode)).ToList();

}

The refactoring was pretty straight forward and I also run in debug mode to check that the behavior was correct but now I wasn’t sure of having tested every possible combination.

It turns out that neither QA wasn’t able to test thoroughly, and now that we had this bug in production I was trying to understand whether it was my fault or not.

Luckily, after some debugging I was able to verify that the refactoring was correct after all, and that the bug was introduced be a change in a different part of the application (global variables were involved). But the main problem (the one that almost made me faint) was that I wasn’t sure that my simple refactoring didn’t change the behavior of the system.

I could have avoided all this

All that sweat and that auto-accusation, all that “I’m going to get fired because of this” could have been avoided simply by doing some categorization tests. It would have taken me probably an hour to cover that piece of functionality but, after that, I would have been able to refactor safely and not think about it anymore: I would have never thought that the bug was my fault.

Not only, the code coverage would have increased and with it the overall confidence in the code base and, believe me, that code base could use some confidence.

Final thoughts

The problem with working in a legacy system is that there’s never time to improve it, you spend a lot of time trying to add functionalities and fixes without breaking everything and there’s never time to refactor properly.

After this experience, though, I promised myself to never change again the implementation on even the most simple functionality without first writing a test harness that will make sure that the current behavior of the code doesn’t change.

Author: Daniele Pozzobon

Daniele is an aspiring software craftsman with more that ten years of experience in the software industry. He is currently a consultant in the .Net space for a big insurance company, and previously have worked as a web dev in the manufacturing industry. He has experience with C#, Java, C++, PHP, Javascript, and lately has added some F# to the sauce.
He constantly annoys his friends by talking about software and is passionate about Agile methodologies, which gives him more opportunities to talk annoy his friends even more.
When there are no friends around to annoy, he blogs on CodeCleaners and in his free he time loves go hiking with his wife and two daughters.

When I do refactoring, my rule of thumb is:
If the code was straightforward before refactoring — it should remain straightforward after refactoring.

In the example below, my first thought was to extract the similar blocks into a separate methods and give those methods a human friendly names. I have assumed that if we are talking about flights, then S is South and N is North, but I definitely need more domain knowledge.