On 8/8/05, Martin Cooper <[EMAIL PROTECTED]> wrote:
> 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. ;-)
>
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. 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".
>
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.
>
> > * 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.
>
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...
> >
> > 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]
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
