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