Refactoring Legacy Code

DossyShiobara had some interesting ideas about RefactoringLegacyCode.

From the XpMailingList [message http://groups.yahoo.com/group/extremeprogramming/message/21261]:

In his "Refactoring" book, Martin Fowler mentions that he refactors unfamiliar code while reading it, in order to understand it. What do I do when I want to do the same thing, but I don't have unit tests to allow me to take that risk? Do I just jump in, and be as careful as I can, and hope that I don't break anything? I can't really write tests first, because I don't know what the code is supposed to do. Any advice would be greatly appreciated!
The code is very complex. There are lots of really long methods, duplicated code, large classes, etc. I feel that refactoring is the only way I can really understand what's going on. -- Larmore, Edward

Refactoring legacy code written before test-first ... I do LOTS of this.

I've oversimplified (for the sake of speed) scenarios down to a few base cases:

The code "works", you know it works, but you need to refactor in order to put the code into a form you can understand and work with in order to extend its functionality.
The code works, you know it works, and you figure you ought to try and refactor it since you're spending the time to read it. You actually have no business touching the code, because no story you're working on calls for any changes to be made in this particular code.
The code doesn't work, and you know because of known defects. However, you must touch the code because you're implementing a story that requires you to make changes to this code.
Like #2, except the code doesn't work.

In case #1, you write tests against the code, knowing that if the test fails, you wrote the test wrong. Don't change the code until you are confident you have enough tests to let you know if one of your refactorings broke something.

In case #2, unless you're at the end of an iteration with no other stories to do ... just leave it alone. Refactor the code? YAGNI. Wait until things change to become case #1 before making modifications. But this leaves you at the mercy of a tight schedule - if the code is hard to understand, it will take longer to refactor, and the needed changes will be delayed. Shouldn't you refactor when you have some time to do it? -- PeteHardie

In case #3, this is the most common and saddest state of affairs. Read the code as best you can, write the tests you're certain of (based on what your user stories say should be happening) and work iteratively across the code to fill in the gaps. This method works for me: First iteration, read logical chunks of code and put in comments. Second iteration, apply "Replace Comment with Test Case" refactoring (RefactorReplaceCommentWithTestCase).

Now, you'll have a bunch of test cases (some correct, some incorrect) but they should all pass. Now, go through your tests and figure out which ones are actually incorrect - those will uncover the defects in the code that needs to be refactored. Correct the tests, then proceed as you normally do in test-first design (change the code until tests pass). This "changing" may involve writing new code, or refactoring existing code.

In case #4, it's the hardest one to deal with because internally you feel like you must go and clean up code you KNOW to be defective, but if there's no story calling for it and you stop your current task to work on it, you're not giving the customer what they're paying for. You could always ask the customer to write a story to remove the defect you found, but don't drop your current task just to go and dive into this other thing.

I just realized that you said you couldn't understand the code until you refactored. Here's a trick I use in this situation. I like to call it "non-invasive refactoring," since I don't actually change the code:

Read the code a line at a time. When you spot a logical grouping (a "logical chunk"), write down on a piece of paper some kind of identifier for this chunk, and a brief description of what the chunk does. Sometimes a chunk will be a single line; sometimes it will be a whole page of code or more. The idea is to perform code-folding manually, so that you can read lots of code in a higher-level fashion. You're basically trying to build both a list of code modules, and a flowchart showing the flow from one code module to the next

When you recognize two code chunks that yield the same output state from the same input state, you've found a violation of OAOO. Sometimes you'll identify code that could never be reached. This could be a candidate for deletion - unless it's supposed to be reached, in which case you've found a defect.

Refactoring even the ugliest code is quite possible if you take it in very small bites, and stick with writing tests first. That way, when you're done, you'll at least have written some tests for the next time you make a pass at refactoring; and you can stop at any time without feeling you haven't accomplished anything.

-- DossyShiobara

I remember refactoring a piece of OmniMark code by porting it into Perl and converting it from a stream parser to a regex parser. This unintentionally fixed some of the broken pieces of the code that had not been noticed. Refactoring legacy code can actually fix bugs sometimes. It is sometimes easier to fix a bug by refactoring the code it's in than by tracking it down.

If refactoring code fixes a bug, wasn't what you did, by definition, not refactoring? Refactoring is a change to the code that has no effect on the code's behavior. [If you don't have a test, of some kind, how can you say that anything was "fixed"? You only know that the operation has been changed.]

I strongly concur with the approach written at the start of this section. My summary would be: "Don't refactor to understand code. Write tests to understand code." UnitTestingLegacyCode is not a trivial task.

There's a project I'm trying to sell to a client that I'm a bit afraid of because of the legacy code issue. The existing system is written in very poorly structured FoxPro for DOS, and has numerous known, intermittent bugs that cause incorrect data to be written into the database. I currently have patches in place to fix up the problems when they are detected. The new system I would have them develop will certainly be in a different language, and with a different back-end, so I'll have to find a way to refactor across the chasm from FP for DOS to, perhaps Java w/ PostgreSQL.

If I'm going to write tests for existing code, that's going to be interesting. For one thing, how do you write tests for code that, unlike code written under TDD, was not written to be easily testable? For another thing, the old FP language is neither ObjectOriented, nor easily callable from external systems.

-- SteveJorgensen

I was in the same chicken and egg situation regarding hard to test code and refactoring. In fact, if I could think of a small, pithy name for the situation, there'd be a new WikiWord. The key is to realize you will not be smote by bolts from heaven if you refactor untested code. The cost is that you have to manually check to make sure you didn't break anything. After you're sure, write some tests to let you know if something there breaks with further refactorings. Use that nugget of factored, testable code as a stepping stone to the next convenient mess. Of course, the smaller and easier to manually test the first bit of code is, the better. I've only done this once, but it seems like it should work often.

We use a rather interesting object model kept just this side of the network transition layer (looks like an abstracted database connection that can only call stored procedures until one realizes that some of its "procedures" do things that stored procedures can't. Anyway, the object layer is full of bugs that reflect bugs in the procedures and bugs in the GUI frontend. There are (yes, still are) several cases where methods in the object layer do one thing in the base class and something completely different in the derived class. There are tests for nothing, but if something is broken, the GUI can be tested well enough to expose bugs in the object layer in a couple of hours (nice to have almost complete dependency, hmm?).

It is my task to add a new variation of the most commonly used thing. Generally, we know how it all works, so we could *attempt* to change it without fear of breaking unrelated code. But as I said, the object layer is loaded with bugs that reflect GUI bugs and worse. The refactoring took two stages, one for completely rewriting the load from database, and the other for saving to database. For both of these, I refactored the existing code, generalized it, and added the feature I needed. Both stages repaired some of the oldest bugs in our app, completely removing the need for some of the workarounds we needed for the longest time. The second stage exposed and removed several BugsWaitingToHappen? (e.g. saving null dates didn't work and aborted the rest of the save, but was never caught because ExceptionsCancelTransactions was not correctly implemented and dates happened to be the last question most of the time). There's still some leftover mess but not a whole lot that can be done about it.

I think we need a guide on what to do in the worst case scenario. And when I say worst, I mean WORST.

In other words:

All variables are global.
There are no functions, only subprocedures which act on the global variables.
Every subprocedure is at least 500 lines, some are several thousand.
Every subprocedure has more than one responsibility.
Copy-paste is preferred to writing methods, AND subtle changes, with side-effects, are made in the middle of the copy-pasted code.
If there are classes, they only serve as holders for single subprocedures.
Business logic is interspersed throughout the entire program.
And the kicker that makes me really have no idea what to do: The entire purpose of the program is to automate a third-party program which does not produce a testable output.

In my experience (adding features to a port of a rewrite of a 20-year-old piece of software), refactoring legacy code needs to be taken in small incremental steps:

Identify all uses of global variables, and name them appropriately (so it's easy to tell what's global and what's not)
Convert procedures into functions; replace all uses of global variables with function parameters that the callers must pass. Do this iteratively and eventually you will find the true owners of those global variables.
Break long procedures/functions apart into meaningful helper functions. Along the way you will find duplicate code that can be reused. Try to impose as much genericity as possible.
Procedures/functions that have multiple responsibilities should be broken apart to force callers to call multiple functions.
After side effects are eliminated and functions have single responsibilities, it should be much clearer where copy-pasting was done.
Create new classes/types based on highly related function parameters, break apart large classes so they each have one responsibility.
Isolate business logic from other code, even if they are often highly coupled. Reduce the interfaces so that they TellDontAsk.
Practice TDD and make the code testable.

See RefactoringLanguage, XroachMetaphor, TestPoint

CategoryRefactoring