Saturday, 7 May 2011

In which I complain that coding standards are a waste of time

Just kidding. I'm not going to rail against coding standards. No, I'm going to rail against the absolute and dictatorial enforcement of coding style guidelines, to the point where a single misplaced curly bracket can fail the code review of a thousand line project before it's even started. I'm going to rail against the fact that many people these days don't actually know that coding style guidelines and coding standards are not the same thing.

Coding style guidelines are not coding standards. Coding standards are rules on how to construct software. Rules that have practical and defined benefits. Rules than can be proven to assist in the structured and correct building of code. For example:
  • Do not perform database access in the constructor.
  • All database access should use the framework's DAL.
  • Do not rely on complex instantiation instructions; provide a factory method.
  • Do not inherit from a class in a different module or library; only encapsulate.
  • Avoid singletons.
  • Don't maintain state in utility classes.
Of course the specifics change with each project, with each team, with each individual. But the point is that the coding standards define how code is built, and in no way does it define how much whitespace to use.

Another assertion is that automated coding style checking is a static analysis method. Incorrect. It is a way to count whitespace and make sure your curly braces are not out of place. It doesn't check types for logical consistency. It doesn't check variable access for potential casting exceptions. It doesn't ensure that all types are locatable by the class loader. Static analysis tools and lint checkers check far more important things than parenthesis placement.

Ironically, many of the languages where over-zealous application of coding style guidelines is rife are in the dynamic language communities. PHP, Ruby, etc. I say ironically, because they're dynamic languages and cannot, by definition, have static analysis performed on them. That's why they rely so heavily on unit tests.

In these communities, it seems that a meme has taken hold without true understanding. "We need consistency and automation!" they cry, and so they try it. But they put it in all the wrong places. Your code should stand or fall on whether it is logical, correct and understandable. How you choose to indent is your own business.

Coding style guidelines are the lowest form of code analysis. Code reviews in disfunctional teams often degrade to picking spots about coding style.
  • Put curly brackets on a new line.
  • Put curly brackets on the same line.
  • Indent multi-line statements to the first parenthesis.
  • Indent multi-line statements one tab stop.
  • Put a space between if and (
  • Do not put a space between if and (
So much nit-picking, so little actual, useful insight. A few people got together and had a thought:
"If we're spending all our review time picking little, insignificant faults in each other's code rather than checking that it's actually any good, then maybe we should change things?"
There were a few possibilities, but the solution they chose was "automate the nit-picking of insignificant whitespace differences." This was the wrong solution. The correct solution would have been
"Why are we nit-picking about whitespace anyway? Are we really that petty? Let's start reviewing based on actual coding standards and unit test results."
Failing code for not meeting coding style guidelines precisely is a waste of time. It's arbitrary. It's subjective. There is absolutely no evidence that enforcing coding style in this way has any positive effect on code quality, readability or coherence. However, failing a code review for petty reasons does have negative knock-on effects.


Judging code quality by amount of whitespace
is like judging programmer productivity
by lines of code written.


The compiler doesn't care. The user doesn't care. In all practical ways, the coding style guidelines should remain just that; guidelines. The most egregious flaunting of the rules should be flagged in code review. If somebody completely fails to maintain sane indentation, or writes tightly packed code that's hard to decipher, then by all means throw it back. But if the only problem is that somebody's editor indents the last line of a multi-line array in a slightly different way than your standards dictate, lighten up and realise that it doesn't matter at all.

Can anyone out there, in all honestly, compare this:

    doSomething(someValue,
                someOtherValue);

with this:

    doSomething(
        someValue,
        someOtherValue
    );

and categorically state that one is harder to read than the other? No, because it's subjective. It's all subjective, and it's all arbitrary. Making code functionally and logically correct are objective standards and are provable. Making code readable is entirely subjective and cannot be dictated by failing reviews over hard and fast rules about indenting. 

If anyone out there can explain why coding style guidelines have been elevated to this pinnacle, this pedestal, this lofty height to which we mere mortals can only dream to reach, then please enlighten me. As far as I can tell, there is no reason at all beyond cargo cult programming, rhetoric and misunderstanding.

End of rant. Just needed to get that off my chest.

2 comments:

Ben said...

I think it also depends on team size, as you will always get someone who wants to write the whole function on one line and moan about not having the freedom to express himself!!! If there is room for debate then you will always have an argument.. If the style is clearly defined, there is no debate, it is a given!!

Also, I've seen people want to use tabs and other people use spaces.. Sometimes you can then open a file that has been edited by two different people and depending on your own editor the code is all over the place depending on tab_indent size. Then what do you do? Spend you time trying to format it, or just live with it? If you spend your time formatting it, isn't that a waste of your own time?

Also, I really think picking on the white space aspect is moot, as most editors can clearly define that for you, so you do not have to worry about it. Other aspects of the code sniffer, for example picking up doc block mistakes is beneficial, as other tools rely on tags, such as DocBlox and Sonar to name just a couple.

I can see both sides of the argument ;-)

Doozr said...

As I said, blatant disregard for agreed guidelines should fail code review, although arguably if you have a developer on the team who insists on writing his functions on one line and won't even meet halfway, there are bigger issues than coding style. 

My complaint is really more to do with the broad brush application of these guidelines as if they are gospel. Failing a code review because the developer has written a function all on one line then it's a world apart from what essentially boils down to a minor difference in editor configuration. A little common sense and a less absolutist attitude goes a long way to helping the team dynamic. 

IMHO, as long as code is indented to suitably separate logical blocks, then I don't care how any individual choose to indent their multi-line ifs, calls and arrays, or whether they prefer dangling curly brackets. As long as I can read it easily, it makes sense, and is not ambiguous, then I'm happy.

As far as automated tools go, they irritate me because it smacks of a wrong attitude on the part of the reviewer. Throwing code away as useless because of minor style choices without even reading it is the wrong attitude to take.

There is no need for a "nit-picking" phase in a code review at all. Just read the code as part of the normal review. If any part of it jumps out as being badly layed out, hard to decipher, logically ambiguous or obviously in breach of guidelines, then by all means flag it. Otherwise, live and let live. 

Post a Comment