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:
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:
{
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.
Follow me on Twitter
Nice list:) I think I’ve seen all of these, and been guilty of most (all except #5 — swallowing exceptions is a shooting offense!).
I think a lot of them arise from developers who live in fear of making changes to legacy code — e.g. leaving code commented out ‘just in case’ someone needs in the future, or having the guts to rename a class, but not having the follow-through to rename the file and update comments.
I’ve recently been reading Robert Martin’s book, Working Effectively with Legacy Code, and it sort-of deals with this — do anything you can to get a method or a class into a unit test, then lean on it to give you confidence to refactor it _properly_.
These are things I hate to see in any code. Not just legacy ones
Hey Damian. I think this list really hits some key mistakes coders make when they don’t think of the next coder that will work on the same files later. Possibly even themselves. I work in a large retailer’s technology division and because the business is ….old.. to say the least, the code is like this EVERYWHERE.
It’s just nice to see someone else has been there.
Thanks all for your comments
I think every developer has been there at one point or another. Nobody wants to work on old code, but it has to be done!
Robert Martin’s book, Working Effectively with Legacy Code, and it sort-of deals with this — d
Layers upon layers of legacy, yep.
I would add: huge functions or methods. To me, that’s anything more than about 20 or 30 lines. I’ve had to work with code containing 2000-line functions, and not for any good reason.
BTW, “Working Effectively with Legacy Code” is Michael Feathers’ book (it’s in a “R C Martin Series”).
Damian, I found your site by accident trying to find a contact email for Dan at The System Works. Very insightful blog on passwords etc. In not very technically minded but it was an interesting read anyway. Good luck with your project.
Arthur K