This is very interesting. Seems like there's a critical mass (Checkstyle,
FindBugs, PMD) of code improvement data that we could act on.
Maybe the next release cycle should be devoted to code improvement and
driving these issues to zero?
Cheers,
Philip
p.s. Unlike Aaron, tab characters drive me crazy. I don't think it would
be that hard to get rid of them---just open the offending file in Eclipse
and reformat it (with the 'insert tabs for spaces' set off.) Before we go
there, I'd want us to converge on a standard for Eclipse's formatter
settings. For example, no aligned Javadoc comments!
--On Sunday, February 12, 2006 2:18 PM -1000 Aaron Kagawa
<[EMAIL PROTECTED]> wrote:
Hey Guys,
I've been testing out the new Checkstyle 4.1 release with Hackystat. Here
are some interesting findings. Note that all this was done locally. I
didn't commit any thing.
1) The new checkstyle version found MANY errors that the old version we
were using didn't find.. Note that I had to turn failOnViolations off to
be able to see all the errors at once without having to fix them. I also
ran ant -q all.checkstyle on all of the modules in Subversion.
Here is just a sample of the roughly 100 errors that were found.
c:\java\svn\hackyCore_Build>ant -q all.checkstyle
[checkstyle]
C:\java\svn\hackyCore_Common\src\org\hackystat\core\common\selector
\combo\ComboSelector.java:40:42: Expected @param tag for 'user'.
[checkstyle]
C:\java\svn\hackyCore_Common\src\org\hackystat\core\common\selector
\combo\ComboSelector.java:40:53: Expected @param tag for 'page'.
...
[checkstyle]
C:\java\svn\hackySdt_ReviewIssue\src\org\hackystat\sdt\reviewissue\
dailyanalysis\ReviewIssueDailyAnalysis.java:110: Expected an @return tag.
...
[checkstyle]
C:\java\svn\hackyCore_Installer\src\org\hackystat\core\installer\vi
ew\swing\main\common\CommonPropertiesButtonPanel.java:73:44: Expected
@param tag for 'e'.
[checkstyle]
C:\java\svn\hackyCore_Installer\src\org\hackystat\core\installer\vi
ew\swing\main\proxy\ProxyPropertiesButtonPanel.java:73:43: Expected
@param tag f or 'e'.
....
[checkstyle]
C:\java\svn\hackyApp_Cgqm\src\org\hackystat\app\cgqm\plugin\issue\I
ssueResult.java:68:36: Expected @param tag for 'user'.
[checkstyle]
C:\java\svn\hackyApp_Cgqm\src\org\hackystat\app\cgqm\plugin\issue\I
ssueResult.java:68:49: Expected @throws tag for 'CgqmException'.
...
[checkstyle]
C:\java\svn\hackyApp_Review\src\org\hackystat\app\review\analysis\c
ache\ReviewActivityUserCache.java:103:38: Expected @param tag for 'user'.
[checkstyle]
C:\java\svn\hackyApp_Review\src\org\hackystat\app\review\analysis\c
ache\ReviewActivityUserCache.java:103:48: Expected @param tag for
'workspaceRoot Set'.
...
[checkstyle]
C:\java\svn\hackySensor_Bcml\src\org\hackystat\sensor\bcml\BcmlSens
or.java:103:32: Expected @throws tag for 'BuildException'.
[checkstyle]
C:\java\svn\hackySensor_Jupiter\src\org\hackystat\sensor\jupiter\Ju
piterSensor.java:280:63: Expected @param tag for 'event'.
[checkstyle]
C:\java\svn\hackySensor_Jupiter\src\org\hackystat\sensor\jupiter\Ju
piterSensor.java:301:43: Expected @param tag for 'event'.
[checkstyle]
C:\java\svn\hackySensor_Sclc\src\org\hackystat\sensor\sclc\SclcSens
or.java:116:33: Expected @throws tag for 'BuildException'.
[checkstyle]
C:\java\svn\hackySensor_Locc\src\org\hackystat\sensor\locc\LoccSens
or.java:108:32: Expected @throws tag for 'BuildException'.
[echo] (13:33:35) Completed all.checkstyle
BUILD SUCCESSFUL
Total time: 2 minutes 3 seconds
Sending build result to Hackystat server... Done!
c:\java\svn\hackyCore_Build>
It is interesting to point out that the majority of the errors are in
hackyApp_Review, hackySdt_Review, and hackyApp_Cgqm
---------------------------------
2) I added a couple more Checks that we should be following.
<module name="LocalVariableName"/>
<module name="MemberName"/>
<module name="PackageName"/>
<module name="TabCharacter"/>
<module name="LeftCurly">
<property name="maxLineLength " value="100"/>
</module>
<module name="NeedBraces"/>
<module name="RightCurly">
<property name="option " value="alone"/>
</module>
This generated about 500-600 more errors. Many of which were either
TabCharacter or RightCurly violations. I suppose the TabCharacter issue
isn't as bad as the RightCurly violations. In addition, there must be
around 40-50 violations of naming conventions (ie. instance variables or
local variables with capital names), for example:
[checkstyle]
C:\java\svn\hackyCore_Kernel\src\org\hackystat\core\kernel\sdt\SdtM
anager.java:117:14: Name 'EclipseSensorPluginObject' must match pattern
'^[a-z][ a-zA-Z0-9]*$'.
---------------------------------
Ok.. What should we do? I suggest that we upgrade to Checkstyle 4.1 and
fix the errors that the old version wasn't finding. Then I suggest we
add at least the following checks and fix those.
<module name="LocalVariableName"/>
<module name="MemberName"/>
<module name="PackageName"/>
<module name="LeftCurly">
<property name="maxLineLength " value="100"/>
</module>
<module name="NeedBraces"/>
Once we eliminate all those errors then we can start to think about
adding the TabCharacter and RightCurly checks. But that is going to be a
big task to remove all those.
<module name="TabCharacter"/>
<module name="RightCurly">
<property name="option " value="alone"/>
</module>
thanks, aaaron
