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: r980981 - in /commons/proper/math/trunk/src: main/java/org/apache/commons/math/analysis/ main/java/org/apache/commons/math/analysis/interpolation/ main/java/org/apache/commons/math/distribution/ main/java/org/apache/commons/math/exception/ ...
Date Sat, 31 Jul 2010 18:55:19 GMT
Le 31/07/2010 01:19, Gilles Sadowski a écrit :
> 
> Are there a definite coding style guidelines that justifies all those
> changes?

Yes, it is the checkstyle.xml file.

> I give below my preferences when they don't match yours.
> 
>> @@ -34,7 +34,7 @@ public interface BivariateRealFunction {
>>       * @return the value.
>>       * @throws FunctionEvaluationException if the function evaluation fails.
>>       */
>> -    public double value(double x, double y)
>> +    double value(double x, double y)
>>          throws FunctionEvaluationException;
> 
> "public" is redundant but it is one place where I find it useful: i.e. when
> you copy the interface contents in an implementing class, you have the
> correct declaration.

Yes, but it is considered better style to avoid redundant modifiers.

> 
>>      /**
>> +     * Simple constructor.
>>       * @param a Spline coefficients
>>       */
>> -    public BicubicSplineFunction(double[] aV) {
>> +    public BicubicSplineFunction(double[] a) {
>> +        this.a = new double[N][N];
>>          for (int i = 0; i < N; i++) {
>>              for (int j = 0; j < N; j++) {
>> -                a[i][j] = aV[i + N * j];
>> +                this.a[i][j] = a[i + N * j];
>>              }
>>          }
>>      }
> 
> Why "a" instead of "aV"?
> 1. You have to write more characters ("this.a[i][j]" instead of "a[i][j]").
> 2. It would be fine if the two were of the same "kind" but here one is a
>    vector and the other a matrix.

You are right. The warning was because the javadoc parameter did not
match the method parameter name. I change the method parameter, we could
change the javadoc if you prefer.

> 
>> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/interpolation/LinearInterpolator.java
Fri Jul 30 22:03:04 2010
>> @@ -25,6 +25,7 @@ import org.apache.commons.math.util.Loca
>>  
>>  /**
>>   * Implements a linear function for interpolation of real univariate functions.
>> + * @version $Revision$ $Date$
>>   */
>>  public class LinearInterpolator implements UnivariateRealInterpolator {
>>      /**
>> @@ -34,8 +35,8 @@ public class LinearInterpolator implemen
>>       * @return a function which interpolates the data set
>>       * @throws DimensionMismatchException if {@code x} and {@code y}
>>       * have different sizes.
>> -     * @throws NonMonotonousSequenceException if {@code x} is not sorted in
>> -     * strict increasing order.
>> +     * @throws org.apache.commons.math.exception.NonMonotonousSequenceException
>> +     * if {@code x} is not sorted in strict increasing order.
>>       * @throws NumberIsTooSmallException if the size of {@code x} is smaller
>>       * than 2.
>>       */
> 
> Why the fully-qualified name for "NonMonotonousSequenceException" but not
> for "DimensionMismatchException"?

Because DimensionMismatchException is really used in the class and hence
the import is done above. However, since NonMonotonousSequenceException
appears only in javadoc and not in real throws, the import does not
appear (and if it appears, other tools complain because they do no check
the javadoc and think the import is unused). I had a hard time finding
if the exception was really thrown or not by the way as it is not
specified in "throws" declarations and only appears in javadoc. I think
it is thrown by MathUtils.

> 
>> -        if (xLen == 0 || yLen == 0 || z.length == 0
>> -            || f.length == 0 || f[0].length == 0) {
>> +        if (xLen == 0 || yLen == 0 || z.length == 0 || f.length == 0 || f[0].length
== 0) {
> 
> I find the original more readable (I would even prefer one condition per
> line).

We can put one condition per line if you prefer, but the warning was
about the || operator being on the beginning of line whereas it should
have been on the end of the previous line. It's the convention we adopted.

> 
>> @@ -433,7 +434,7 @@ class TricubicSplineFunction
>>          for (int i = 0; i < N; i++) {
>>              for (int j = 0; j < N; j++) {
>>                  for (int k = 0; k < N; k++) {
>> -                    a[i][j][k] = aV[i + N * j + N2 * k];
>> +                    a[i][j][k] = aV[i + N * (j + N * k)];
>>                  }
>>              }
>>          }
> 
> Here, you didn't rename "aV" to "a" (which is fine!).

There was no javadoc warning here.

> 
>>      public NumberIsTooLargeException(Localizable specific,
>>                                       Number wrong,
>>                                       Number max,
>>                                       boolean boundIsAllowed) {
>>          super(specific,
>> -              (boundIsAllowed ?
>> -               LocalizedFormats.NUMBER_TOO_LARGE :
>> -               LocalizedFormats.NUMBER_TOO_LARGE_BOUND_EXCLUDED),
>> +              boundIsAllowed ?
>> +              LocalizedFormats.NUMBER_TOO_LARGE :
>> +              LocalizedFormats.NUMBER_TOO_LARGE_BOUND_EXCLUDED,
>>                wrong, max);
> 
> The extra parentheses make the thing more readable.

They trigger warnings.
The problem with lots of warnings is that important things are hiding
within numerous unimportant things. So these unimportant things becomes
important not by themselves but because they hide the rest.

When I tried checkstyle last night, there were several hundreds of
warnings. After having tuned checkstyle to avoid the ones I considered
false alarms (the missing javadoc for the about 250 error message
enums), there were 127 warnings left which I had to chase one by one. I
fixed 111 of them and went myself to sleep with 16 messages still there.

I still consider this is too much. Rather than having to ask ourselves
each time if these warnings can be accepted or not, up to now we have
decided to solve *all* of them and to publish commons-math only with 0
checkstyle warnings and 0 findbugs warnings. This is true even if some
of these warning are really silly; we still fix them.

> 
>>      public RealPointValuePair(final double[] point, final double value,
>>                                final boolean copyArray) {
>> -        this.point = (copyArray ?
>> -                      (point == null ? null : point.clone()) :
>> -                      point);
>> +        this.point = copyArray ?
>> +                     ((point == null) ? null : point.clone()) :
>> +                     point;
>>          this.value  = value;
>>      }
> 
> Same remark as the preceding one, but here it's not coherent since you left
> redundant parentheses in "(point == null)"...

It's not me who decide what is redundant and what is not. I am happy
with redundant parentheses when checkstyle is happy with them. I'm sure
the guys who developed it had a good reason to accept them around the
conditional and not around the complete statement. Most probably because
there are other operators before or after the statement in one case and
not in the other, but I will not bother looking the exact reason. The
rule is: follow checkstyle decisions.

> 
>> -        if (startConfiguration == null
>> -            || startConfiguration.length != startPoint.length) {
>> +        if ((startConfiguration == null) ||
>> +            (startConfiguration.length != startPoint.length)) {
>>              // no initial configuration has been set up for simplex
>>              // build a default one from a unit hypercube
>>              final double[] unit = new double[startPoint.length];
> 
> I don't get your rules: sometimes you remove redundant parentheses,
> sometimes you add them...

My own rules are pretty much the same as yours I think: redundant
parentheses can help reading. These rules are only superseded when
checkstyle say otherwise. If it complain, then I remove the extra
parentheses. Simple and consistent.

> 
>> -    /** Number of gradient evaluations. */
>> -    private int gradientEvaluations;
>> -    /** Objective function gradient. */
>> -    private MultivariateVectorialFunction gradient;
>>  
>>      /** Convergence checker.
>>       * @deprecated in 2.2 (to be removed in 3.0). Please use the accessor
>> @@ -60,9 +55,15 @@ public abstract class AbstractScalarDiff
>>       */
>>      protected double[] point;
>>  
>> +    /** Number of gradient evaluations. */
>> +    private int gradientEvaluations;
>> +
>> +    /** Objective function gradient. */
>> +    private MultivariateVectorialFunction gradient;
>> +
> 
> Sometimes you seem to want to gain space (e.g. by concentrating a list of
> condition checkes on a single longish line) but here you add blank lines
> between field declarations...

Perhaps is it because I'm short ? I like to see vertical space.

> 
>>      /**
>>       * Simple constructor with default settings.
>> -     * The convergence check is set to a {@link SimpleScalarValueChecker},
>> +     * The convergence check is set to a {@link org.apache.commons.math.optimization.SimpleScalarValueChecker},
>>       * the allowed number of iterations and evaluations are set to their
>>       * default values.
>>       */
> 
> The fully-qualified name is not necessary: When you read the source you can
> get it from the "import" statement, when you read the HTML you can click on
> the link.

If I added the fully qualified name, it was because there was no import.
And if there was no import it is because some tools didn't like them
(either checkstyle or eclipse, I don't remember here).

> 
>>       * during QR decomposition, it is considered to be a zero vector and hence the
>>       * rank of the matrix is reduced.
>>       * </p>
>> -     * @param qrRankingThreshold threshold for QR ranking
>> +     * @param threshold threshold for QR ranking
>>       */
>> -    public void setQRRankingThreshold(final double qrRankingThreshold) {
>> -        this.qrRankingThreshold = qrRankingThreshold;
>> +    public void setQRRankingThreshold(final double threshold) {
>> +        this.qrRankingThreshold = threshold;
>>      }
> 
> "this" is not necessary. I think that the code is more readable when it is
> omitted whenever possible.

You are right. The this was here beforehand because the parameter name
and the field name were the same. However, since checkstyle complained
(it did not analyze correctly the fact it was a simple set method for
which it has exceptions in its rule), I changed the parameter name. I
forgot to remove the "this" specifier. Feel free to remove it.


> 
>> -        final boolean isMinim = (goal == GoalType.MINIMIZE);
>> +        final boolean isMinim = goal == GoalType.MINIMIZE;
> 
> No parentheses hinders the parsing by the human eye...

Yes, but checkstyle didn't like them.

> 
>>      /**
>> -     * @param func Function.
>> +     * @param f Function.
>>       * @param x Argument.
>>       * @return {@code f(x)}
>> +     * @throws FunctionEvaluationException if function cannot be evaluated at x
> 
> I am inclined to write full sentences (with "the" and punctuation).

Yes, but I think at this time I really started to be really tired to
have to do all this cleanup.

> 
>>                      } else {
>>                          b = u;
>>                      }
>> -                    if (fu <= fw
>> -                        || w == x) {
>> +                    if (fu <= fw || w == x) {
>>                          v = w;
>>                          fv = fw;
>>                          w = u;
>>                          fw = fu;
>> -                    } else if (fu <= fv
>> -                               || v == x
>> -                               || v == w) {
>> +                    } else if (fu <= fv || v == x || v == w) {
>>                          v = u;
>>                          fv = fu;
>>                      }
> 
>>>From my recent experience (with precisely this code), I strongly favour
> making *one* check per line!

OK, just put the || at the end of lines instead of the beginning of the
next line.

> 
>> -        final boolean isEqual = (Math.abs(xInt - yInt) <= maxUlps);
>> +        final boolean isEqual = Math.abs(xInt - yInt) <= maxUlps;
> 
> Sigh.
> 
>>          /**
>> -         * Create an iterator (see {@link MultidimensionalCounter#iterator()}.
>> +         * Create an iterator
>> +         * @see #iterator()
> 
> This is not exactly the same.

So I failed. Fix it  as you understand it and make sure checkstyle does
not complain.

> 
>>          for (int i = 0; i < N; i++) {
>>              for (int j = 0; j < N; j++) {
>> -                final int index = i + N * j;
>>                  coeff[i + N * j] = (i + 1) * (j + 2);
>>              }
>>          }
> 
> I suspect that the JIT will optimize the "index" declaration. Keeping it
> helps in debugging (when using "println").

When adding println, you can add the index computation too. when you
remove the println, you remove the computation too.

This one is in fact really important. In the company I work for, we also
develop embedded software for things that are really critical (like
helicopter engines or airplanes brakes systems). One of the very very
important rules is : no dead code (and in fact no code for which you
cannot track back to the specification the exact reason why it is here.
We are not doing life critical software in commons-math, but some rules
are good to obey.

I will go back to remove more warnings. There is one I would prefer you
fix yourself: the missing javadocs for the Order class, and the javadoc
for its Direction enumerate. I didn't understand why there are two
levels here. While you will be there, don't forget to also add the
javadoc for INCREASING and DECREASING. I guess I could do that, but you
can too.

While we are discussing checkstyle settings, please look at the
following thread from last year
<http://markmail.org/message/63slpacuks3n6i2x>. Most of the checks
discussed at this time have been added, but some important ones are
still not there: MultipleStringLiterals, FinalLocalVariable and
FinalParameters.

MultipleStringLiterals was delayed because of the error messages.
Implementing this was the reason I started to add some static final
Strings in some classes. As we have found another solution with the
enums, I think it will be easy to add.

The FinalLocalVariable and FinalParameters on the other hand will
probably need many changes in variable declarations, so it will take
time. However, for new code I would advise everyone to put final on
almost all variables if possible, this will ease the transitation later on.

Thanks for having reviewed this boring commit. I hope I will not have to
do this sort of thing often.

Luc

> 
> 
> Gilles
> 
> ---------------------------------------------------------------------
> 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