I have this problem. I can't stop myself from refactoring existing code that works but is, in my opinion (and perhaps objectively), badly designed or contains other "code smells". This can have a significant negative effect on my immediate productivity. But ultimately will be a big maintenance boon.
If you also suffer from this "affliction", how do you restrain yourself? Or at least manage the refactoring to avoid having to alter large chunks of existing code in order to make it maintainable for the long term.
I have trouble stomaching smelly code, but will usually put off cleanup and refactoring until I have to actually make changes, or if I have some down-time.
Until then, I'll just note ideas for changes in comments.
What worked for me was getting fired from a job for it. :\
That said, my basic problem was twofold:
I was refactoring code that no one had asked me to work on.
I found it too hard to put tests in place, so I did it without them. Which, naturally, broke some things.
What I learned was:
Don't work on code you don't need to work on, and that no one has asked you to work on. It's easy to remember this now, given the consequences in the past.
When you need to refactor code you are supposed to work on, have tests in place, but they don't have to be automated.
After reading too much TDD stuff, I tended to think of automated tests as the only kind of test there is. In reality, even a bunch of
Debug.Print
statements can be a decent test to figure out if functionality is staying consistent.If you have to refactor, and you can't do automated tests, you must do some kind of test, whether it's printing text, or a UI script, or whatever. Any effective test is better than no test at all.
The highest-rated answer urging you to go ahead and refactor is good for many cases, but somewhat simplistic too. (I'd probably comment on it, but have no privileges to do so - I hope this works stand-alone too.)
If you work with a large (legacy) system that's been in development for years and years, there are always too many things to refactor at once (unless you have been exceptionally rigorous all those years, which I don't believe :). So, you simply cannot get on all the tangentials you'd like to; that's a fact of life you have to accept. Otherwise you'd always be speding days on end cleaning everything up, when the original change (bugfix or enhancement) could have been done in much less time (tests and some refactoring included!).
So, usually you have to draw a line somewhere; refactor only code that directly concerns the task at hand, and only if it will not take a disproportional amount of time.
As for the bigger overhauls of architecture - which certainly you can't avoid when dealing with aforementioned large codebases. You'll have to select the ones deemed most critical, and task and prioritize them in your process high enough that they will really get done, even when these changes add no external value themselves (i.e. only immediate value for the developers, by making the code more manageable). (Now, if this would not be possible - if decision-makers are not smart enough to see that it's necessary to use time on such improvements, well, then your codebase is simply doomed in the long term. :))
If you are free of any constraints of commercial software development, your mileage may vary. ;)
By the way, good question - I too find myself thinking about where to draw the line quite often.
First, you have to realise and accept that the only reason that the bad code matters is if it needs to be read and/or modified. It only needs to be read and/or modified if you're looking for a bug or adding functionality. So only touch the code if you're bug-fixing or enhancing. So, if you come across some massively nested if-then-else statements, or a badly named variable or method, leave it alone unless you need to actually understand or change that code to finish the task you're actually working on. If you do have to change the code then refactor it enough to make the code comprehensible and the change easier to make, but no more.
It's easy really.
Everything we modify, we have to come up with a written test plan that is understood and reviewed by a QA engineer. Not modifying stuff drastically simplifies actually getting code released. Therefore, unless you're touching that piece of code for some other reason anyway, any kind of refactoring is pretty much a no-no.
EDIT: Even if your company does not have such policy, just remember that you need to test what you change. Estimate the amount of time you will need to test your changes.
Answering the question "What is the added value?" also helps.
I just wanted to add that if the project in question is supported by unit-tests then refactoring is going to have considerably less risk attached, and would have a degree less productivity cost (because you're going to know exactly when you're ging wrong).
There's still a cost/benefit to be done, but in principle I'd agree with Longhorn that you shouldn't be stopping yourself.