commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luc Maisonobe <Luc.Maison...@free.fr>
Subject Re: svn commit: r1139126 - in /commons/proper/math/trunk/src: main/java/org/apache/commons/math/analysis/solvers/ site/xdoc/ test/java/org/apache/commons/math/analysis/ test/java/org/apache/commons/math/analysis/solvers/
Date Fri, 24 Jun 2011 07:49:59 GMT
Le 24/06/2011 09:09, Dennis Hendriks a écrit :
> I agree with you that braces should always be used. Personally, I do
> have one exception though, and that is if the statement following it is
> on the same line. That way, it is one line, instead of 3 lines, which
> makes it more readable. If the 'if' statement spans multiple lines, I
> always include braces, even if there is only one single statement
> involved. But that is just me...

So we have at least two different opinions here, which is good. Let's 
see what other people think.

>
> I think the rule is already active in Checkstyle. I think I just chose
> to ignore it here.

No, the rule is not active in our checkstyle configuration. I have 
tested it, there are about 130 violations of this rule in our code base, 
so it is really simple to fix all of them.

>
> In general (not just this Checkstyle rule), is it allowed to ignore
> checkstyle errors, if one believes that the code would be better if the
> rule is ignored for a specific instance; that is, in case of false
> positives?

No, we tend to fix really everything, at least at release time. If we 
let warnings, it becomes difficult to sort real problems from false 
positive. People have to take time to check every warning manually each 
time, so allowing warnings really wastes more time in the long term that 
it saves in the short term.

If a piece of code really needs to not comply to a rule, then we use the 
warning suppression mechanism around that piece of code. See at the end 
of our checkstyle configuration, there are already 5 
SuppressionCommentFilter set up.

This also applies to findbugs, and we use findbugs-exclude-filter for 
false positives.

Luc

>
> Dennis
>
>
> Luc Maisonobe wrote:
>> Hi all,
>>
>> The provided patch for MATH-599 includes a number of no-brace if
>> statements like the following ones:
>>
>>> + if (method == Method.ILLINOIS) f0 *= 0.5;
>>> + if (method == Method.PEGASUS) f0 *= f1 / (f1 + fx);
>>
>> I'm not sure if we have an unwritten rule for this, but personally I
>> dislike this style a lot. Checkstyle provides a NeedBraces rule to
>> avoid this.
>>
>> What about activating this rule ?
>>
>> Luc
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message