Refactoring by the Book

Refactoring is one of the techniques that allows us to be agile and apply an evolutionary approach to our design. ¬†A core XP practice and one of TDD’s pillars, constant refactoring keeps the evil design upfront at bay and maintain our codebases in a healthy state. One of the few things I think most developers agree on is that refactorings are helpful and should be done at some point in any project, preferably in a continuous manner. The problem is that, like every other developer term, the definition of refactoring has become muddled over time. Refactoring is now commonly conflated with it’s more dangerous cousin, the Rewriting. The term is used whenever we want to improve the design of some part of a codebase, either a single class or entire subsystems.

The original refactorings

Although they existed a long time before that, Refactorings were introduced to the large world by Martin Fowler’s book¬†Refactoring: Improving the Design of Existing Code¬†where he writes about coding practices from legendary SmallTalkers like Kent Beck and Ward Cunningham. A definition from the same book: “Refactoring is a disciplined technique for restructuring an existing body of¬†code, altering its internal structure without changing its external behavior. Its heart is a series of small behavior preserving transformations. Each¬†transformation (called a ‚Äúrefactoring‚ÄĚ) does little, but a sequence of¬†transformations can produce a significant¬†restructuring. Since each¬†refactoring is small, it’s less likely to go wrong. The system is kept fully¬†working after each small refactoring, reducing the chances that a system can¬†get seriously broken during the restructuring.” The original refactorings were presented using a pattern format. They have names, likeExtract Method¬†or¬†Replace Temp with Query, an explanation of when to use the refactor and a list of steps you have to follow in order to apply them. You can browse the refactorings catalog on¬† They are meant to be applied in a conscious manner, one after the other, until your design is good enough for you to move forward. Words like¬†disciplined¬†and¬†small¬†are present on the very definition of the term. When you wildly rewrite large parts of your system, you are not refactoring, even if you are just trying to achieve a better design. The system needs to be kept working the whole time. That means the tests need to stay green! The TDD flow ¬†is supposed to be Red -> Green -> Refactor, and not Red -> Green -> Red -> Red -> Oh nooooo! -> Rollback.

Some ways to refactor on the green
Let’s see how we can refactor some code in a more disciplined manner, keeping the tests always on the green. Instead of working on some contrived User class we are going to use a real example, the class¬†Request¬†from the¬†Pacto¬†project. You don’t need to understand what the class does, just pay attention to the code structure and how we apply the refactorings iteratively.
This class has many small issues, but the main thing I want to do is to remove theinstatiate¬†method. ¬†This method is forcing the¬†Pacto¬†codebase to deal with instances of a foreign class,¬†Faraday::Response. ¬†The thing is that¬†Faraday::Response¬†is a very¬†simple class, and we can easily implement the interface we need on the¬†Pacto::Response¬†class itself, eliminating the dependency on external code. Step 0 – Check the current tests Before we start to refactor, a very important step is to check if the code in question is actually covered by tests. A quick way to do it is to just do ¬†the refactoring, in our case simply remove the¬†instantiate¬†method and see if anything breaks. If you get zero red tests, that’s not a good thing! That means you have no way to be sure that your refactorings didn’t impacted the codebase, no safety net to catch your mistakes. There’s a famous phrase that says “refactorings without tests is just changing stuff”. If that happens, just rollback your change and start implementing the tests for the behavior you want to change. Luckily for us¬†Pacto¬†has test suite, so we get a precise red spec if we remove theinstantiate¬†method.

Step 1 – Move Method
The failing spec tell us that we need a method¬†body¬†on whatever¬†instantiate¬†returns. We can see the body definition passed on the¬†Faraday::Response¬†initialization. Let’s apply a slightly altered¬†Move Method¬†refactoring to move it from the¬†default_env¬†definition to theResponse¬†class.
We run the tests to make sure we didn’t break anything. Since we only changed private methods and we kept the original structure, everything runs fine.

Step 2 – Change Faraday::Response to self
It turns out that step one is the only thing we need to do in order to be able to replaceFaraday::Response with self on the instantiate method, so we  change the method.
That’s probably the most useless method ever, but remember, we want to keep the tests green. By allowing this travesty to live a little longer on the codebase we can be sure that nothing will break and move forward with a few other refactorings before paying attention to other parts of the¬†Pacto¬†codebase.

Step 3 – Remove the unused default_env method
So we have a private method that’s not used anymore. Safest refactoring you can have, we don’t even need to run the tests for this one, right? Well, remember that in Ruby private methods can be called with¬†send¬†(a very bad practice, by the way), so you are never totally sure that’s something is not important just by eyeballing a class. Always run your tests. Thankfully¬†Pacto¬†is not a metaprogramming happy codebase, so our tests still run green after removing the¬†default_env. marcosccm-refactoring-5

Step 4 – Remove unused @definition field
Another piece of private unused code, same rules as the previous step. marcosccm-refactoring-6

Step 5 – Remove excessive conditionals
On the¬†Response#body¬†method we have some conditionals that could be easily replaced by a default value for the¬†schema¬†attribute. Doing a “let’s see if something break” trick we discover that this conditionals are not under test, so first we add the relevant specs: ¬† marcosccm-refactoring-7 Then we replace the conditionals with a default value for the¬†schema¬†field: marcosccm-refactoring-8

Step 6 – Remove instantiate method
Finally the class is looking good enough and we can go forward and remove theinstantiate method. Whoever is using that method can just use a Response instance instead. For this step we simply delete the method and fix all the broken specs. Thankfully in this case, the Response class was used only by another class, so it was a very straightforward process. For cases were you have many dependencies, you might let the offending method live for a little longer and replace each use separately on work on each case separately.
There’s still some things we could do on this class, like turn it into a¬†Struct, but it’s good enough. Knowing when to stop refactoring is as important as knowing how to do it, the world is full of yaks and rabbit holes.

I summarized some parts of the article, but you should read this at least one time in your life. If you don’t know how to do a good refactoring you and your team will be lost. ūüė¶

I know, very extensive article today, but it is very important too! Bye ūüôā