How to refactor correctly

Note: This post was mostly inspired by The RailsConf 2014 talk titled “All the little things” by Sandi Metz. The video can be found here: https://www.youtube.com/watch?v=8bZh5LMaSmE

What is refactoring?

The basic idea of refactoring is to change code into other code, which behaves exactly the same. While this initially might sound stupid, consider these two examples and see for yourself which you like better. Keep in mind that these are functionally completely equal:

void DoDataAction()
{
    for i from 1 to 52
    {
        j:= i + r(53 - i) - 1
        s(i, j)
   }
}

void SwapDeck()
{
    constant int CardsInDeck = 52
    for i from 1 to CardsInDeck
    {
        j:= i + randomInt(CardsInDeck + 1 - i) - 1
        swapCards(i, j)
   }
}

In modern days, there’s essentially two kinds of refactoring now:

1. IDE-assisted refactoring
2. The good stuff

IDE-assisted refactoring is pretty boring, but useful. Renaming variables, methods, classes, etc. safely is a huge plus. Many IDEs allow extraction of methods and interfaces and much more. But then again, it’s just a click away and there’s not really a lot to mention either.

Now on to the good stuff: Have you ever looked at a piece of code and the moment you saw it you noticed that it was awful and needed to be rewritten? How does one actually go about such a thing? It’s not as hard as you think.

First of all, make sure there are a lot of unit tests which can be run quickly. These unit tests must make sure that code you write is actually good. Don’t ever let a scenario occur in which all unit tests pass, but your code is still “wrong” for some reason. Writing good unit tests comes with experience, but this isn’t the scope of this article. Just make sure unit tests are there and don’t take 30 minutes to run.

Next, look at all the cases of your code. Often, huge masses of code and nested conditionals mean that you’re trying to take care of 30 different cases. Unit tests often reflect that as well, with names such as “test_item_normal”, “test_item_special_1”, “test_item_null”, etc.. In such cases, rewrite your code bit by bit.

Begin with the first case, write a condition just for that case and handle that case in an external method. Write code for that case and see that your unit tests are green again. Rinse and repeat for all the other cases. Keep in mind that your unit tests must pass before you continue.

Now that your code is finally broken into pieces, your old blob o’conditionals should just look like this:

void SomeComplicatedMethod(Item item)
{
    switch(item.Type)
    {
        case 'Normal':
            HandleNormal(item);
            break;
        case 'Cheese':
            HandleCheese(item);
            break;
        case 'Sword':
            HandleSword(item);
            break;
        case 'Pudding':
            HandlePudding(item);
            break;
        default:
            //Do nothing
            break;
    }
}

In fact, this sort of code almost screams “Please abstract me!”, so let’s just do that. In fact, the best way is to this is to create a class for each individual type of item, and make them all inherit from Item. Then every class needs to implement a common “Handle”-method and implement their behaviour there. And between all of that, make sure your unit tests always run green.