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.
* I
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() 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
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 NT 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 :) 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 :)

8/8/05, Frank W. Zammetti <fzlists (AT) omnytex (DOT) comwrote:
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 :)

You forgot FindBugs. It really does find bugs. ;-)

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.

Checkstyle is run from Maven, so whether or not we move to Checkstyle
3.5 right now depends on whether or not the Maven Checkstyle plugin
supports it. If it does, I'm fine with upgrading, assuming you can get
the new Checkstyle jar added to ibiblio.

As for adding more checks - given how many errors/warnings we have
now, I would be -1 on changes that generate more, until we seriously
reduce the current count. we're clean, or nearly so, I'd be K
with selectively enabling more checks.

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.

Per Craig's comment, I think we'd want to see how much this finds that
we'd want to clean up, versus what we want to keep the way it is, and
if this can be fine-tuned. I'm very much opposed to having warnings
show up that we declare to be "K", so the goal would have to be a
completely clean Checkstyle run rather than one that results in
warnings that "we can ignore".

* 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.

We could try it out, but I seriously doubt that we could come up with
a pattern that would make this rule pass. ;-) And changing the class
names to make it work might well break backwards compatibility.

* I
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 :)

I don't much care about this one. If a class has so many imports that
you can't see the wood for the trees, the class is probably due for
refactoring anyway. ;-)

I'm definitely with Craig, though, on the wildcard imports.
bitten, twice shy.

* CovariantEquals
The CheckStyle docs say it all on this: "Mistakenly defining a
covariant equals() method without overriding method
equals() can produce unexpected runtime behaviour."

I'm K with this one. I don't think we actually define equals() much anyway

* 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.

Sure. That's more a bug than a style issue anyway.

* 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.

I'm not sure if there are places in Struts where we do this and need
to do this. If not, then I'm fine with adding this check to make sure
that we don't do it in the future. If so, then we need to find a way
to avoid warnings in these particular cases.

* PackageDeclaration
This is almost a silly check frankly, but again, no harm no foul.

Yes, this is silly. If Struts committers are checking in classes with
no package declaration, then I think we have bigger problems. ;-)

* 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.

I'm pretty sure that modern JVMs deal with the double init, so I don't
think there's any perf issue. Also, it's sometimes clearer for Java
newbies to see things initialised. However, I don't really care one
way or the other.

* 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.

I agree with Craig on this one, but I'm not sure exactly what the rule
will flag and not flag. I'd be fine if it flags gratuitously
unnecessary parentheses and leaves those that aid in clarity, but that
seems like a rather subtle semantic for a tool like Checkstyle to deal
with. ;-)

At the following URL

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.)

Dang, I thought I caught all the tabs! I sure tried. Maybe people have
been adding them again

8/8/05, Martin Cooper <mfncooper (AT) gmail (DOT) comwrote:
8/8/05, Frank W. Zammetti <fzlists (AT) omnytex (DOT) comwrote:
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 :)

You forgot FindBugs. It really does find bugs. ;-)

Yep as well as questionable usage idioms like iterating over the
keys of a Map and calling map.get() for each one, versus iterating
over the entrySet() return. :-)

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.

Checkstyle is run from Maven, so whether or not we move to Checkstyle
3.5 right now depends on whether or not the Maven Checkstyle plugin
supports it. If it does, I'm fine with upgrading, assuming you can get
the new Checkstyle jar added to ibiblio.

As for adding more checks - given how many errors/warnings we have
now, I would be -1 on changes that generate more, until we seriously
reduce the current count. we're clean, or nearly so, I'd be K
with selectively enabling more checks.

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.

Per Craig's comment, I think we'd want to see how much this finds that
we'd want to clean up, versus what we want to keep the way it is, and
if this can be fine-tuned. I'm very much opposed to having warnings
show up that we declare to be "K", so the goal would have to be a
completely clean Checkstyle run rather than one that results in
warnings that "we can ignore".

Agreed. False positives will naturally lead to ignoring the true
negatives that might get intermixed.

* 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.

We could try it out, but I seriously doubt that we could come up with
a pattern that would make this rule pass. ;-) And changing the class
names to make it work might well break backwards compatibility.

* I
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 :)

I don't much care about this one. If a class has so many imports that
you can't see the wood for the trees, the class is probably due for
refactoring anyway. ;-)

I'm definitely with Craig, though, on the wildcard imports.
bitten, twice shy.

* CovariantEquals
The CheckStyle docs say it all on this: "Mistakenly defining a
covariant equals() method without overriding method
equals() can produce unexpected runtime behaviour."

I'm K with this one. I don't think we actually define equals() much anyway.

Although maybe we should? The configuration classes come to mind

* 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.

Sure. That's more a bug than a style issue anyway.

* 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.

I'm not sure if there are places in Struts where we do this and need
to do this. If not, then I'm fine with adding this check to make sure
that we don't do it in the future. If so, then we need to find a way
to avoid warnings in these particular cases.

* PackageDeclaration
This is almost a silly check frankly, but again, no harm no foul.

Yes, this is silly. If Struts committers are checking in classes with
no package declaration, then I think we have bigger problems. ;-)

Such as "it won't work on a JDK 1.4 platform" :-)

Craig

* 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.

I'm pretty sure that modern JVMs deal with the double init, so I don't
think there's any perf issue. Also, it's sometimes clearer for Java
newbies to see things initialised. However, I don't really care one
way or the other.

* 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.

I agree with Craig on this one, but I'm not sure exactly what the rule
will flag and not flag. I'd be fine if it flags gratuitously
unnecessary parentheses and leaves those that aid in clarity, but that
seems like a rather subtle semantic for a tool like Checkstyle to deal
with. ;-)

At the following URL

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.)

Dang, I thought I caught all the tabs! I sure tried. Maybe people have
been adding them again