Refactoring a working project

Question

Let's say you got a project that is really poorly written, contains lots of code smells, wtfs, etc. Moreover, its code structure is so complicated that it is extremely hard to add any new functionality to it. On the other hand, the project works as it is supposed to.

You want to refactor the project, perhaps move it to a new framework, how would you approach this problem? Will you try to build a new one from scratch or use some techniques (specify) to convert the working project into a new one?

---

I would like to clarify the question a little bit as there is a confusion what do I mean when saying "refactoring".

I'll give an example about a car, think of it as a software project. Let's say you have built your own car. Its construction is pretty weird: engine is upside down, thus all the pipes are laid differently, electricity wires are tangled together and nobody has an idea where do they start or end, etc.

However, everything is working fine: you can easily ride it to shop, work, etc. However, its fuel consumption is a bit too high. Also, if you ever wanted to install new headlights to it, it would be a disaster with all the mess in the wires.

You cant afford buying a new one, so you have to refactor the car somehow: change engines position to normal, make the wires tidy, etc. You need to do this, because sooner or later you will need to change engine, headlights, install new stereo etc.. On the other hand, you still need something to drive you to work every morning, so you must make sure that you don't screw everything up.

Now let's go back to the project. How would you refactor the project as complicated as the car above, while not disturbing its primary function and purpose.

---

I would also like to make this a community wiki. Please edit.

So far the main tendencies are:

• Don't write a new one from scratch
• Unit test the hell out of it, then start refactoring small pieces
• Read Working Effectively with Legacy Code (Robert C Martin)

Links:

• Working Effectively with Legacy Code (Robert C Martin)
• http://www.joelonsoftware.com/articles/fog0000000069.html
• Refactor the low-hanging fruit.

Answer 1

First, create a suite of automated unit tests, and confirm you have high code coverage (70% or more). During refactoring, you will run those tests very frequently, in order to convince yourself (and Management) that you haven't broken anything.

No unit tests = no refactoring.

---

Let me change my mind a little. You don't need unit tests - you need high code coverage from tests that will be run frequently, as you change the code. These could be automated unit tests, or automated functional tests.

I do not believe that manual tests are an adequate substitute. They are much less likely to be run frequently during the refactoring process. Refactoring is much less likely to succeed without the constant reassurance that the code has not been broken in the process of fixing it.

Answer 2

This will give one perspective on rewriting from scratch:

http://www.joelonsoftware.com/articles/fog0000000069.html

Answer 3

You should read Working Effectively with Legacy Code.

Answer 4

Working is the only kind of project you should be refactoring. If you're fixing bugs, you're changing the behavior, and refactoring is explicitly about not changing behavior. But working has different definitions. The good one - the useful one for refactoring - is well-unit-tested. If you've got good test coverage (automated tests!), you're ready to refactor. If you don't...

Read Michael Feathers' Working Effectively with Legacy Code. Nibble away at the code. Pick a WTF that particularly offends you, and test the hell out of it, with automated unit tests. Then replace the WTF with something reasonable, ensuring that the tests continue to pass. Lather, rinse, repeat.

Refactor the low-hanging fruit.

Answer 5

Unit tests are a good thing. However, the code has to be in a certain state before you can even unit test it. I bet you have data validation that talks to the persistence layer inside the message-handling functions. I'm sure you have thousands of lines of business logic that interrupts the user with confirmation message boxes.

You have to separate the business logic from the user interface layer, and that can be a lot of work.

Now you've got a classic Catch-22 situation - you're not supposed to refactor it without unit test coverage, and you can't write unit tests until it's refactored.

So the only solution is to be very patient. Understand exactly how the application is supposed to work. Try and understand what each piece of messy code is doing. Accept the fact that sometimes you will not be able to make sense of some code, and the reason for this is that the code actually is pointless. But the pointless code will look exactly the same as the fundamentally useful code, and only extreme hard work will give you the insight to tell the difference.

Work towards the goal of having unit test coverage - but the lack of unit tests should not stop you starting. Accept the fact that you may never get the unit test coverage you want, but keep the ideal in mind.

And every day you will say to yourself "This would be so much faster if I just rewrote the whole thing". DO NOT ACT ON THIS FEELING. (See Joel's article, already mentioned).

Answer 6

There's a lot of text in response to this question that follows the literature very closely, and (with no offense to the posters of those answers) it makes me wonder if people simply haven't reworked a system of any significant scale that is really and truly Fucked Up.

I've rewritten three significant applications in my career (Significant: 50kish LOC or larger, data tier, logic tier, integration requirements). Every system has required me to say To Hell With This at some point to good practice. You know what I learned from doing that? At a certain point, it can be really, really safe. Absolutely, you need to consider what it means to throw caution to the wind, but it is significantly more important that you ship than that you follow someone else's idea of good practice.

Let me illustrate with an example of what I'm talking about:

I'm working on a system today that was written over six years that was originally converted to a .NET language from an application that was at the time perhaps a decade old, and was written with a DOS client that held all of the logic. The original code and most of the subsequent updates and conversions were handled by inexperienced programmers. This is a document management engine for all intents and purposes, and there isn't a single abstraction for "document" anywhere in the code.

I was asked to implement a means to transfer files across a WAN such that it would function with the core routines of the system. I started out creating a nice little test client and server with decent practices wrapped around them, tests etc. I went through the core system architecture and looked for the right place to re-stub my code. All I found was big nasty chunk after big nasty chunk of copy and pasted code with tiny modifications that constituted units.

I started changing little bits of code, things started breaking, I reset my changes. I extracted methods and found that variables were scattered throughout the code and relied on changes made throughout the procedure. I tried extracting classes, only to find that I was extracting methods wholesale and that the state data from the old class was, once again, scattered throughout the code, and not amenable to transition.

So I said THWI. Two weeks later we had a nice little zipping client and server and our core code was much more nicely factored for our trouble.

If you know a system, if you pay attention, if you test your code, and if you PAY ATTENTION, there's often nothing wrong with making big changes in an unsafe way.

I'm gonna get downvotes for this, but Agile practices have clouded people's vision too much, and it's really time for quotes from the literature to stop dominating these discussions. If you work with a system every day, you know the system, and no amount of unit testing is going to equal what you can make happen if you take charge and fix the damn system.

Of course, this is why you need to work with a system and learn the system instead of just going from technology to technology. Rewriting to a new language is not nearly as good an answer as rebuilding with an eye to improving the design. If you then find there are limitations in the system that you could overcome with a new technology, it becomes substantially simpler to make that change at that point.

Answer 7

I'd opt for refactoring rather than rewriting wherever possible, because it's less risky. If done right, you'll always have a working program, and it'll gradually get better as you make incremental changes to it. If you have to stop early (due to time or budget constraints), you still get the benefit of the work that's been done so far, because the program is still usable and it's better than it was before, even if it's not perfect yet.

On the other hand, if you opt for a rewrite and have to stop partway through, you're left with the original working-but-bad program and the new clean-but-incomplete program. You'll have to decide whether to scrap the new version and let the effort go to waste, or sacrifice functionality and gain bugs by switching to it before it's ready.

Answer 8

First, I agree with the other posters - do not give in to the temptation to rewrite from scratch. The code does work, after all. Second, and it may sound obvious, but concentrate your refactoring on those areas that need modification (for new features, etc.). There may be code that's crying out to be refactored, but if it's working and in a stable part of the app, leave it alone.

Working Effectively with Legacy Code does a great job of addressing the catch-22 that you don't want to refactor without unit tests, but you can't introduce unit tests without refactoring. Among other things, he suggests relying on automated refactoring tools to avoid introducing bugs when refactoring without tests.

Others may disagree with me, but IMO sometimes you you have no alternative but to do the initial refactoring without unit tests, and rely on automated or (careful) manual integration tests to make sure you got it right. The initial refactoring should allow the code to be run in a unit test harness, than you're on your way.