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.
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.
This is worth about a comment on Reddit.
I was taught – in the military no less – ‘Never pass a fault’. Good advice.
Boy Scout Rule: “Always leave the campground cleaner than you found it.”
By Uncle Bob Martin
Great article, I agree with the sentiment, but the choice isn’t always so black and white.
There are some things which can’t be tested (POM’s) for example. You could have some very weird looking dependencies and while you might be almost sure that it could be thrown off, there always a off change that some corner case dependency that is not obvious.
Test cases aren’t always present or correct. While adding test cases seems logical – its pretty hard to do so (confidently enough) for a method that has a CC of 40+.
I would never feel confident re-factoring such code even if I wrote a test case!
It easier to talk about eliminating the people’s angle but that doesn’t work out in a team either –>
“And I don’t think they will get offended if you fix it. They might, but that’s not your problem.” That statement doesn’t always cut ice!
What I do like to do personally is “Always leave the campground cleaner than you found it.” That’s good advice and helps improve off the code base a bit at a time!
@Sam – I agree it is not always obvious what is wrong and what not. And certainly I wouldn’t touch a method with CC of 40+ without dedicating a lot of my time to it (and speaking to its author). But the corner cases, strange dependencies, etc. should be documented. Like “This is here because of…”. When you write strange code, you know you are doing it. And you should leave a comment for others.
Wow, that’s horrible advice.
If every engineer immediately fixed everything they thought was a problem when they came across it, nothing would ever work.
“There’s a subtle reason that programmers always want to throw away the code and start over. The reason is that they think the old code is a mess. And here is the interesting observation: they are probably wrong. The reason that they think the old code is a mess is because of a cardinal, fundamental law of programming:
It’s harder to read code than to write it.”
http://www.joelonsoftware.com/articles/fog0000000069.html
So fixing everything immediately would probably be useless most of the time because the original code had to work within constraints you don’t know about up front.
Especially in a long lived codebase, styles, ideas, and patterns change over the years, so what might have been a kick ass idea 10 years ago (*cough* XML-based IoC *cough*) seems stupid now. But at the time was a best practice and was architected and tested and lived in production and should be left alone.
And then what may seem like a trivial refactor could trigger a large testing effort.
“Hey guys – I rewrote the core server’s thread scheduling to use the new concurrency library from the fancypants project.”
“Uh…so now we have to regress everything on the server?”
“Naw, it works fine on my machine, we should be fine. Trust me.”
Better advice would be to note everything that looks horrible, put it in as a research spike in a backlog, and then decided to do the actual refactor after the research spike is done and shows the refactor is necessary.
@old_hacker_guy yup, perhaps I should add a paragraph clarifying that. By all means, you should not go ahead and change everything you don’t understand. That’s obvious.
You quote Spolsky, and I mentioned him – yes, I’ve read that article, but it’s about different things. I’m talking about the bits that are wrong. That should not be done that way. For example (Java): injecting the same class in all controllers, rather than in their base controller. Or – using a mixture of DateTime, Date, String, Calendar. Or using collections without generics. Etc, etc. And Spolsky is talking about the moment, when reality forces you to write if (specialCaseX) { // do magic }. You don’t change that code. It’s hard to read, but it’s obvious that it makes something because it should, and not because the guy who wrote it had no sense of programming style.
I agree, but sometimes people can be really over protected about their code. Once I even had to listen a rant about code ownership and stuff like that (even when I was actually fixing the build!)
World is crazy 😛
What about working with unreasonable deadlines? Sometimes you work in projects where you sistematically get assigned 3 days to do a task of 4 or 5, this happens a lot.
That doesn’t give much time to refactoring. I personally go by the rule of leaving the code a litle bit better than what we find it, but just a litle bit: add logging, error handling, javadoc, rename a method, a variable.
Starting bigger refactorings than that on the middle of some underestimated task will just be a source of further delays and loss of focus.
I usually leave bigger refactorings to the beginning of the sprint, while analisys is being finished and we are doing some code QA.
Concerning TDD I practice it often but am not religious about it. If the program is not designed from the ground up to be testable, making it testable by splitting it up in small testable components can be in itself a huge refactoring.
And how can we do such refactoring safely without tests in the first place? It can be a chicken and egg problem.
Adding first unit tests to such a program is very difficult, because they tend to become ‘integration’ tests that are hard to write and maintain.
Or – check whether it’s in the bug database and if not add it. Then it gets prioritised against all other tasks like it should.
Yes, of course you should put sense in that. People are saying I’m wrong because “you shouldn’t randomly fix code”. Obviously. But you must fix it as soon as it makes sense. And that’s not “some time in the future”.
Just out of curiosity, what is your opinion the famous patch to OpenSSL that destoyed security of gazillions Debian systems all over the world?
I don’t have an opinion as I don’t know about that patch. But I guess it hasn’t been thought about enough. Extra caution must be paid in such critical areas like security.
I’m pretty sure that the author of that idiotic patch thought it “made sense” when s/he wrote it. And it passed all tests.
The fact that security was involved is just an effect multiplier. If instead of compromising the whole system (like it did) the patch was going to make some mp3 play badly it would still have been a bad patch, breaking code for no reason except perceived “beauty” for the eye of an ignorant.
If you find some piece of code that seems bad, re-read it again and again, until you convince yourself it’s indeed the correct thing. If you cannot and you can bet your house and firstborn that it’s really a mistake then contact the code author if you can and ask him/her about it and only then do the change.
Primum non nocere.
That’s why I wrote you should consult peers and tech leads. Your opinion alone may be incorrect, because you lack context, or other things.
Good piece of advice. My only problem with the legacy code written in ASP classic is creating unit tests- I don’t think there is such that could automate testing in this language.
The other day, while I was at work, my sister stole my iPad
and tested to see if it can survive a 40 foot drop, just so
she can be a youtube sensation. My iPad is now broken and she has 83 views.
I know this is totally off topic but I had to share it with someone!