Dealing With A Legacy

Noel Rappin, Friday, September 5, 2008 @ 2:02 pm

Original Image from ATPM.com

A couple of months ago, I wrote about auditing existing Rails applications en route to potentially taking them over.

In a related story, guess what I've been doing for the past couple of months?

Here are some tips, tricks, random thoughts, and cathartic rants about dealing with other people's code:

Keep Focused on the Goal, You Can't Fix Everything

Odds are, you're poking around in this existing code base to solve some specific problem, some bug fixes, or add a new -- OHMIGOD that controller method is inefficient. Hold on a second, that needs to be -- OUCH that model totally needs to use named scopes. Okay, I'm going to get to that in just one -- HEY there are no tests here. One minute...

If you are like me, it takes an actual act of will to turn away and not clean up ugly code. In this case, though, you probably do need to put the item on a to-do list and get back to the issue at hand. Once you start pulling at loose threads, the chance that the whole thing unravels get pretty high, especially on legacy code that you didn't originally write. If you see things that also seem high-priority, it's fully appropriate to go back to The Powers That Be and discuss fixing those as well.

Still, you are actually going to have to change things, especially since there are bugs and features that you are expected to deal with. Here's advice for that.

Don't Poke Around, and Be Very Afraid

The first step is learn the system as best you can (you won't fully learn it until you start actually changing things). As crazy as the previous programmers may have been, they probably had some reason for any bizarre design decisions you see. Try to figure it out (if you are in contact with them, ask). This is another reason not to reflexively clean everything up in an unfamiliar code base -- what initially seems sloppy may have a reason. (And hey, if you are doing something that might seem sloppy to a future programmer, perhaps a small comment might help.)

What you don't want to do is barge right in and start moving things around. Check and see where each method is called (the RubyAmp bundle for TextMate is good for this, other IDEs have this kind of search as well). Understand the routing. If there are multi-part forms or processes, trace through the work flow. Use the site and watch the logs, using the query trace or footnotes plugins for more details on what's going on.

One Thing At A Time

When you do start working on the code itself, try very hard to work on one issue at a time, commit it and check it in separately. Sometimes it's possible to get the stakeholders to approve each change, more likely you'll get that done in bunches. Keeping each task separate (even possibly using a separate git branch for each task) makes it easier to back out if you hit a code landmine or if you've misunderstood the requirements. This is always a good idea, but especially so if with legacy code.

When To Clean Up

Despite my earlier claim that you can't fix everything, I think it is appropriate to clean up bad code that relates to the portion of the code you are working on. If the new controller action that you are writing depends on a model method that's inefficient, or duplicated code, or doesn't use named scopes, then you can and should fix that as long as it goes toward solving the problem you're working on.

The tricky part is when you come across potential showstopper bugs or security issues in the legacy code base -- plain text passswords in the log file, insufficient protection against URL sniffing, potential cross-site vulnerabilities. Stuff that's not just a question of the previous programmers having a weird style, but where they are actually factually incorrect. These really should be fixed as soon as possible, and sometimes it's best to patch them as you come across them, if the fix is very quick. It's still usually best to keep the rest of the project in the loop and let the client or whoever set priorities for fixing issues.

Test, Test, Test

For an agile, TDD kind of coder, the most important fact about Somebody Else's Code is that it won't have tests. If it wasn't written via TDD, odds are that the code is actively structured to make testing difficult. Never fear, TDD is here!

Get whatever existing tests are there running clean. Even if the original coders didn't actually write tests, there are probably a lot of Rails-generated tests, and odds are they are out of date. Having broken tests will be a continual drag on your own process, so it's worth the time to clean them up.

This should be part of your getting to learn the codebase process. But it's a little different from a normal test cleanup -- at first, you want to try to fix the tests without touching code, the goal here is just a clean test run. If you have to comment out a test at this point, go ahead -- this is not something I would normally advocate, but desperate times and all that..

Once the tests run clean, run rcov to see just how bad off you are. In contrast with normal practice, it can be helpful here to write some tests that just hit all the controller methods without making any assertion (or maybe just assert_response). At least with those tests in place you might find out if you've broken something really badly.

When you start writing new code, try to use TDD best practices and try to add tests that isolate bug cases. If you are adding new methods and actions, this isn't too difficult, but it gets tricky when you are augmenting existing methods. If you can carefully refactor your new code to a separate method that's best, otherwise you may get caught with just tests that prove things don't crash again.

Stub objects can be very helpful here for untangling dependencies that are a little too tightly coupled to make testing easy. By stubbing out any calls from within the long tangled method you're trying to fix, you can test all the different logic paths, even if it would normally be difficult to set up a non-mock environment for the call.

To Update, or Not To Update

Should you bring your legacy code up to current Rails and plugin versions? I wouldn't even consider this until after the test suite is running and has some coverage. I did just recently move a legacy site from 2.0.2 to 2.1, primarily for gem bundling and because named scopes are awesome. You need to be careful with this -- there aren't many compatibility issues from 2.0.x to 2.1, but there are a few (here's a good resource).

A longer upgrade from 1.2 to 2.1 is more likely to break things, but also provide more potential benefit. It's probably worth trying the upgrade on a branch without checking back in, to get a sense of how big the issues are.

Deploy Issues

As crazy as the programmers who worked on the project before you may have been, whoever put together their production environment is likely to be crazier. Get to know the production environment as best you can, but if the app is already in production, then now is not a good time to make wholesale changes in the setup. If there's a capistrano script, understand everything it does before you try a deploy. Allow a lot of time and resources for that first deploy to the legacy server, since the odds that something will pop up are really high. If possible, you should try to do your first production deploy quickly, before there are that many changes, it'll be easy for everybody to verify the fixes and will give your team knowledge of the process.

agile ajax

Comments

• Leave a comment
• Comment feed for this post
• TrackBack URL

None so far.

Leave a comment