Conundrum: During the course of working on a new feature or fixing a defect, you find a legacy problem in code. What should you do? Fix it and risk altering the behavior of the code. It has either been working up until now by some fluke, or else the defect has not been detected or worth anyone's time to report. Should you leave it alone and allow the problem to make the code harder to work with later? Fixing the problem will only add to the time of your original task and force you to regression test. Few will appreciate the work. Fixing it, however, seems right somehow. Code with fewer problems is easier to refactor and build upon.

I've been finding myself in this situation time and time again as we work to modernize a web application. I can't tell if I'm being obsessive or honorable when I go off-tangent working on these old bugs. How do you handle these situations?

11 Answers
11

I work on a very small team, so it kind of depends on what the change is:

If its a small, obvious bug fix, I definitely go for it. I also throw in extra comments if I have to work through someone else's code and other little improvements that fall under the "boyscout rule" to me.

If the code is so entwined that you have to ask "Will changing this break something and require testing" then no, you shouldn't change it. Bring it up in your bug tracking system if it worries you.

This, incidentally, is why I try to code smaller methods with more obvious type-signatures as well. If you know there aren't side-effects and can make the ins and outs match, you can fix, rearrange, or tweak any of the interior code without risk.

But don't feel like lack of appreciation is a reason not to fix bugs you find or to improve the code base for any reason. If nothing else, you're being kind to the future you who will assuredly be back in there to fix something else.

EDIT:
You also need to watch your time on the project. Obviously under tight deadlines, you need to focus on getting the main work done, but if you're just under "normal load" then I think a little cleaning up here and there makes everyone happier in the long run.

If there are plenty of unit tests, so you can be fairly sure you haven't broken anything, fix it.

Otherwise, add a //TODO, add it to your bug tracking, whatever

Basically you're doing a risk assessment: what's the risk of changing vs not changing. If you don't feel like you've got enough experience (with programming in general, or the system in particular) ask someone else in the team.

If you don't fix broken windows then there is a tendency for them to create a downward spiral of code quality. And the more of them there are the bigger job of fixing them, and therefore it is less likely that they will be fixed.

Whether to fix them now or later is up to a matter of judgment. Is it a simple fix? Are you sure the code is doing what you think it is? Is it likely to distract from your current task? Are you under time constraints? Is it likely to introduce more bugs?

At the very least, mark the item in your tracking system and make sure it gets fixed later. It is a important to mark it in the tracking system even if you decide to fix it now, to make sure that it is tested as well, and to document changes.

If it's an obvious bug, such as something that will violate security, corrupt data, or raise an exception that gets displayed to the user, then fix it. Otherwise, ask someone who knows the codebase better than you.

Seems reasonable. What about something that is seemingly minor, like malformed HTML that a browser rendering in Quirks mode tolerates? The bug, in this case, is doing little harm, but I know that it will make life harder down the road if some new content/plugin requires the page to be rendered in standards-compliant mode.
–
CoreyOct 26 '10 at 20:57

@Corey: Yeah, that's the sort of thing you'd want to consult a more experienced developer on. You have an opinion, and I agree it's probably the right decision, so present your case, but bear in mind that there might be other factors you're not aware of that the guy who's been working on this for 5 years understands.
–
Mason WheelerOct 26 '10 at 21:19

It depends, if its a small bug where you are certain that your fix has low impact then personally i would fix it as part of the other work and then let the PM know.

If it has any risk to client, users or company bring it up with the Project Manager and discuss a course forward. Its their job to evaluate risk so bring it to their attention and the case for fixing it. Then honour their decision.

Our testers hate this. Unless it is very trivial we log it in the bug database, then allocate it to a release and write regression tests. If the developers are just going to making changes that are not on the schedule how can you ever keep a deadline?

I've been on teams where non-critical defects or standard-violations are submitted as a "Weak Code" defect. I would say that the person who finds a critical defect has a responsibility to throw some kind of flag

it depends on the bug. the main concern is introducing new bugs. Better to deal with a known issue rather then an unknown one. If its simple, say a text change, or a simple logic error, we fix it, otherwise leave it alone.

One thing to note, tho, we are a small shop of 4 devs and an intern and the bug i fix is probably the bug i created.

If the code is obviously wrong, the fix is fairly straightforward and you believe the risk of impacting the users is low, then go for it. It comes down to professional judgement.

You have to remember though that if you've found it then presumably the users haven't, or else they'd have reported it. Rather than spending time fixing an issue that may never be encountered by a user, you might be better off spending that time fixing issues that are causing your users problems now.

Well document the observations first, and decide whether to fix it later.

Have some formal discussion (e.g, in the regular meeting) or informal discussion (e.g., during the lunch) with your colleagues, and make the changes after you gain more confidence on the behavior of the codes you are going to fix.

Although it seems a bug/defect to you, it may actually be a "feature" this time. It could be a poorly implemented solution to get around some corner cases at the last minute of the previous release, and your "clean fix" may revive some previously resolved issues.