66

I was looking over some old code that I wrote. It works, but it's not great code. I know more now than I did at the time, so I could improve it. It's not a current project, but it's current, working, production code. Do we have a responsibility to go back and improve code that we've written in the past, or is the correct attitude "if it ain't broke, don't fix it"? My company sponsored a code review class a few years ago and one of the major takeaways was that sometimes it's just good enough; move on.

So, at some point should you just call it good enough and move on, or should you try to push projects to improve old code when you think it could be better?

8
  • Are you doing this in your own time or on the company dime? Feb 17, 2012 at 23:35
  • 44
    most places your first responsibility is to add value to the company. If you are doing anything that doesn't add directly to the bottom line anything else is wasted effort. Old tested working code is tested working code, touch it to make it better just as a gold plating exercise and now it becomes new untested and thus un-working code, which isn't adding value to the bottom line.
    – user7519
    Feb 18, 2012 at 8:17
  • 7
    Ask your boss what he wants you to do. You will most likely be told to leave it alone. Also, code changes should in my experience only be done as part of an actual delivery. Random changes over time have too large a probability of introducing errors, which takes you too long to fix as you do not have the changes fresh in mind.
    – user1249
    Feb 18, 2012 at 8:32
  • 1
    Add some comments about possible improvement in the documentation and leave it at that?
    – James P.
    Feb 18, 2012 at 17:13
  • 2
    Write unit or regression tests. Don't change the code, but make it possible for someone to come along later and make needed changes confidently despite not being familiar with the code. Feb 20, 2012 at 23:51

16 Answers 16

67

Unless you will be making changes to this "old code" to fix bugs or add features, I wouldn't bother improving it just for the sake of it. If you do want to eventually improve it, make sure you have unit tests in place that will test the code before you start refactoring it.

You can use the "old code" to learn better practices. If you do this, it's a part-time learning experience and you shouldn't expect that code to replace what's currently released or deployed by clients/customers.

8
  • 4
    What I was thinking is "software rot" and broken window theory from the pragmatic programmers and the effects of software entropy. pragprog.com/the-pragmatic-programmer/extracts/software-entropy
    – jmq
    Feb 17, 2012 at 21:55
  • 5
    Unless you will be adding value to the company by refactoring code that already works and is in production, it will be difficult to justify the necessary time to remove the "software rot" to your superiors. Only do this if you will be actively working in this code, otherwise there isn't much benefit other than the refactoring experience you would gain.
    – Bernard
    Feb 17, 2012 at 22:03
  • 11
    @jmquigley, software only rots if its source code is changed, and broken windows have an effect only if they are seen. An old codebase, if there is no need to touch it, will work the same way for as long as its environment allows it. Feb 17, 2012 at 22:23
  • 2
    Try not to introduce error into a working program in the process of "improving" it ;-) Feb 17, 2012 at 23:12
  • 4
    I think code 'rot' is an unfortunate metaphor. code does not rot, if you do nothing to it it does not change at all. if anything, code work hardens - you introduce entropy as you work it, and you can remove this entropy by spending a lot of energy to refactor it (similar to recasting/forging)
    – jk.
    Feb 18, 2012 at 10:07
33

Refactor the areas of code you need to modify most often. Since you visit these areas frequently, refactoring will improve your understanding of the code and readability for when you have to visit them again, whereas there is no point to refactor code no one is ever going to look at again.

Moreover, if you only refactor areas of code when you need to modify them, it gives you a valid excuse if your refactoring causes a regression. Of course, try to cover your refactorings with unit tests, but this is not always possible, especially with legacy code and you should expect big refactorings to create regressions every once in a while.

If you need to add features and your design is slowing you down or causing duplication then refactor. When I design code I almost never predict exactly what changes will be needed in the future. However, when I add new features, I usually end up refactoring the shared code. By the time, I've added a third feature/refactoring cycle, my refactoring is already paying for itself and my shared code is fairly stable.

TLDR: refactor as needed, there is no obligation.

3
  • could you be more specific what you mean by 'refactoring causes a regression'? Is the purpose of refactoring to improve the design of your software without changing its behavior? Can you give an example?
    – Manfred
    Feb 18, 2012 at 0:34
  • 3
    @John: Yes, that is the purpose of refactoring: improve the code but maintain the behavior.
    – Bernard
    Feb 18, 2012 at 3:23
  • @John any non-trivial refactoring risks unintentionally changing behavior, especially when there are no regression tests in place. Feb 18, 2012 at 16:14
15

Everything you do has a cost. You have limited time to program. Everything you do in programming should be looked at against, "What's the benefit of doing this?" and "What's the benefit of doing something else?"

If you don't take into account the opportunity cost (What's the cost of doing something else?) then quality is free. It's better to improve your code. You'll learn something. It'll run better. It'll sell more.

The real question is what's the next best thing you could be doing with your time? And then you have make trade-offs.

Will it sell more of the software?

Will it cause less headaches to support?

What will you learn?

Will it become more aesthetically pleasing?

Will it improve your reputation?

Ask these questions (and any others relevant to you) for this and other activities in your queue, and that'll help answer if it's worth fixing. These trade-offs are why economics is important for programmers. Joel said it before I did, and an old mentor said it to me even before Joel.

Or if you want a simpler answer, just stop watching TV until you've fixed the code. :-)

3
  • 4
    To borrow from a real example not in software, some transport companies will pay employees to clean and polish their rigs, for a variety of reasons, often because it helps the drivers/operators build pride in 'their' unit, so they take better care of it, and pay closer attention to it; averting problems quickly and conveniently. On the same token, if there are miles to make, get in the truck and drive it and worry about cleaning it after a few more thousand miles (going coast to coast).
    – JustinC
    Feb 18, 2012 at 9:35
  • @justinC - exactly! Your example captures both another reason, and opportunity cost.
    – MathAttack
    Feb 18, 2012 at 11:53
  • Cost (or value, etc)is indeed the keyword for the right answer. It should add value in the whole sum, also considering that touching old code might break stuff (remove value) no matter how much more optimized it might look like.
    – cregox
    Mar 16, 2015 at 18:34
6

I think you should start by asking yourself the question that seems to have been missed. In what way is the code bad? And by this you are really asking yourself the following:

  • Does the code not do what it is supposed to do?
  • Is the code causing you problems when you are maintaining the software?
  • Is the code completely self-contained?
  • Do you have plans to update or replace the code at any time in the foreseeable future?
  • Is there a sound business case that you can make to update the code that has you concerned?

If there is one thing I've learned over the years, it's that you can't afford to second guess yourself after the fact. Every time you solve a problem in code, you learn something and will probably see how your solution - or something similar - might have been a better solution to a problem you solved differently years ago. This is a good thing, because it shows you that you have learned from your experiences. The real question however is whether the code you wrote before is likely to become a legacy problem that gets more difficult to deal with over time.

What we are really talking about here is whether or not the code that bothers you is easy or difficult to maintain, and how costly that maintenance is likely to be as time passes. This is essentially a question about Technical Debt, and whether it is worth paying off that debt using a little surgical maintenance now, or whether the debt is small enough that you can let it slide for a little while.

These are questions that you really need to weigh up against your present and future work load, and should be discussed at length with your management and team in order to get a better understanding about where to invest your resources, and whether your old code is worth the time you may need to spend on it - if at all. If you can make a good business case for introducing change, then by all means do so, but if you are facing the urge to tinker with code because you feel embarrassed about the way in which you wrote it, then I suggest that there is no room for personal pride when it comes to maintaining old code. It either works, or it doesn't, and it is either easy to maintain, or costly to maintain, and it is never good or bad simply because today's experience was not available to you yesterday.

5

Definitely don't improve it just for the sake of improving it. As others have said (and is common sense), we all get better (for some value of "better") as time goes on. That being said, reviewing code (either formally or informally) often illuminates these bits and pieces that we probably wish never saw the light of day again.

Although I don't believe we have a responsibility to go back and improve code that isn't fundamentally broken, insecure, or depends upon features on a deprecated/EOL list, here are some things I've done in the past in this situation:

  • Identify people on the team who really dig going back and improving code, as a sort of brain cleanse or nice, bite-sized task that gives a sense of accomplishment, and find time in the schedule for them to do it on a regular basis. I've always had at least one of these sorts of folks on a team, and this approach can also boost morale (or at least the mood) if we're all in a lull or down period.

  • Use it as the basis for a learning exercise. For interns, students, or new/junior members of the team, refactoring old code with guidance from senior members of the team gives them the opportunity to communicate with new team members, understand more about the system, and get in the habit of writing/updating unit tests and working with continuous integration. It is important to note that this exercise is done with guidance, and clearly framed as an exercise to make the code better because it does not conform to current standards/norms.

So, no responsibility to fix it, but that old stuff can be really useful. And unit tests and CI are your friends!

6
  • 4
    giving bad code to a newbie is the worst anti-pattern of project management there is, they see it and think it is correct and just duplicate it, bad code spreads like a cancer because of bad advice like this.
    – user7519
    Feb 17, 2012 at 22:22
  • 1
    @JarrodRoberson Thanks for pointing out what I should've made more obvious; I've edited my answer to note that I do this very specifically as part of a learning experience, and they're working with a mentor. It's not a throw-them-in-the-deep-end situation.
    – jcmeloni
    Feb 17, 2012 at 22:24
  • I still think it is worded in a way that people will read "Give it to the newbie -- interns and students, especially." and stop reading for comprehension right there. If it was worded where it started out with a senior member and a junior member pair programing XP style or something like that it might not be so bad. I have problems with the gold plating aspects and added risk of changing things for the sake of changing them of the first bullet point as well.
    – user7519
    Feb 17, 2012 at 22:30
  • 1
    Code reviews are for preventing bad code from entering version control, not for identifying poor code after the fact, it is way to late then.
    – user7519
    Feb 17, 2012 at 22:34
  • @JarrodRoberson Thank you for pointing out the issues you had with my vague and unfortunately potentially misleading language; I have made more edits and stand by what's there now.
    – jcmeloni
    Feb 17, 2012 at 22:39
2

Not necessarily. However if you are performing code maintenance and happen to stumble upon something that could be better, you could change it provided you have the appropriate unit tests to make sure you didn't create a regression.

If no such tests exist and you can't be sure it will behave exactly how it should, you are better off leaving it alone.

1
  • 4
    I would add "and if changing it won't add a lot of time to the planned schedule"
    – HLGEM
    Feb 17, 2012 at 21:36
2

From a engineer's perspective, sure I would very much like to work on old code until it can't be improve any more. But is there a business case for it? At the end of the day, the code is there to contribute to the bottom line, the profitability of your workplace. If it is not, just leave it.

There is a big caveat, if by improving the code, you are going to avoid a bug from occurring, (thinking the big Y2K balls up here..) I would definitely bring it up with someone higher up, maybe your business unit manager, and discuss the consequences of improving the code.

2

As for me, the question can be rephrased and generalized: "Why should somebody spend additional resource (time, money, mental, etc.) if that concrete piece of code works fine now? Isn't it a waste of resource(s)?"

Every time I find a legacy code, I think about several things that seem to be important.

  1. What for I'm going to make changes?
  2. Will I need to support this code? How soon can it happen (near/far future)?
  3. Is this code comfortable enough to work with? (Right now)
  4. Can I be sure that all the changes which I do are safe? Different tests usually help to answer this question.
  5. What consequences can I meet if I will make mistake(s) during code modification? Is it possible to revert my changes quickly? Will it be a waste of time?
  6. ...

Actually, you should ask yourself a lot of questions. There is no a full and final list of them. Only you and probably your teammates can find the answer.

P.S. I have noticed that I tend to rewrite the code very often, whereas my friend and teammate tend to leave it untouched. That's because we answer the mentioned questions differently. And I have to say that we answer them wrong very often... because we can't predict the future.

2

Does Microsoft need to update windows? Do all the *nix Distros have to keep evolving? Or a more practical example does everyone still drive 1970's cars?

1

You can normally write code better the second time round, you learned more about the domain by writing it initially.

There isn't a simple answer for wether it's worth refracoring. Generally you shouldn't unless a) it's a good learning excerise for you or b) you would save time in the long run with better code. Remember every change risks bugs.

As a side note I would strongly advocate unit tests. It's very easy to muck up refracoring especially if you don't have current behavior clearly marked down with automated rests.

1

I believe that we have a responsibility to write the best possible code that we can today. That means using everything we have learned in the past and refusing to take short cuts in our design. If schedule pressures cause something to be cut, I don't believe the quality of our software should even be considered, that neat little animated paper clip, on the other hand...

Now if you are working and find that code you need to interact with is not written up to the level you are currently capable of writing, then it is reasonable to fix it IF you are able to do it in a controlled methodical manner. I personally have very minimal experience with this kind of Refactoring, like everyone (nearly everyone?) I tried the whole "lets just change everything and pray it works" and got bite by that bad enough. I'd suggest looking into Martin Fowler's discussions (either his book or one of many articles) on the matter. He seems to have inspired most of the current thought on the process.

Of course sometimes you may not be able to clean the code as you'd like. Joel Spolsky wrote an article on why you may want to devote a few weeks to simply Refactor your code. Read it here and yes it is a little old, but I think the information is still relevant.

I hope that helps

1

I feel that refactoring / improving old code defines the programmer himself. Wanting to improve code that you wrote a year ago only shows your progress, and if it makes the application better and you have the ability, time, and approval to improve it, do it.

1

Better/improved is a multi-axis comparison. Do you think you can make it faster, smaller, more resource efficient, more readable, more useful information, more precise results, more flexible, more general, able to run on more systems, eliminate a dependency on a separate product?

Why should your company pay you to spend time rewriting this code, instead of writing new code or rewriting some other piece of code?

You should make improvements as opportunity presents itself, but opportunity means that you are either already working on the code, or you've indentified a business reason to make the change.

Pushing a change to production introduces a non-zero chance of breaking things (unit and functional testing only reduce this chance, they do not eliminate it), and should only be done when the expected benefit outweighs the risk.

Which is another point to consider -- do you INTEND to push this change to production, or simply to the development branch? The bar for the two scenarios are completely different. If it is only going in the development branch, and may never get into production, then opportunity basically means you're looking at the code for some reason, and it's not time consuming to do. It can be reviewed as required if a push should ever happen, and left out if it is considered unwarranted at that time. If on the other hand, it's going to production now, as I said above, you need to consider if this change is worth the cost: in terms of time spent making the push, and the benefits of using the new code.

5
  • Changes of production code that are not intended to be pushed into production? But are kept in the development branch? hmm. Don't think I would accept that. It will only serve to confound and compound later development because no-one will be sure whether it should be changed (again) or not to comply with other changes that are intended for production. Feb 18, 2012 at 10:40
  • @MarjanVenema: when development stopped, all branches should be identical. If you later see something that can be improved, but doesn't HAVE to be improved, improve it in the development branch, and leave it there waiting for development to resume and a new push to be made.
    – jmoreno
    Feb 18, 2012 at 17:52
  • Ah, ok, I see. To me that means it is still intended for production, just later. And it is quite a risk unless you make sure that there is an accompanying issue in your issue tracking system so the testers/qa department also get their stab at your "improvements". Otherwise they will make it into production untested as it rides along with other changes. Feb 18, 2012 at 18:21
  • @MarjanVenema: perhaps I should have said intend to push it to production immediately. If you are positive that the change would never get pushed to prod, then don't make the change. If OTOH, you are uncertain, but the change is easily done and you're already there...make the change. As for tracking and testing, that was meant to be covered by "reviewed as required".
    – jmoreno
    Feb 19, 2012 at 1:11
  • hmm, I guess I would opt not to make the change unless it is directly related to what I am working on. And, as we have a branche per issue, I always know when (which release) something will go into production. Also, I really feel any change made needs to be tested, not "just" reviewed as required. People's assessments of what constitutes a risk differ and a second pair of eyes never goes astray. Then again, I work mostly on server side code where changes may subtly affect the results returned to clients. Risk assesment for changes to client side code possibly are a lot more straight forward. Feb 19, 2012 at 9:42
1

One good rule of thumb is that if it took you time to understand what the code is doing, and if this time could be reduced going forward by some well-chosen refactorings, then you are serving the needs of the business and of your team by taking these steps.

If the code does not benefit from your analysis, if the field names have not become expressive of the code intent and the methods have not been broken down into readable chunks, then the business has lost something of value: your state of comprehension. With tools like Visual Studio and ReSharper, these kinds of refactoringss are safe, logic neutral, and of potentially great value in future developer productivity and confidence.

As Uncle Bob Martin says, "Always leave the campground cleaner than you found it."

1

This is a question to ask the management in your company.

Do you have the time and resources necessary to go back and make changes to old code?

Do you have the time and resources to have a QA team TEST what you've changed to make sure it's not broken?

I've fallen into this trap before where I would be in a module fixing a bug, and see code I or someone else wrote that was inefficient or inelegant, change it, and end up with more problems to deal with than if I just fixed the bug I was tasked to fix.

0

It depends.

If the code in question is truly broken, such that it contains bugs that will inevitably surface at one point (e.g., some counter that will at some point overflow and cause nasty problems), then fixing those may be worth the effort - but if you do, put all possible safeguards in place first to avoid introducing new bugs: make sure you have meaningful unit tests covering everything you touch, have all your changes reviewed by someone competent, insist on testing thoroughly, make sure you can revert back to the old version at any time, back up any data before going into production, deploy to an acceptance environment first and have it tested by actual users, etc. You'll also want to get approval before going ahead: if the bug is truly a time bomb, and your manager is somewhat competent, you will be able to convince him/her to let you fix it (and if you don't get approval, then maybe that's a sign that you're wrong).

OTOH, if your complaint is mere lack of code elegance, then don't you dare touch it. It's been doing faithful service in production for quite a while now, changing the code would only satisfy you, but not add any value, and like every change, there is a risk that you break something. Even if you don't, your time is valuable (literally: you're getting paid for what you do, so every minute of your time costs money), and since you're not adding any actual value, such a change would be a net loss for the company and the project.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.