commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
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 13:14:41 GMT
Gilles Sadowski wrote:
> Are there a definite coding style guidelines that justifies all those
> changes?

Yes.  Like most Commons components, [math] uses CheckStyle to
specify and enforce coding style guidelines.  The file including the
checks is "checkstyle.xml" in the top-level svn directory.  We can
talk about removing / modifying some of the checks from that file,
but we need to either fix or "filter" exceptions in the source
(there is a facility in checkstyle to omit certain checks from
portions of the source).  The CheckStyle report generated by "mvn
site" shows where new or modified sources fail the checks.

Phil
> 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.
> 
>>      /**
>> +     * 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.
> 
>> +++ 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"?
> 
>> -        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).
> 
>> @@ -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!).
> 
>>      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.
> 
>>      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)"...
> 
>> -        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...
> 
>> -    /** 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...
> 
>>      /**
>>       * 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.
> 
>>       * 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.
> 
>> -        final boolean isMinim = (goal == GoalType.MINIMIZE);
>> +        final boolean isMinim = goal == GoalType.MINIMIZE;
> 
> No parentheses hinders the parsing by the human eye...
> 
>>      /**
>> -     * @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).
> 
>>                      } 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!
> 
>> -        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.
> 
>>          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").
> 
> 
> 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