struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Laurie Harper <>
Subject Re: SAF1 Checkstyle
Date Mon, 19 Jun 2006 17:02:45 GMT
Frank W. Zammetti wrote:
> Ted Husted wrote:
>> You might at least want to start those discussions about the
>> checkstyle settings, so that we can develop a strategy about how they
>> would be fixed the next time there is a window of opportunity.
> Fair enough...
> I saw two issues that seem to account for a large number of the 
> complaints...
> * The issue "Variable xxxx must be private and have accessor methods.". 
>  This is the VisibilityModifierCheck check.  I don't think directly 
> addressing it, i.e., following the suggestion it gives, is the right 
> answer... I can't imagine it wouldn't break things, either in the SAF 
> codebase itself or in users' application code.
> Fortunately, there appears to be a setting in Checkstyle to deal with this:
> Or we could remove this check entirely.  I'd vote for just setting 
> protectedAllowed to true though.

Since this is a change to the checkstyle rules, not a change to the 
codebase, I don't think it needs to wait for a release/GA/whatever. 
Might as well just get it done now...

> Looking at core alone, that would probably get rid of half of the 
> remaining issues.  This is the only proposed rule change I have at this 
> point.
> * The issue "Expected @throws tag for xxxx".  This is coming up a lot 
> because there are runtime exceptions thrown in methods that are not 
> declared (which is of course valid, but doesn't follow the style rules). 
>  I would suggestion simply declaring them... I don't *think* that has 
> any side-effects... anyone think there is?

Declaring runtime exceptions in the throws clause of a method signature 
has no impact on the signature, so it should be safe. However, it's 
generally considered poor style (on what basis I don't know, doesn't 
seem like a problem to me, but...). If there's a checkstyle rule change 
that could be used to take care of this then, again, there'd be no need 
to wait. If it really does need code changes, it'd be worth holding off 
but opening a JIRA issue so we don't forget to come back later.


>> When I did a a time study in February, curing the the current errors
>> with the current settings would take at least 40 hard hours. We need
>> to find a way to do the work faster, or get more people to work on it.
> Agreed, and your estimate may even be too optimistic given that first 
> bullet above, if the rule change wasn't implemented.  I frankly don't 
> see another way to deal with that particular complaint.
>> (We probably should have made this a Google Summer of Code project.)
> Hehe, yeah :)
>> There is also the issue of how we want to handle the exception issues.
>> We will be addressing exception handling in SAF 2, so it's a pertinent
>> question.
> I certainly don't disagree that some exception handling updates should 
> be looked at, but I'm not sure that's pertinent in the context of 
> Checkstyle complaints... the exception bullet above I don't think should 
> be expanded to modifying how exceptions are handled now, it's just 
> getting rid of the complaints, which I think is as simple as declaring 
> the exceptions.
>> We might also consider fixing some of the errors at a time, for
>> example maybe just the exception handlers. That could have less of an
>> impact that trying to cure all six thousand at once.
> Same point as the above paragraph... I think we're talking about two 
> different things really.
> Although, if you wanted to tackle just one category of complaint at a 
> time, that would be fine... I was thinking of a package at a time, but 
> the work breakdown can certainly be done any of a number of ways.
> Ultimately though, that one rule change above would cut down a pretty 
> substantial number of complaints, and in this case I don't think 
> changing the rules is a cop-out.  So, I'd like to see a consensus 
> reached on that alone, everything else could be back-burnered as far as 
> I'm concerned.
>> -Ted.
> Frank

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message