Some Ideas on Refactoring Legacy Code

This are some notes about refactoring legacy code that I’ve come up with from my own experiences doing the same. I do not claim it is in line with the “official” XP doctrine on refactoring. Any errors, idiosyncracies and other perturbations are strictly attributable to me.

Why am I doing this?

The first question to answer is: Why do the refactoring in the first place? Considering the amount of time refactoring legacy code can use up, it is an important question to address.

There are two broad categories of refactorings: the architectural or redesign refactorings, and the “little” refactorings. The architectural refactorings readjust the interaction of various parts of the code base. They are used to clean the code at a higher level. I am not just cleaning up the code, I’m redesigning on the fly. For architectural refactorings, the emphasis is on class extraction and fixing the interaction between classes. When I’m done, the interface between classes and groups of classes is clean and simple. Individual classes have a single conceptualization and are generally small and simple.

The little refactorings are meant to clean the code at a low level. When I’m done the code for individual classes is clean, simple and clear. The code is transparent in it what it is trying to achieve. I can read the code and with little effort or comments determine what it is doing.

When both types of refactorings are complete, the code is easy to follow at a high level, the architecture is transparent and how each component of the architecture achieves its function is transparent. The code base is more maintainable when I’m done because other developers can read the code quickly and easily.

When am I done?

The next question to consider is: When am I done? The answer is it depends on what I want to achieve. Some examples:

• I have done a “merciless refactoring” on legacy code for pedagogic reasons. It takes a lot of time but very instructive for learning OOP. The emphasis here was on the word “merciless”. Each class, each member variable in each class and each method in each class is reviewed towards splitting it up or perhaps splitting it off into a separate class.
• I have done refactorings to learn what the code was doing. This is a coarse-grain refactoring and is an excellent way to understand, in a deep way, what the code is doing. The emphasis here is to clarify the code but not go as far as “merciless”.
• I have done refactorings to uncover and fix bugs. This is midway between the previous two points. The emphasis here is divide and conquer. Split off the suspect code into separate method or chunk, and then add unit testing to either prove the code has a bug or prove it doesn’t.

If I’m clear on what I’m trying to achieve, it’s easier to know when I’m done. If it is to uncover and fix bugs, I’m done when I’ve found the bug and it’s fixed (with the new UTs proving it). If it is to understand the code, I’m done when I feel comfortable with the depth of my understanding. If it’s merciless refactoring, I’m done when I cannot see any other conceptual chunks to cleave into a separate class or method.

I’ve also taken the incremental approach and done several passes at the code, starting with coarse-grain refactorings and refining as I get more familiar with the code (and have more tests in place). This approach can span many weeks or even months. The code is slowly migrated to a fully refactored and unit tested form.

How do I approach the refactoring?

Let me state up front: I like rules-of-thumb, and heuristic and probabilistic reasoning. Therefore my approach to refactoring is based heavily on a few rules-of-thumb I’ve come up with.

Extracting Classes is the #1 Priority

If legacy code were written in a good OO style, there would be little need for refactoring except for polishing. Since I’m generally in the code because it’s in bad shape or indecipherable, my first priority is to disentangle the classes inherently in the code.

But wait, let me back up a little. The code is bad right? Otherwise I wouldn’t be in there except for pedagogic reasons. If I were refactoring strictly for pedagogy, I’m betting that I’ll learn more refactoring really bad code then by polishing good code.

But whether for pedagogic purposes or for practical reaons, there are intertwined classes in the code. If a class has a single, simple purpose then almost by default the code will be clear and simple. The only code in such a class is directly related to it’s purpose and it’s purpose is supported by all the code in the class. When there are two entangled classes in a piece of code, they interfere with each other, creating a sort of moire pattern in my understanding of what the code is trying to do. If the interference is weak, I can still make out what is going on but if the interference is strong, it may overwhelm my attempts at understanding it.

So understanding the entanglement and the teasing out the intertwined classes is my first priority.

Simplifying the interface of a class is #2

The interface of a class is it’s public methods/variables and their signatures. The simpler that interface, the simpler the class tends to be. Or should I say “The simpler the class, the simpler the interface”? It doesn’t really matter, the two are heavily related and any effort in one area will generally benefit the other.

The fewer the number of methods the better. The fewer the number of parameters on a method the better. Most or all of the methods should be involved when typically using the class. If there is a partition of methods, i.e. certain methods are used in some scenarios, and other methods are used in other scenarios, then the class should be split (see above).

The interaction among the public methods also affects the simplicity of the interface. A common example is the Open() method must be called before the Close() method and the Close() must be called before the Open() is called again. There is a dependency between Open() and Close() that is not apparent by just looking at the signatures of the methods. The dependency, to be made clear to the user of the class, must be documented somewhere or be so idiomatic (as it is in this case) that the user already knows it a priori.

There should be 1 – 2 lines per method on Average

Almost all methods in the code base should have 1 line of code (excluding guard clauses, lines with just braces on them, wrapped lines, and other syntactic necessities.) I like this rule very much because it reinforces what “merciless” really means. When I feel like I’ve done enough refactoring on a class, I double check the number of lines in each method. If there are any over 1, I justify them to myself or continue refactoring them. The rule motivates me to extract code and methods. The end result are final classes that are very simple and singular in purpose.

Realize refactoring legacy code is not the same as greenfield refactoring

When I’ve extracted out whatever objects I can and I’ve refactored the code, I’ll have a system that is more OO than it was before. In general, it will not be as clean as code from the normal XP merciless-refactoring-as-you-go style of coding.

If I have a perfectly clean code base, for example the very first line of code of a brand new application I’m writing. I refactor it mercilessly. I then add two or three lines of new code and refactor mercilessly. Compare that to refactoring a method with 250 lines of code.

The mental or psychological effort is nearly trivial in the first case. Any “code smells” are readily apparent. Complicated areas of code are easily found and corrected. The effort is substantially more for the legacy code. The qualitative difference is perhaps due to mental fatigue or some sort of psychological phenomenon, but it is real and translates into more time spent performing the refactoring.

XP’s test-first, UT and FT (functional test) practices work very well in the forward direction: I start with a clean system, add clean pieces and end with a clean system. But I am going the other way. I never know and will never know that I have a clean system in the way that XP allows me to know.

The Method

Here’s an outline of the method I use to refactor legacy code. I have listed most of the activities I do; I may or may not do them in the order listed.

Create a safety net

It is very easy to introduce bugs when refactoring legacy code. I write a handful of ATs that test the basic functionality of the entire system. I make them completely automated and invocable by one command/button. I invoke the ATs many times so I take a few extra minutes to make them easy to use:

• A good rule is to have the AT’s produce little output. Either a single “PASS” or a list of failures. That way I can tell at a glance whether my test passed or not.
• I try to balance the coverage of the ATs against how long they take to run. I can stand about 2 – 4 seconds total running time.
• I also develop a more comprehensive AT suite that I run periodically.

I don’t write UTs until I’ve got a stable set of objects. I usually don’t know they’re stable until I’m nearly done, but sometimes the class just seems done. When I’ve written UTs up front, I end up rewriting the UTs as I sling the code around. This slows me down and causes me to hesitate before I change a bunch of code. This is bad.

Use a good –make that a great– editor

Is there more to say? For me, a pretty-printer makes things much easier to understand. Know the keyboard shortcuts for the editor. Have multiple monitors, multiple screens. A merge/diff tool is very useful too.

Make a small change, recompile and test after each incremental change

It is very easy to make mistakes. I try to make very small changes at any one time. Even if it looks “obvious” the code is correct, I’ve been wrong often enough to distrust that obviousness. I do not do global replaces. If there is a code change in 10 places, I do the first in its entirety before I move onto the next place. I recompile and run the ATs at least once per each of the 10 places.

Sometimes I’ve been forced into doing a large change. I prepare for these by writing a quick backup script, e.g. copy all the source files to a backup directory. I make sure the backup script works and try rolling back to the backup versions. Then I proceed with the large change. I’ve found a source control tool has been generally too clumsy for this kind of work. The simple backup is quicker, easier to rollback and I can have multiple versions of backup. I can also use my favorite full-directory tree comparison tool at will.

Create two new object immediately: Globals and Constants

I put all final (or const) variables or classes into Constants. The objects in Constant have to be constant across the lifetime of the session and generally used by the rest of the system.

The objects put into Globals have to be global for the lifetime of the session and generally used by the rest of the system. If it is not possible to initialize all Global variables, I put an Init() method in Global and call it from the mainline to initialize. I add methods to Global as needed to access the Global instances. I try to create more than just setter/getter methods. I move as much code surrounding the use of a Global instance into the new Global method as I can.

I don’t use Globals and Constants loosely. On the other hand, I don’t agonize whether or not to put an object into them. If later on I feel that something should not be global, I just extract it out.

The Globals class usually starts out as a dumping ground. I need to have a place somewhere in the system where I can quickly place methods and variables. I don’t want to slow down the work just because I can’t find a spot to put a particular method. Eventually, I find that most variables I put in this class are not globals after all.

I tend to place as much as code as I can into Globals and then extract a class from it every so often. When the refactoring is done, Globals is usually empty or nearly empty. Anything left over should be truly a global method or instance: if my entire code base uses it, it is a global. If Globals is empty, I delete it.

Break the longest methods up first

A good “bang for the buck” effort is to break up long methods into smaller ones based on “hey these 10 lines of code *belong* together”. I don’t worry about performance or getting the break-up right the first time. And I don’t clean up the new methods. I don’t “fix” bugs or things just yet. I just break up the code into lots of pieces. I’ll be reorganizing and fixing things in a little while anyway.

Finding these 10 lines is sometimes very easy: there is a comment heading them up explaining what they do, or there is a blank line separating groups of lines. In other words, the original programmer intuitively knew that the lines belonged together but didn’t write a method to group them. I just help the code along by extracting the method.

Rename variables and methods

Generally, poorly structured code also has poorly named variables. It’s amazing what a difference just these two steps (split long methods and rename variables) can make. I find that the structure of the code becomes substantially clearer after only a couple of passes.

Extract language blocks as methods

Look at Switches, if and else blocks, for blocks, while blocks, try/catch blocks, etc. The language blocks are a pretty straightforward sign that those lines of code belong together.

Be careful though, it isn’t necessarily a good thing to extract out all of the blocks into methods. I look for lines of code that are logically associated or “belong” together, not that just happen to be together in the same block.

Look at signatures of all methods in a class

Sometimes I find many similar signatures in the methods of a class. This sometimes indicates that a class can be extracted. The formal parameters of the methods are the instance variables of the new class. The extracted methods then have empty or nearly empty parameter lists.

I create the new class as soon as I can, but I take a moment to think about why those particular methods group together. I name the new class based on that relationship, that becomes the conceptualization of the class.

Extract a class, add code to it

When I extract a new class, I immediately look for as much code that I can add to the new class, but it has to fit the conceptualization of the class. While I do this, I keep track of how the methods in the new class are fitting together. Do they, as a group, make sense under the class’s name? If I need to, I change the name of the class to better reflect what the class is now doing. If the class has gotten too complex, I’ve gone too far and I need to split the new class.

Examine the code around instance variables

The simplest classes have one instance variable. Multiple instance variables indicate multiple classes (but not necessarily so). I look at the code surrounding instance variables of a class. If there is repetitive use with exact or similar code around the instance variable, I extract a method. If I’ve extracted several methods that all use the same instance variable, I see if I can extract out a new class.

Examine the code for third-party API calls

I look for repetitive uses of a particular API call or object, e.g. InputFileStream (java). If those calls have any commonality, e.g. I open a file with InputFileStream and look for a few special lines in the file. If so, I extract out a method. If the API calls are being applied to the same sort of object, e.g. the code is only opening an ini file, I extract out the new methods into a new class, e.g. an ini file class.

Look for groupings in the public methods of a class

If a group of public methods in a class are logically associated to one another, I extract the methods into a new class. I periodically look at the Global methods with this rule in mind. Can I partition them into logical associations? If so, I group the methods into a new class. I move as much of the Global code as I can into the new object.

Do some of the methods only interact with a few instance variables and those instance variables are only used by those methods? If so, I extract those methods and instance variables into a new class. I move as much code as possible into the new class. See if you can turn the calls to those objects inside out: instead of an object calling a method on the Global class with a set of parameters, see if you can pass the object into the Global method. If it makes sense to do so for a number of Global methods, extract those methods out into a new class.

Doing all of the above might cause you to create objects that really don’t stand alone, so you may end up merging two objects after the fact. If you find that you have split a class, merged it and then split it again, stop. Find out why you are thrashing.

While you’re creating the objects, try to develop a deeper understanding of how the application is structured. Creating and naming new objects becomes a whole lot easier if you know what the major components of the code are and how they relate to each other. You might even be able to predict the existence of future objects this way and make it a self-fulfilling prophecy.

At some point in time, you will not be able to discover any new objects using the above methods. Try some other techniques to change your perspective and to take a breather: Review Refactoring by Michael Feathers and apply all his techniques to your code.

At this point, UTs (unit tests) may be helpful. Balance this off with the effort of writing the UTs. Have someone else look at the code. If they cannot understand the intent of the code easily, refactor the code until it’s structure and intent is easily apparent to both of you.

Resist adding comments! You can however, leave class level comments to clarify why that class exists and any notes on how to use it. It is better to change the code such that the comments are not necessary, e.g.

// jump to the next date
curdate += 10;
curdate %= get_days_of_the_month();

refactored to:

jump_to_the_next_date(currdate);

In the second case a comment is not required.

Apply a re-engineering tool to your new code. What does the UML look like? Does it make sense? Do any of the classes look like “Controller” classes? Are there any classes that are disconnected? Do the class names make sense from this perspective? Check the top level interfaces. Can you change them to reduce the total amount of code in the system? If so, keep the new macro-architecture in mind while you refactor at a micro level. You can slowly convert to the new architecture taking small and safe steps along the way.

Also flipping back and forth from the micro- to the macro- level can give you a different perspective. Things discovered at the macro- level can give you a clue to overall program structure and therefore to new objects. Look at the overall class structure of your program.

Apply these good OO heuristics: Is the class structure flat? Reduce the hierarchy depth as much as makes sense. Is there associations between classes? Are classes as dumb to the outside as possible? Reduce associations between classes to a minimum by making each class dumb about everything outside itself. Does any class know about other classes in the rest of the program? Refactor the class so that it knows only about itself.

There are two exceptions to these rules. First is that each class lives in the operating system or environment provided libraries. I freely include those anywhere they may be useful. The other exception is the main program or a small number of control objects. These act as glue to tie together all of the tools to solve the problem at hand.

You may object that this makes it more difficult to port to a new OS or library. I would argue that a well factored application using good OO style is more important to achieve than portability. It is of no use to sacrifice code maintainability and readability in the medium term (a few years) for a long term goal that may or may not come to fruition. I think that cross-platform compatibility is overrated as a requirement although there have been tragedies because it wasn’t. Simply said, cross-platform techniques are very difficult to do and to get right. Languages like Java and Perl mitigate these problems but do not make them go away. I would say the vast majority of applications would be better off if they were done right for one platform rather than done badly for several.

More…

Remove dead code. Find it and remove it, don’t just comment it out. Rely on your source code repository to get it back if you really needed it.

Use idempotency. Allow methods to be called multiple times and still work correctly.

Use a class to encapsulate many formal parameters. When several methods have signatures with 4 or 5 or more parameters, create a class for them. Start with an empty class except for these parameters and then start moving bits of code into them.

When you just extracted a method, immediately look for duplicate code similar to the method I just extracted and replace it with a method call.

Look for lines of code that use the same instance variable, or same temporary. Extract them into a method. -

Look to extract very simple classes even if it had only one method and one member variable. Look for additional methods to add to the class.

Break up a big method into a class. The old temporaries and formal parameters to the method become instance variables in the class. All duplication within the old method will disappear it’s contents are refactored into the new class. That done, look at making the new class an instance variable in the original class and getting rid of the old method entirely.

Look for series of methods with similar signatures; extract them into a new class. The replication within the signatures is code duplication. The class extraction gets rid of that duplication.

Look for series of methods with similar prefixes or suffixes in their names. e.g. BobInit(), BobTerm(), BobOpen(), BobClose() strongly suggest that “Bob” is in fact a class.

Check if each class has a singular purpose. A quick *indirect* measure of this: there will be very few, if any, ‘if’ statements in any one class. If not, look at moving the ‘if’s higher up in the calling chain and using Design Patterns  to remove the ‘if’s.

Another quick *indirect* measure of this: there are only 1 or 2 member variables in the class. More are ok but justify their existence.

Check if there’s no more duplication. A quick *indirect* measure for this: avg # of statements per method => between 1 and 3.