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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
