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.
Outside of coding, Wikipedia espouses a <A HREF="http://en.wikipedia.org/wiki/WP:ENGVAR">similar philosophy</A> regarding the use of different flavours of English for reasons similar to your second one: preventing the article from flip-flopping (edit warring) every other edit:
ReplyDelete<BLOCKQUOTE>The English Wikipedia has no general preference for a major national variety of the language; none is more “correct” than the others, and users are asked to take into account that the differences between the varieties are superficial. Cultural clashes over spelling and grammar are avoided by using four simple guidelines.
Consistency within articles: Each article consistently uses the same variety of English throughout; for example, center and centre are not used in the same article.
...
Strong national ties to a topic [e.g., <I>The Lord of the Rings</I>]
Retaining the existing variety: If an article has evolved using predominantly one variety, the whole article should conform to that variety.... In the early stages of writing an article, the variety chosen by the first major contributor to the article should be used....</blockquote>
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.
ReplyDeleteIn 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.
@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.
ReplyDeleteYou 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.