Fix That Code Immediately!

You are working on that fresh project and you see a bad piece of code somewhere. The wrong way to approach it is “nah, that’s someone else’s code, I’m not doing anything about it”, “I don’t have time to fix that – I have other tasks”, “I’ll surely break things if I change this”.

The problem is – bad code accumulates. Even if the piece is a small one, it adds up over time and soon enough you have that “big legacy project that was written by some incompetent guys and no one wants to support”. Someone once said that in six months all projects are “legacy”, because they have a lot of accumulated bad code. Or in other words – technical debt.

That’s why you should fix it immediately. When you see some piece of crap, or something that’s not exaactly a great practice – fix it. Now. Or it will be too late, because then other code will start depending on that, and new code will follow the same practice (sometimes with copy/paste), and fixing it will be a nightmare. Let’s address the wrong statements above:

• “nah, that’s someone else’s code, I’m not doing anything about it” – so, what? You are in that project, you have the “right” to modify it. If the other person has written their code in a bad way, it might be that they even don’t know it’s bad – so they won’t fix it. And I don’t think they will get offended if you fix it. They might, but that’s not your problem.
• “I don’t have time to fix that – I have other tasks” – this is a task as well. And you can raise an issue/ticket in your issue tracker that says “refactor X”, and then log hours there. You can delay it until the next sprint (if agile). Problems with management insisting on making new things rather than fixing the old ones? Tell them to go read “Refactoring”…or Spolsky…or this blog. (It won’t help, but anyway)
• “I’ll surely break things if I change this” – possibly, yes. Hm, wait, you have unit-tests, right? And integration tests, and build-acceptance tests? If not – go ahead and fix that first. Then you won’t be so afraid of breaking things.

Code reviews are also important in regard to this problem. If all code that gets committed is code-reviewed, the chance that a bad piece will go in unnoticed is decreasing. It still happens, but more rarely.

The only problem with that approach is – how can you be certain a piece of code is bad? Well, here comes experience, knowledge of best practices, patterns. I can’t give you a recipe for that. But you should have a couple of people in your team that are capable of identifying bad code. If there is none – get Code Complete (and Effective Java (if your language is Java)).

So – fix that code immediately. It saves time and headaches, and makes you a bit more proud of the project, rather than “that’s some piece of crap that some incompetent folks wrote, I was just doing some tasks on the side”. Because you can’t say that – if the project is crap, it’s your fault as well.

Important notes (thanks to commenters):

• You should not change something just because you think it’s bad. Show it to your peers and technical leads. If it is something more than a couple of lines – discuss it more extensively and make a story about it. But do that as soon as possible.
• The advice is not about code that is complex and hard to read because of that. if (specialCaseX) {//do magic} is probably there because of some complex business requirement. If you want to improve things, research that and add a comment.

From http://techblog.bozho.net/?p=725