Mailing-List: contact dev-help@myfaces.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "MyFaces Development" <dev@myfaces.apache.org>
Received-SPF: neutral (athena.apache.org: local policy)
Subject: code style checks: how much is enough?
From: simon <skitching@apache.org>
To: MyFaces Development <dev@myfaces.apache.org>
Content-Type: text/plain
Date: Tue, 08 Jul 2008 00:13:29 +0200
Message-Id: <1215468809.6261.44.camel@simon-laptop>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

Hi All,

I've been experimenting with CheckStyle configuration setups, as you may
have seen. There are now three different check files available,
"normal", "loose" and "strict", and it is fairly simple to specify which
rules are used for each project.

The myfaces-build-tools project was the area I used for trying to get a
reasonable balance of effort to reward. The projects there now all pass
the "normal" checks, except for two which are just too different and so
are set up to only use the "loose" check rules. The orchestra projects
all pass the "strict" test (this is really why I have been trying to
sort out checkstyle...)

I've verified that it is possible (and quite performant) to have
checkstyle check rules at *compile time*, ie when "mvn compile" runs.

I think that we should certainly enable compile-time checks for the
license header, ie ensure that "mvn compile" fails if the license header
isn't present on all files.

But do people care enough about code style to enable the rest of the
checks at compile time (ie prevent people from adding code that violates
the rules)? We have an agreed code style, but it is not being followed
in a lot of places. *If* people think that consistent code style is
important enough, we can also enable these checks at compile time; the
performance hit is minimal. But obviously the existing code would need
to be cleaned first.

I'm willing to clean up the existing codebase for checks that people
feel are worth enabling at compile-time, but not for those which only
appear on a report as developers clearly don't look at the reports even
at release time (see the number of bugs listed in the findbugs report!).


Here's some info on existing projects:

shared/trunk/core: 1083 errors including:
* 0 files with invalid license headers
* 166 lines longer than 200 chars
* 166 if-statements without {} around the body
* 637 uses of sun brace style
* 13 inner assignments. This means code like:
    if ((retval = _cacheL1.get(key)) != null)
  This particular example is not dangerous, but these can be.
* 1 switch statement with no default clause


shared/trunk12/core: 882 errors including:
* 8 files with invalid license headers
* 187 lines longer than 200 chars
* 177 if statements without {} around the body
* 385 uses of sun brace style
* 13 inner assignments
* 3 switch statements with no default clause


So, is code style important enough to check, or not? 

Please express your opinion on each of these:
[ ] Check license headers are correct as part of "mvn compile"
[ ] Check for tabs in files as part of "mvn compile"
[ ] Check for dangerous code as part of "mvn compile"
    (eg assignment-in-condition, equals overridden but not hashcode)
[ ] Check general code style (braces, varnames, unused imports etc) 


Regards,
Simon