Hello all,
As per Ted's suggestion, this thread is meant to discuss updating the
Struts CheckStyle rules file as brought up in Bugzilla ticket #35956.
My motivation for this suggestion is because I wanted to do what I could
to help towards the 1.3 release, and most of the true issues seem to
have been resolved or are being dealt with. Resolving as many of the
CheckStyle complaints as possible is something I can do, so I am giving
it a go.
I am a big believer in static code analysis and in releasing things that
have as close to a perfectly clean report as possible. I usually run
CheckStyle, PMD and JLint against my own stuff, but one thing at a time :)
I'm suggesting two things really... one is to begin using CheckStyle 3.5
(which would have to be added to iBiblio) and to add some additional
rules to the rules file.
3.5 adds some checks that could be nice to enable down the road and also
includes some bug fixes, which of course you always want to see in a
code analysis tools to avoid false alerts.
As for the new rules I'd like to enable...
* StrictDuplicateCode
Duplicate code is just plain bad... not in terms of something not
working, although that is certainly possible in some situations, but
it's just a sign of carelessness. If nothing else, I'm sure no one
wants the Struts code base to show any carelessness that could easily
be avoided.
* AbstractClassName
This one might not be one that can be activated because I can conceive
of it breaking the public interface if it turns up any classes not
properly named already... then again, it may very well find no such
problems, in which case turning it on is good going forward.
* ImportOrder
Imports in alphabetical order just makes it a little easier to find if
something is used. This is obviously not going to break anything, and
is easy to fix with virtually any IDE in existence today (or an
already-mapped macro in UltraEdit for us anti-IDE folk), so it's
low-hanging fruit, might as well grab it I figure :)
* CovariantEquals
The CheckStyle docs say it all on this: "Mistakenly defining a
covariant equals() method without overriding method
equals(java.lang.Object) can produce unexpected runtime behaviour."
* StringLiteralEquality
I'd be very willing to bet there are no such mistakes in Struts, doing
s == "test" when you meant s.equals("test") is indeed a novice
mistake. Still, better safe than sorry I figure.
* IllegalCatch
Again, as the docs say: "Catching java.lang.Exception, java.lang.Error
or java.lang.RuntimeException is almost never acceptable." In this
cases where it actually *IS* acceptable, simply ignoring the error is
acceptable.
* PackageDeclaration
This is almost a silly check frankly, but again, no harm no foul.
* ExplicitInitialization
It's kind of a code smell to initialize class members to their default
values. I've also heard it argued that it introduces slight
inefficiencies, but I'm not sure I believe that personally.
* UnnecessaryParentheses
I personally find code with parenthesis that aren't actually needed to
be more difficult, some feel the opposite. If they are truly not
needed though, it's just more characters for my brain to parse I
figure.
At the following URL...
http://www.omnytex.com/struts_checkstyle_reports.zip
...you can download three CheckStyle reports...
The first is with the current rules file, resulting in 4829 complaints
(many of which are relatively minor things, i.e., tabs instead of
spaces, javadoc problems, etc.)
The second is the updated version with all the above checks added...
this results in 16584 complaints, which is clearly a lot... however, the
vast majority of the additional complaints result from the
StrictDuplicateCode and are probably ignorable, so that's one rule that
on second thought maybe shouldn't be added...
The third is also an updated rules file NOT including the
StrictDuplicateCode check. This results in a more reasonable 5580 (only
751 more than the current ruleset).
I would be more than happy to provide a patch for the rules file if
there is a consensus on what should or should not be enabled, although I
hardly think a patch is required :) Once I know what the ruleset will
look like I'd like to start dealing with as many of the complaints as
possible, so the sooner there can be a resolution on this the better.
Thanks, I look forward to hearing everyone' thoughts :)
--
Frank W. Zammetti
Founder and Chief Software Architect
Omnytex Technologies
http://www.omnytex.com
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
