On "Working Effectively with Legacy Code"

In the last few weeks I have spent most of my time working on a legacy system which is going to be partly replaced by a new one which greatly broadens its scope. But what exactly is legacy code? Michael Feathers says that

To me, legacy code is simply code without tests.

It is as simple as that:

Code without tests is bad code. (...) With tests, we can change the behavior of our code quickly and verifiably. Without them, we really don't know if our code is getting better or worse.

Alas, too often the answer is not the one we'd like to hear.

We started out so well

Almost every system, if you have at least decent programmers working on it, starts out as a wonderful green field: defined architecture, code conventions, solid principles, patterns and practices.

After a while the gardeners get busy with other gardens, and a couple of weeds start to poke out. Then some more, then more. And more. Meanwhile seasons pass and rain falls, so our green field gets muddier and muddier. Actually it looks more like it has rained cats and dogs, day after day after day. Mud becomes quicksand, and nobody dares to even get close to it.

The (in)famous spaghetti incident

Too soon the project reaches the point in which whenever you pull a string you have an undesired and often undetected change in a completely different part of the system. When I want to sound important I refer to this as the butterfly effect, a metaphore used in chaos theory. Yes, I know that your projects are different and you have never seen it happen, so try to use your imagination and stay with me a little more, will you?

That was exactly where I was, all tangled up in obscure dependencies: the perfect spaghetti code.


As I tweeted, I love spaghetti when they are in my dish, not in my code, so I had to do something (even because deadlines are always waiting in ambush). To sharpen my tools I retrieved my copy of "Working Effectively with Legacy Code". The book covers many aspects of what we developers have to face daily: instead of the "write once, run anywhere" mantra, we have to deal with "write once, read it at least ten times", but too often our code is obscure. Yes, our code. And we don't have tests, or at least we don't have enough. Do you have a complete coverage? If you do, you have all my respect, otherwise welcome in the family.

How do we get out of this?

First of all, we have to get our code into a test harness, but it is not as easy as it sounds. The book contains a series of 24 different dependency breaking techniques, each of which presented in several almost-real situations, to minimize the impacts of changes and ensure that we are not breaking anything. It sounds like the classic catch-22 thing:

When we change code, we should have tests in place. To put tests in place, we often have to change code.

This is where the techniques come in handy. I was pleasantly surprised when I discovered that, after some years of TDD, almost every problem and solution described in the book sounded familiar, so I skimmed it more than actually reading it. I still remember when I first read it: man it was hard, not the writer's fault, but because of the reader.
One of the most important things is that you have to preserve the behavior of your system when you refactor it (and preserve the rest of the system when you introduce new features or change existing ones), so you should at least know what it does. What is better than some characterisation tests?

Cleaning it up

I love NetBeans. I always have, since it was Forte4Java. Now I love it even more because it has a heap of automated refactorings that make "Refactoring" needless (ehm... that's not true, NetBeans deals with the mechanics, but you should know what you're doing, so stop reading and go buy your copy if you still don't have it. Done? OK, let's get on).

Do you need to create a new test class? nothing simpler, just a simple SHIFT + CTRL + U and a template with all the skeleton methods is ready for you. Just punch in the starting conditions and the expected results and you're done. Feathers describes all refactorings steps by steps, but a few keystrokes are all NetBeans needs to extract superclasses, methods, interfaces, pull up or push down members and methods, delegate, and so on and so forth. Nevertheless, you should know what you're doing.

While working your code could get uglier in some places. It could be temporary, or temporary in the Italian way (which means definitive). Even in the latter case, at least you would have tests in place so that you know you're not breaking anything.

In several places Feathers suggests to reduce incapsulation to put code into a test harness. Somebody might find it strange, extremists might find it insane. Yet...

Encapsulation isn't an end in itself: it is a tool for understanding.

Remember when I wrote that you write code once and read the very same code over and over? What 's the point of an encapsulated design if I spend hours for the most trivial task? Mnd you, I'm not suggesting every field must be public and collections should be directly exposed, but nobody is going die if a private method becomes protected for the sake of testing.

Where have my spaghetti gone?

After a while, one after the other, your pull spaghetti out of your dish, slowly replacing them with neat and tidy sushi.

Before you realize it, you will look at your codebase with different eyes, and chances are that you will enjoy dealing with legacy code. Don't enjoy too much though, even if you have a lot of technical debt to pay: you don't have to pay it for the sake of doing it, but because it allows you to move faster when you need it (and you know you will need it).

Let me stress this again: if I didn't have some hundreds of tests already in place, these weeks would have been months. Baby steps, nip and tuck, red and green bar. But I only introduced tests and refactored legacy code when I needed it, otherwise I would spend too much time on activities that were not a priority. See something rotten? pen it down, fix what you're doing and if you still have time you can deal with it. Look for decisions that could change, but change your code just in time (the YAGNI rule rules).

How to remove newline chars in SQL

I'm working on the migration from a legacy system to a new one, and the source database is filled with presentation details like newline characters and HTML tags. To get rid of all this stuff (which doesn't belong in there) I used a simple yet effective custom function:

create function removeNewLineChars(@source varchar(150))
    returns varchar(150)
as
begin
    return replace(replace(replace(@source, char(10), ' '), char(13), ' '), char(9), ' ')
end

As the source and target database are Microsoft SQL Server, T-SQL is the dialect of choice.

Let me say it again: presentation details do not belong in data, at least not in an enterprise system. Unluckily, you see it a lot in old applications (and, alas, even in some new ones). You can shoot me for that, but I won't change my mind.

Tangled and circular dependencies

Back to the cyclical dependencies problem I wrote about one year ago, the other day I finally tackled it (15 years spent playing rugby must have taught me something), as it was left hanging in the air for too long. We started out like this:

Basically project B uses the domain model and persistence services from project A, which in turns uses transport services and a the domain model from project B (no, it is not the same as above, because the model B is just based on model A but it is managed from a different perspective). Let's not forget to say that presentation, model and services are all tangled.

A disclaimer: this is not what I'd call a refactor, just a different way of organizing projects thanks to NetBeans features (no Maven complexities, thank you).

I first extracted the core domain classes of project A used in project B and moved them to a new project, let's call it A Core, then had both project A and B depending on this one. Sounds easy, but it wasn't... it was quite long and tiring, and it required a little tweaking each now and then, just like the subsequent passages. That lead me here:


Now that could look good, but even if it is better it was not enough because of some legacy installations in which Project A must be compiled using JDK 1.4 and run on a Tomcat 4, but project B has a JDK 1.5 source level (Tiger, or whatever it is called now) and runs on a Tomcat 6. This forced me to extract from Project B all classes on which Project A depended to move them on a new project, let's call it B core. Nip and tuck, here and there, to land that prince with the perfect hair, and here we are:


Now all cyclical dependencies are gone. Packages with the very same name exist in different projects, leading to a namespace which "doesn't talk" to the developer; on the other hand this can bring a lot of confusion, even if the compiler understands everything perfectly. That's where fun begins... I just began scraping the surface, now we have to dig in deep. The first step will be choosing the right names for packages, so to convey the rationale behind their repositioning. The next one will be moving related classes left untouched by the dependencies game but logically belonging to one of the "core" projects. Other steps are still up in the air...

Moral: always clean up your mess. BTW, it does not matter if you just inherited it. Now it is yours, and it is you the one who has to deal with it. Leave spaghetti to gravy and keep them out of your code. There must be a reason if they say "clean code that works" and not just "code that works"...

Severance Pay part III

It's becoming just like "Another brick in the wall"... I didn't update after this post, but - long story short - we somehow delivered the required functionalities, more or less within reasonable estimates.

Now the customer has more needs. Same old story, same old act. But now we have an integration test and a little coverage... life should be easier.

Severance pay part II

Right, you spotted me. I closed last post with a joke. No, no, the scalping part was ok, I was talking about the estimates. You do have to estimate. And we did. And we went to talk to the management, reporting that the new feature would only require about a couple of days. If the code were reasonably clean and understandable, which is not. At this point we tried to explain why we need to spend five days writing tests and refactoring (actually ten days, as two team members are pairing on it).

The discussion went on like this (M = Manager, T = team):

M: How can we ask our customer to pay twelve days...
T: ...er... fourteen, we already spent a couple of days trying to figure out...
M: Exactly! how can we bill fourteen days for a feature that would normally require two?
T: That's right... but if you want to bill two days, nobody will ever pay us for the remaining twelve...
M: Well... the domain logic is very complicated, we could ask for five days...
T: Yes, but we would still need a couple of days whenever the customer asks to fix or add something just to reckon what's going on under the hood, and we can't always say we need more than we actually do... that would lead us to an unhappy customer... and...
M: OK we'll talk about it next week... I want to understand.

Now, what's the point? he is right. But we are right as well. And the customer too. How will our heroes sort that out? More on this will follow...

Severance pay

We need to add a new feature to a severance pay software. Legacy software. Very legacy. Normally we would discuss the requirements, inspect the code, estimate, sign an agreement, develop, release and get the money. We do not even need more than one iteration, as the change is relatively simple and small. All is well until we reach the "inspect the code" part (yes, just the second step...).

The code is a nightmare in which side effects rule the world, and you're not even aware of it. As time goes by, and your understanding of the code increases ("understanding" is a real overstatement) you start to realize that democratically elected objects and methods are no longer in power: the dark times of side effects have come, good code long time forgotten. And you are doomed to become insane. Of course, being already on my way to insaneness, I have an undeserved advantage. But that's a whole different story.

Well, we definitely have to understand this code. After banging our heads on the wall for some time we decide that the first thing we'll do is to look for the side effects and turn them into methods which names tell you the story, so if a method sets the foo variable it is called setFoo and not getTheValueOfThatFieldCalculatedForThatPeriod. Easier said than done, because side effects have side effects, which in turn have side effects and so on. The FIRST thing we have to do is to build a test harness (don't tell me you thought for a single moment the code had tests). Thus, as everything that happens deep in the dungeons of the code is a strange mixture of lore, smoke and magic - and an insane amount of luck - we decide to test the final output of the whole calculation. Ok let's do it.

First: let's create some test data, decoupled from the database. After all, we don't want to waste unnecessary time hitting the database (and we should all refer to the same one). That's long but easy, as many values and properties are required. while (not done) {Clicketi clacketi tic tic clack click clack} ok we're done. Let's write a simple test and let's check the very last value that the output page would display. Clack click clack... Run test...

...Error: xxx.persistence.XxxPersistenceException: pkg1.pkg2.pkg3.CoefficientTable: no default connection found.

NO DEFAULT CONNECTION FOUND? Whatthe... ok, ok, the application reads the coefficients used in the calculations from a database. We make a note to decouple it later, maybe the application will load them during the startup, we do not care where they come from, what we really want is to inject the coefficient data when we actually need to make the calculations. For now let's use a TestSetup and connect to the database (and disconnect when we're done). Running JUnit tests...

1 test failed... junit.framework.ComparisonFailure

ComparisonFailure? But if I actually print the page I get the right result... and the property I check in the test is the one we use for the output... What...???

A-ha! That's where we discover that the software that calculates the final values does not really calculates the final values, despite the name of the methods, because the class that generates the HTML of the output page (why a class? why not a servlet? why not a simple jsp page and a bean?) makes even more calculations. Yep, you guessed, based on side effects as well. Undeclared and inconceivable side effects, of course.

I do not dare to ask my team to estimate the tasks for the new feature... I do not even dare to ask them to estimate the time they will need to be able to estimate the new feature...

Let's close our eyes, hold our noses and refuse to listen... any free wall? we've got some heads to bang... BUT NOT OURS! My team members want the scalp of the original developers... and who am I to prevent them to be happy?

A legacy application: the story of the fridge and the oven

Today I'll tell a story which inspiration comes from an overheard conversation.

The situation: you have a legacy application which you use in your organization to provide services to your customers. You have no tests. It uses proprietary libraries. Layers are nowhere to be seen. A simple change requires a gargantuan amount of code. And sweat. And chills down your back. You know it will not be able to provide the new services the business needs without a huge architectural and model change.

A possible solution: start a new project adopting an agile and iterative method, focusing on the most important architectural aspects (an UP practice) and on the most valuable features (a practice you find in many methods). Incrementally substitute the old system with the new one (an EVO practice).

The solution from the management: learn everything there is to learn about this application: it is one of our assets, and as such it must be considered. When your experience is consolidated, we'll reuse the same framework for all our new projects.

You can consider this story from two different points of view. The manager, who has no development experience or knowledge, thinks that the problem lies in the team, and fails to acknowledge all the signals the team is giving him. He is afraid and wants to keep things under control. After all, if we have provided the service so far we can go on just as well. Why venture into a new project, with all the risks that new technologies imply, first of all the team's inexperience with them? Another perspective: the management might have dealt a sale of the product within a very short time frame - this might be a confidential information - and he knows - or he's afraid that - the team will not be able to deliver on time.

In each case, most of the times an honest conversation between the management and the team can lead to find a good, if not the best, compromise: a classic win-win strategy.

One last (funny? tragic?) note: the project manager, trying to explain the problem to the management, provided this metaphore :

"Look, we have this huge refrigerator. You need an oven. You're asking me and the team to learn how this refrigerator works, so that we can embed the oven within it. What we really need is a power plug which can work for the fridge and for the oven. Why would you need a fridge to roast a turkey?"

The answer:

"Your oven will have frosting features, which no other oven has. That will be a significant competitive advantage."

Legacy software and code coverage

According to Michael Feathers, software is considered legacy if it does not have automated tests supporting it. And, not surprisingly, there are (too) many legacy applications out there. And that raises a couple of questions: how do you change legacy code? yes, but how do you change it without feeling the urge to update your cv? Mr Feathers gives a lot of hints on changing code and dependencies-breaking techniques, presenting you with different problems and possible (and tested, of cours) approaches.
At the moment I am working on an "inherited" old (and, yes, legacy of course) application for which I'll have to check several chapters of Feather's book: I can't get this class into a test harness, I can't run this method in a test harness, dependencies on libraries are killing me, my application is all API calls, my project is not object oriented how do I make safe changes, this class is too big and I don't want it to get any bigger, I'm changin the same code all over the place, I need to change a monster method and I can't write tests for it, how do I know that I'm not breaking anything, and so on and so forth.
So... where to start from? The answer is quite simple: I have to write tests. OK, that was easy. Which tests should I write first? short question, long answer. Long story short: check the book for that. But how do I know what's covered and what's not? Thanks to Fabrizio I've discovered Cobertura, a test coverage tool which instruments the bytecode of your classes and produces reports about, well you guessed, test coverage; you can see a sample report here. Fabrizio shows how to integrate Cobertura in NetBeans, which happens to be my favorite IDE. I did it and... well... I have quite a lot of work ahead, but at least I have a new tool.