commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles Sadowski <gil...@harfang.homelinux.org>
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 Fri, 30 Jul 2010 23:19:45 GMT

Are there a definite coding style guidelines that justifies all those
changes?
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


Mime
View raw message