Don't Change Code Unless It's Broken

This came up recently on my team.  When making changes to pre-existing code, it is tempting to bring the code in line with our personal tastes.  If we like Allman style braces, we'll change the code away from K&R bracing.  If we don't like Hungarian, we will be tempted to change the variable names by removing the annotations.  There are many more styles I could mention here.  When faced with the temptation to change this code, resist.  Unless you are working by a coding standard that clearly calls out your style, don't change things.  Even if your coding conventions do mention a style, consider ignoring the infraction.  There are two reasons for this.

First, every change in the code is a chance to break something.  The code you are working with is known to be working.  Well, mostly.  There is some reason you are in there and it might be to fix a bug.  Even so, most of the code is known to be working.  If you change it--even for something small like bracing styles--you risk breaking something.  Making a change that subtly changes the code flow.  Unit tests will help reduce the risk here but unit tests don't cover everything.  Remember, we write code to deliver features to end users.  Why risk introducing a bug unless you are adding something of value to that end user?

Second, it is just a bad policy to change the code.  It makes code reviewing harder because it's hard to see the real changes from the purely cosmetic ones.  Even if you check in the changes in a different checkin, someone trying to following the history of the code will still come across your changes.  Worse, what happens when the next person to touch the code prefers K&R bracing and changes it back?  In theory, the code could flip-flop every other checkin.

The moral of the story:  Don't make cosmetic code changes.  Change code only because a) it is broken or b) changing it allows you better implement a new feature.

Comments

• Anonymous
  July 09, 2007
  The comment has been removed

• Anonymous
  July 10, 2007
  Whether or not to change code because of style problems is a business decision that depends mainly on a cost-benefit analysis.  While I agree with that in the vast majority of cases, the result of that analysis is "no," I believe that you should consider the merits of each situation rather than institute a hard-and-fast rule staying to never make such changes.  These types of rules needlessly short-circuit a decision-making process. In addition, the argument you make about potentially introducing bugs isn't logical.  Any code changes require testing, including cosmetic code changes, and testing these types of changes is just as effective as testing other kinds of changes.  You need to have unit tests in place for the code anyway, and it should be convenient to run them, which of course negates the risk of introducing problems by making cosmetic changes. That said, I typically don't spend time cleaning up code unless I am getting ready to make some "real" changes to it.  But I usually do perform some clean-up when necessary, and I do this as part of the "learning how the code currently works" part of the development.  Questions that come up as I am learning about the code lead to cosmetic changes to the code to answer those questions, e.g., adding/editing comments, improving variable or function names, fixing intenting, etc.  This lets me get my feet wet with some unfamiliar code, and lets me warm up with some easy, low-risk changes before I get into the "real" work. One final point, we should all have in place tools that auto-reformat source code based on department or corporate coding standards.  These tools should be run prior to code check-in to "normalize" the code in an automated way, before it is tested.  This would at least eliminate a portion of the work.

• Anonymous
  July 15, 2007
  @Tom, I left open the possibility of conforming to a coding standard which is, I think, bascially what you are talking about as a business decision.   You are correct that unit tests are necessary if you are doing this, but I think you overstate their value.  They don't negate the risk of introducing problems, they only reduce them.  There is still risk.  Unit tests don't cover everything. You make a good point about learning the code.  I still think you could do so better by stepping through it in a debugger than by slightly modifying the code.  The big point, however, is not to change things to your preferences.  Travis makes a great reference to Wikipedia to show that this is not just a coding issue.