Senior Java developer, one of the top stackoverflow users, fluent with Java and Java technology stacks - Spring, JPA, JavaEE. Founder and creator of http://computoser.com and http://welshare.com . Worked on Ericsson projects, Bulgarian e-government projects and large scale recruitment platforms. Member of the jury of the International Olympiad in Linguistics and the Program committee of the North American Computational Linguistics Olympiad. Bozhidar is a DZone MVB and is not an employee of DZone and has posted 91 posts at DZone. You can read more from them at their website. View Full User Profile

Fix That Code Immediately!

12.09.2011
| 5854 views |
  • submit to reddit

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

Published at DZone with permission of Bozhidar Bozhanov, author and DZone MVB.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

Fabien Bergeret replied on Fri, 2011/12/09 - 3:28am

I partially disagree with that. Yes, if you see something strange in the code, you have to do something about it, because the project responsibility is shared among the whole team. However, maybe you're not the best resource for fixing it, maybe be you don't know all the context (some could say, that's what docs and comments are about, but, hey, we're in real life ...). So, if what you see is obviously wrong, fix it. If it's only strange, send an e-mail to the guy who did the job ('blame' functionality of SVN is great for that). Another possibility is to design a fix, and share the design with the original developer for approval. Anyhow, I agree with the global statement, which is to solve the issues 'on the fly'.

Bozhidar Bozhanov replied on Fri, 2011/12/09 - 3:44am in response to: Fabien Bergeret

My comments at the bottom address exactly these things: you shouldn't do it all by yourself, silently, and you should not consider every "strnage" piece to be bad.

Mladen Girazovski replied on Fri, 2011/12/09 - 5:04am

Regular code reviews are unfortunately not practised enough, it can help a lot if experienced people would review the code of others before they commit to SCM, ie. before (bad) facts manifest themselves.

Using static analisys  tools regularly like findbugs would also reduce the amount of obvious bugs in the code.

Personally, i'm not a friend of code ownership, it creates the illusion that people can work in their own little comfort zone where they are free to do what ever they want because nobody else will read and/or change their code and they rarely have to justify the decisions they've made. The reality is, problems that one developer creates are problems for the whole team.

 

Lund Wolfe replied on Sat, 2011/12/10 - 9:12pm

I agree that you should fix as you find it if it's a quick readability change (or trivial rename refactor) or adding comments to aid in maintenance. A low quality project will be loaded with bad design and bad code, though, so you need to be judicious is applying time to refactoring.

I often find most of my time is spent in refactoring but it is done when it simplifies a needed fix or enhancement so it is justified. On many projects you can spend an infinite amount of time refactoring, so apply your time where it will make a difference to the users (including minimizing later maintenance effort in same area), not just yourself, your team, and future project developers.

I think you are specifically addressing new development, though, where there is a limited amount of bad code, bad practices already implemented. It is certainly better to correct the issues, including bad design, As Soon As Possible, before it becomes a reusable standard/example (as you said) in the existing code, slowing down forward progress and project quality.

John David replied on Thu, 2012/01/26 - 3:08am

Its always hard to read, understand and correct someone else code if it is poorly written. Now there are 2 points.

1- If you spend time in understanding and correcting this code then it is wastage of resoruces.

2- If you skip the code and avoid correcting it, it may cause big issues in future specially when other modules that are based on this code.

Java Eclipse

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.