Refactoring: Do you follow The Boy Scout Rule?In the book Clean Code: A Handbook of Agile Software Craftsmanship [1] by Robert C Martin he advises that you should follow The Boy Scout Rule when maintaining code. The idea is simply to leave the code a little cleaner than you found it. I.e. it is very similar to the tip about not leaving broken windows from the Pragmatic Programmer [2] book.
I've found that I tend to do this more and more often, but I also see many fellow developers who subscribe to the "if it's not broken, you shouldn't mess with it" idea.
I'm aware that this may be somewhat of a religious debate with no single answer, but I am curious to know what point of view other subscribe to as well the reasons for doing so.
In other words: Do you clean code as you pass it or only when you have to fix the code anyway?
EDIT Related question [3].
ACCEPTED]
It depends how much time I have and whether there are unit tests available.
It's always a bit risky to refactor bad code that you just stumble upon because you may inadvertently change its behavior. Unit tests help mitigate that.
If I'm working on the code (so it will be tested thoroughly anyway) I always try to improve it. My favorite thing to do is to improve the code, add features and also decrease its size (less code = less bugs).
I think it depends if i have to maintain and extend this code. I have a different view than most answers so far. Everyone keeps talking about how "risky" it is to start changing code that is foreign or unclean. You may change behavior, introduce bugs,etc .
My opinion is that if the code is in that state, you MUST refactor it because it actually risky to not refactor.
Being safe by not touching code that you dont understand is an illusion. If you ever want to have your application get better over time, you have to dig in, understand the behavior expected (write unit tests) and then get it to a point that are comfortable changing it.
. Its actually very dangerous to leave it alone as it will just get more legacy and people will work around it and put bandaids on it and it will never be maintainable at all.
I agree with the refactoring strategy of writing lots of unit tests but if any of my programmers says i am being safe, by avoiding code that they dont understand, i tell them they don't realize it but they are doing just to opposite.
Normally, I only touch "foreign" code when I really need to. It is far easier to break existing code than to beautify it.
Having said this, when I see some lines of code of which I know are a duplicate of a problem we solved earlier, I try to replace this code with a function call to the old solution.
Beauty is in the eye of the beholder. For every piece of code you "clean up" you are just inviting another programmer (maybe even the original) to come back and "clean up" after you.
This was a major problem with a project I worked on a year or so ago. A new "star" developer was hired and decided he didn't like the way I wrote my code. He decided to "clean it up" for us (even did it on his own time after most people had left for the day). After about 3 days of this we noticed that the performance of the application was suffering heavily. He wasn't aware that his changes were fairly far reaching an ultimately caused much more harm than good (he didn't break the build or the unit tests, so he thought he was fine). We had to restore 3 days of his work.
Cleaning code for the sake of cleaning code is generally a bad idea, IMHO.
My current assignment has me maintaining an fairly large web application(I wanted to say enterprise, but that would insult web application which are actually enterprise). Anyway, the code is not very well written and no unit tests are present all. Tests are performed by humans. When fixing bugs, we have been reduced to just fixing that small area where the bug is. Not very satisfying, but we have found that refactoring has a high risk of breaking other parts, the coupling is very tight, even though an attempt has been made to lously couple things. Refactoring would take a very long time, and there simply isn't the resources(money and competent people) present to allow us the time to refactor and keep the app running. Before this assignment I did try to live by the Boy Scout Rule, but here it is simple too risky and expensive. I have made the suggestion but the response is always the same: "No, just fix and move on".
In my opinion, in order to live by the Boy Scout rule, others have already mentioned this, unit test, unit test and careful planning.
Yes, if a complete suite of Unit tests are available. If tests are NOT available, then add tests and then beautify the code.
But, yes, I do follow this rule.
I tend to not refactor other people's code unless when it is either unreadable or simply done wrong. I honestly only do it so that the next person which will have to dive into that piece of code wont be as annoyed as I was while reading it.
IMO, code is meant to be easily understandable.
I worked with one incredible engineer a number of years ago. Each code submission was notable in that the state of the project at each commit was fully self-consistent, elegant, and correct. There were never warts, and every time he made a change, he mentally "rebalanced the tree", reimagining the whole project as a side-effect, and doing what was necessary to create that. Sounds inefficient, but he was brillant and fast, so it didn't matter, and the source code was never anything but flawlessly designed for what it did.
The best developers can and should leave code better than they find it; and yet like a ninja, their presence in existing files is nearly impossible to detect to the unassisted (svn blame) eye.
If you don't fully understand the existing code when you're working with it, you're not one of the best developers. I urge you to become one, but until you do, don't mess with stuff that (a) works and (b) does not make 100% total sense to you.
(Disclosure: I am in the latter category myself. But I'm fortunate enough to have worked with true ninjas.)
Yes, I do refactor code, when I pass by BUT if I'm not actively working on the code I only do safe refactorings. Safe refactorings are:
If I work actively on a piece of code I tend to refactor more aggressively because I know the code better and I make sure that I have enough test coverage.
I think, the Boy Scout Rule can be easily overdone. To use the analogy: one should pick up garbage when stubling upon: e.g. misnamed local variables, 3 liners that can be solved with standard library functions, typos in docs, etc. When necessary for the current Task, one should refactor.
But it is not necessary to renovate the camping ground just to set up your tend, so: refactorings 'for coolness' should be done as a dedicated task, and discussed with the team, to avoid back-and-forth refactorings and to keep the work flowing.
And, imho: the boy scout rule should not be enforced on others (as in: 'nice change, but why didnt you refactor the code you didnt write above your change?'), because that leads to an (unconsious) development style, where the location of the code is determined not by necessity, but by the least amount of justification one will have to provide.
Bottom line - if it ain't broke, don't fix it. As others have stated it is far too easy to introduce new bugs or changes in behavior because of your fixes, even with unit tests.
However, I also subscribe to the "broken windows" theory. The way to reconcile these viewpoints is to continually refactor, but do it as a planned activity, and let the refactoring be part of the overall design. If you're just in there to make a quick fix, don't meddle with it, because it will be harder and more subtle than you think. Your goal in such cases should be to get in and get out with the absolute minimal change possible.
Save the refactorings and beautifications for larger upgrades, when you can plan for them and have the time to really understand what you are doing.
I tend to refactor quite a bit so that's why large companies just don't work for me. Smaller companies tend to let you get away with that more.
Of course, make sure you test test test.
Rules are made for two purposes: To be followed, or to be broken.
Reading the answers so far, I can see good arguments for either option. Perhaps you can decide what to do by asking yourself: It is necessary to refactor? Is there really a clear, technical need to change the code? (It must be technical, estetics because 'it looks cleaner that way' definitely do not count).
Refactoring without needing to is premature optimisation.
It depends on the policy of the company you work for. Smaller firms can be more flexible about that sort of thing. The big company where I work would require a full regression test after any changes to an application. The tests are rarely automated, so a change is expensive.
Ad-hoc refactoring is discouraged. Unit tests cost developer time to write, so project managers will always ask that you not write them if given a choice between tests and deployable code. They add to the maintenance burden, because they have to be kept up. It's usually left up to the developer to write them or not. If it's a contract situation, they are not written or checked in unless the contract explicitly asks for them.
This is part of the reason why their portfolio is such a bloody mess, but it's a fact of life.
I think it's what you get when you measure successful performance by looking at budget and dates. Quality is the first thing to go out the window.
One more point: We all have to agree on what "better" means.
The worst thing you can do is start an oil-canning fight in your source control, where two developers reverse each other changes.
If you're working in a team, I'd say that refactorings ought to be discussed. This can be as simple as shouting into the next cube or IMing someone in a remote location to let them know.
The talk will help diffuse a common understanding of what "better" means. It'll also ensure that you aren't causing problems that could affect the current iteration. It'd be a bad thing if you refactored away some work that another person on your team had carefully considered and put in place for a reason you're ignorant of.
The Google query (( JOel "Things you should never do" )) brings up what I think is a nice essay on this. In practice, I find it difficult to imagine the web of ripples. If there is high cohesion and low coupling then most likely the code would not need cleaning up - most folks are good at optimizing isolated algorithms.
