6 things I hate seeing in legacy code

I'm positive I've ranted about maintaining legacy code before, but I going to do it again. At least this time I'm just reading the code so I can understand it and rewrite it rather than having to apply more bandaids.

Oh yes, and I've jumped on the "Top X List!" bandwagon. It's cosy up here. So in no particular order, here are my top 6 things I hate seeing in legacy code:

1. Comments and variable names that didn't update with the code

Yes, the code changes as you write, I understand that. But seriously, can you please update the comments to reflect what you've done?

It's very confusing seeing TODO comments for things that have been done, or comments about things that should be considered before the code goes into production. Or maybe there's a variable called currentIndex which is now actually a string containing a key because the original programmer changed from an array to a dictionary.

These things sometimes take ages to work out. For example, I recently spent several hours trying to figure out what was happening in a chunk of code. Looking at the following line:

strMessage += failedMessageList + NXT;  // not implemented yet

It turns out:

It was in fact implemented (and was very, very necessary)
The failedMessageList variable contained all messages, not just failed ones

So seriously, if you change your code - take a glance at the comments to see if they need changing too.

2. Poorly named files

I had to add this one in. Not because it's a big problem very often, but because when it occurs, it can be really painful. It's obviously frustrating when classes and variables are named badly, but if files are named badly it can be even worse.

Let's say you're reading through the code and find a line that instantiates a Person class. Unfortunately, there's no Person.cs (or whatever language) file. After some searching, you find that the Person class definition is in Teacher.cs - a file that wasn't renamed when the code changed.

Or perhaps you're looking for the UI for the window titled, "Database Maintenance" and have to work out that it's actually a class called Restoring which is found in a file called FrmBackRest.vb. Sadly, a true story.

3. Inconsistent ways of doing things

I don't have a problem if you do something a different way than I do; that's fine - we all have our preferences. I might prefer to write to the database with parameterized stored procedures that I've written myself, while you use an ORM and just call a Save() method on an object you've just updated. That's fine - both work.

What I really don't like, though, is when someone does both. Like, in one place they're calling stored procedures, in another they're building SQL queries on the fly, and in another they're retrieving DataSets via queries, updating the values, and calling a Save() method.

Nothing screams I-just-hacked-this-together-without-a-plan as much as large inconsistencies like this.

4. Even legacy-er code in legacy code

Sometimes I'll come across legacy code that makes it clear that the original developer came from an even older background and didn't quite understand this new technology. As a .Net developer, the most common stuff I see is good old VB6 code in a .Net app.

I came across a Goto statement in a bit of .Net code the other day. Another true story. It was a few lines after "On Error Resume Next".

5. Error handling that hides errors

This is pretty common, and unfortunately it's really only something that gets noticed when you have a problem. While things are working perfectly (and you don't care how it works), there's no problem. As soon as there's an error though, you can't find any details about it because the error handling is crap.

For example:

try
{
    submitResult = transmitter.SubmitMessage(submitInfo);
    if (submitResult.isValid())
    {
        sentCount++;
        upDateStatus();
        return submitResult.Reference;
    }
    else
    {  
        upDateStatus();
        failedCount++;
        try
        {
            string error = GetErrorDescription(submitResult.ErrorCode);
            throw new Exception("Can't sent the message, error: " + Environment.NewLine + error);
        }
        catch{throw new Exception("CONNECTION_ERROR");}
    }
}
catch(Exception ex)
{
    ErrorLog.write(ex);
    upDateStatus();
    failedCount++;
    throw ex;
}

The end result of this? Here are some examples:

The transmitter.SubmitMessage method throws an exception. It gets caught, written to an error log, and thrown again. Ok, that's not too bad...
The result from the SubmitMessage method comes back invalid. We increment our error count, get the details of the problem, and throw an exception. Then, oh no! That exception gets caught immediately, and another incredibly generic "CONNECTION_ERROR" exception gets thrown - completely abandoning the information gathered. But wait, there's more! That exception then gets caught and logged and the error count gets incremented again! Yay! A "CONNECTION_ERROR" occurred... no more information... and an incorrect error count...

See? Don't do it. If you can't test for proper failures (sometimes it's very hard), at least think about what's going to happen.

6. Unused tracts of code

This one comes back to updating everything relevant when you have to revisit your code. A lot of the legacy code I've seen has codepaths that can't possibly be used, methods that are never called, and in some cases classes, modules, and even entire libraries that aren't referenced.

A recent project even had two code files, Connection.cs and Connection2.cs - they had different class names (Connection and Connection2 predictably), but they had the same methods with different implementations. Connection.cs was never used.

If you have to traverse your way through hundreds of lines of poorly-written code, it can get very frustrating when you find that half of it isn't even used. It's even more frustrating when you're trying to match the behaviour you're seeing with the code you're reading, only to find out that it's actually a completely different near-duplicate that's running instead.

And that's it - my top 6 annoying legacy code things. Feel free to comment.