On 8/8/05, Frank W. Zammetti <[EMAIL PROTECTED]> wrote:
> 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. Once we're clean, or nearly so, I'd be OK
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 "OK", 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.
> * 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 :)
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. Once
bitten, twice shy.
> * 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."
I'm OK 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...
>
> 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.)
Dang, I thought I caught all the tabs! I sure tried. Maybe people have
been adding them again...
--
Martin Cooper
> 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]
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
