How Not To Get Fired For A Refactoring

Friday morning.

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.

private List<BoxItem> UglyOrderBy(List<CompleteBox> boxes)
{
    var result = new List<BoxItem>();
    // Put first those with FlgDefault equals to "S"
    foreach (var x in boxes.Where(cas => cas.FlgDefault == "S"))
    {
        result.Add(new BoxItem(x.Description, x.Code, x.SubCode));
    }
    foreach (var x in boxes.Where(cas => cas.FlgDefault == "N"))
    {
        result.Add(new BoxItem(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:

private static List<BoxItem> NiceOrderBy(List<CompleteBox> boxes)
{
    return 
        boxes.
        OrderByDescending(cas => cas.FlgDefault == "S").
        Select(x => new BoxItem(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.

I would love to know your horror experiences and what you did after

To know more about our adventures in legacy code and to get tips on how to clean up your code subscribe to our mailing list!

Advertisements

3 thoughts on “How Not To Get Fired For A Refactoring

  1. I know it can only have two values: “S” and “N”

    Is there any way to put assertion in there (or, better yet, enforce this via types)? It is a supper non obvious invariant for both functions.

    Like

  2. could have been avoided simply by doing some categorization tests.

    Another strategy would be to compare both implementations on random tests. Sort of property based testing.

    Like

  3. 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.

    https://dotnetfiddle.net/QnnN1H

    My potential next step would be extracting the “N” and “S” as a type of constant and use it as a method parameter.

    The author’s solution is not straightforward and makes the code harder to read:

    OrderByDescending(cas => cas.FlgDefault == “S”)

    since it converts one problem of ordering “S” and “N” records into another: ordering true and false values.

    So now it puts “S” on the top because “S” is true? Why true is on the top? … maybe because true == 1 and false == 0… And the descending order for those would be 1, 0…

    So I would rather spend less time reading 5 more lines of code then spend more time trying to understand 1 line of code.

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s