You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by "Frank W. Zammetti" <fz...@omnytex.com> on 2005/08/09 05:05:58 UTC
Proposed CheckStyle changes
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: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: Proposed CheckStyle changes
Posted by "Frank W. Zammetti" <fz...@omnytex.com>.
On Tue, August 9, 2005 1:04 am, Martin Cooper said:
> You forgot FindBugs. It really does find bugs. ;-)
Your right, I did :) My builds at work do include it actually.
> 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.
Good point... anyone know the answer to this? I guess subconsciously I
had made the assumption that the plugin wouldn't care what version it was
using, maybe that's not a safe assumption though.
> 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.
Fair answer. If everyone else feels the same way I can move ahead with
getting rid of as many as possible with the current ruleset. Frankly,
most of them are things like tabs-to-spaces and simple javadoc fixups, so
I can probably mow them down rather quickly.
> 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".
As another person pointed out, most of what this catches seems to be
dealing with the license header... this check didn't seem to be as
configurable as I would have liked, so I think in retrospect I'd probably
withdraw this suggestion.
I agree with your feeling that the goal should definitely be a completely
clean run. That is always my goal with my own code. However, having
worked with all these static analysis tools a fair bit, my experience
tells me that no matter how you can or try to configure the rules, they
sometimes flag things that either aren't actually problems (that's fairly
rare actually) or flag things that you as the developer have a reason for
having broken the rule. So, while 100% clean should definitely be the
goal, 99% is OK in my opinion, so long as you can explain that 1%.
>> * AbstractClassName
>
> 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 actually only count 9 such warnings, so it might not be as onerous as it
seems... that's using the default match pattern, and certainly that could
be tweaked. I too was concerned with breaking backwards-compatibility
though, so certainly this has to be done with caution. Many of the rules
result in things that can be changed with little or no risk, this probably
isn't one of them.
>> * ImportOrder
>
> 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. ;-)
Fair enough :) It doesn't sound like your against it, jsut don't see much
value in it... your probably right, aside from consistency I suppose,
which should count for something I figure :)
>> * StringLiteralEquality
>
> Sure. That's more a bug than a style issue anyway.
Absolutely :) Many of the things CheckStyle can check for aren't actually
style-related any more. None of these types of issues show up currently
by the way, so this would just be a check for the future.
>> * IllegalCatch
>
> 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.
I count about 55 of these types of warning... I have not looked at the
actual code at all though, so I don't know if they are legitimate
rule-breakages or not.
>> * PackageDeclaration
>
> Yes, this is silly. If Struts committers are checking in classes with
> no package declaration, then I think we have bigger problems. ;-)
No question :) I'm of the "no harm to foul" school of thought on checks
like this though.
>> * ExplicitInitialization
>
> 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.
I suspect your right about modern JVMs, I'd kind of be surprised if they
didn't handle this.
For a long time I always explicitly initialized everything... came from my
C background I suppose. Then, I read something that changed my mind on
this, and I wish I could remember where, when or who said it... "When you
explicitly initialize variables to their default values, it shows that you
don't trust the language". It's not much of a reason to not explicitly
initialize I suppose, but for some reason if kind of struck a chord with
me.
Well, certainly this isn't any huge thing either way.
>> * UnnecessaryParentheses
>
> 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. ;-)
In looking through the results, it looks like almost all of them in fact
are parens around return values. I do agree that CheckStyle isn't going
to be able to always properly determine when parens are really
"superfluous" or not, although I think it uses the simple definition that
if they aren't strictly needed to ensure proper order than they are
superfluous. Clearly that doesn't take added clarity into account, and it
can't, I agree.
> Dang, I thought I caught all the tabs! I sure tried. Maybe people have
> been adding them again...
There's gotta be a Maven plugin for that, no :)
> Martin Cooper
Frank
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: Proposed CheckStyle changes
Posted by "Frank W. Zammetti" <fz...@omnytex.com>.
>> > * 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" :-)
No need to bother us with minutia like that Craig :-) LOL
> Craig
Frank
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: Proposed CheckStyle changes
Posted by Craig McClanahan <cr...@gmail.com>.
On 8/8/05, Martin Cooper <mf...@gmail.com> wrote:
> On 8/8/05, Frank W. Zammetti <fz...@omnytex.com> 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: dev-unsubscribe@struts.apache.org
> > For additional commands, e-mail: dev-help@struts.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: Proposed CheckStyle changes
Posted by Martin Cooper <mf...@gmail.com>.
On 8/8/05, Frank W. Zammetti <fz...@omnytex.com> 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: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: Proposed CheckStyle changes
Posted by "Frank W. Zammetti" <fz...@omnytex.com>.
Craig McClanahan wrote:
> Although the general principle is sound, there can be a
> counter-argument that cut-n-paste can sometimes avoid undesireable
> cross-package dependencies. Most rules engines I've seen allow you to
> create an exceptions list where the developers say "yes, I know this
> violates the
> style rule, but we want it this way anyway." Does CheckStyle have that feature?
CheckStyle does allow some per-rule configuration, but I honestly have
always just used the rules in their default forms, so I'm not sure what
can or can't be done specifically.
But, looking at the docs right now, for this rule it looks like the only
relevant option is the the property min, which is the minimum number of
lines that must be equal to be considered a duplicate. Might be
something to play with.
> I assume we also want to prohibit wild card imports? Besides driving
> me nuts, they are also semantically dangerous.
I couldn't agree more... that one was already enabled in the current
ruleset or I defintely would have mentioned it :)
> "Simply ignoring" doesn't work if you are catching checked exceptions,
> so again I would hope this could be selectively turned off on
> individual use cases.
You can set it to ignore a particular exception class name(s), that's
it... the rule checks for catching java.lang.Exception, java.lang.Error
or java.lang.RuntimeException. The rationale is that you should be
checking for subclasses, something I personally agree with... that's why
I said some situations can be acceptable to ignore... if you *know*
there is a reqson you are catching Exception, then it's ignorable (IMO).
> Personally, I've been burnt enough times by assuming developers know
> what the operator hierarchy is to err on the side of redundant
> parentheses whenever there is any possible doubt. On the other hand,
> I could easily be convinced that redundant parentheses around a return
> value don't add anything useful, so it wouldn't bother me to need to
> remove those.
>
> Without examining the current reports, I'll bet this one triggers a
> bunch of warnings on the Struts code base.
I stopped counting at around 300 :) Judging by how far down I was in
the report, I'd bet there around around 600-800 of them.
Unfortunately, that rule doesn't seem to have any configuration options,
so it'd be a judgement call... I certainly see where your coming from,
so I'll just say that if there was some consensus on the point I'd be
willing to do the leg work... as you said, parens around returns can
probably be removed, but maybe everyone thinks erring on the side of
more parens in other places is better... I don't think I'd be convinced,
but I understand the rationale and would go along with the crowd :)
> We can catch tabs instead of spaces? Cool! :-)
Yeah, that one I like too :) I also love the one that checks for lines
longer than 80 characters.
> Craig
Frank
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: Proposed CheckStyle changes
Posted by Craig McClanahan <cr...@gmail.com>.
In general, doing this sort of nagging on the developer list is
*exactly* the right thing to do for getting patches you believe in
moved forward :-). I'm not directly involved in Struts 1.3
development so I'll leave overall acceptance to others, but a couple
of general comments on your suggestions follow (snipping to focus on
just the relevant issues).
On 8/8/05, Frank W. Zammetti <fz...@omnytex.com> wrote:
[snip]
> * 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.
Although the general principle is sound, there can be a
counter-argument that cut-n-paste can sometimes avoid undesireable
cross-package dependencies. Most rules engines I've seen allow you to
create an exceptions list where the developers say "yes, I know this
violates the
style rule, but we want it this way anyway." Does CheckStyle have that feature?
[snip]
> * 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 assume we also want to prohibit wild card imports? Besides driving
me nuts, they are also semantically dangerous.
> * 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.
"Simply ignoring" doesn't work if you are catching checked exceptions,
so again I would hope this could be selectively turned off on
individual use cases.
[snip]
> * 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.
Personally, I've been burnt enough times by assuming developers know
what the operator hierarchy is to err on the side of redundant
parentheses whenever there is any possible doubt. On the other hand,
I could easily be convinced that redundant parentheses around a return
value don't add anything useful, so it wouldn't bother me to need to
remove those.
Without examining the current reports, I'll bet this one triggers a
bunch of warnings on the Struts code base.
[snip]
> 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.)
We can catch tabs instead of spaces? Cool! :-)
> Frank W. Zammetti
Craig
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org