On 3/29/11 8:01 AM, Gilles Sadowski wrote: >>>>> Modified: >>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java >>>>> URL: >>>>> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java?rev=1086057&r1=1086056&r2=1086057&view=diff >>>>> ============================================================================== >>>>> --- >>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java >>>>> (original) >>>>> +++ >>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java >>>>> Sun Mar 27 22:06:18 2011 >>>>> @@ -225,7 +225,8 @@ public class PearsonsCorrelation { >>>>> * @throws IllegalArgumentException if the arrays lengths do not >>>>> match or >>>>> * there is insufficient data >>>>> */ >>>>> - public double correlation(final double[] xArray, final double[] >>>>> yArray) throws IllegalArgumentException { >>>>> + public static double correlation(final double[] xArray, final >>>>> double[] yArray) >>>>> + throws IllegalArgumentException { >>>>> SimpleRegression regression = new SimpleRegression(); >>>>> if (xArray.length != yArray.length) { >>>>> throw new DimensionMismatchException(xArray.length, >>>>> yArray.length); >>> As is, it is not clear (from a design point-of-view) that it would make >>> sense to override the method (hence the legitimate request to make it >>> static). If polymorphism is really the intention, I think that it would >>> be worth adding an interface ("Correlation" maybe?). >> We have been talking about moving away from interfaces as the >> preferred way to support people plugging in alternative >> implementations because they have in several places gotten "behind" >> due to the fact that adding anything to them breaks compatibility. >> We should probably continue that discussion in a different thread. > OK, comments kept for a separate thread... > >> The method above is the core of the implementation > One is left wondering why the "core" does not need anything else from the > class (no fields, no methods); it is self-sufficient (hence the legitimate > request for making it static). It *is* the core method. It does not require instance data. This is very common in [math]. What it encapsulates is the core algorithm. Eliminating the possibility of overriding that is a bad idea, IMO, since it effectively makes the class a dead end. Looked at in another way, I would not make that method final or the class final, since I can imagine scenarios where users might want to swap out the implementation of the core method while maintaining the rest of the functionality of the class. There is no need to put this restriction on users or to break all of the existing users, so I do not think it is a good idea to make the method static.
Phil >> and making it >> static means that you can't extend this class using a different >> implementation. > Is that a problem (other than the practical one you invoke just below). Yes, it is generally considered bad design, since it makes it impossible for users to override the behavior in subclasses. >> It also breaks all existing code using the class. >> There are lots of other examples in [math] - throughout Commons >> actually - where methods *could* be static, but are not because we >> want to preserve the ability for ourselves or users to extend the >> classes and override the methods. > The issue which I'm raising is whether it is functionally clearer (as in > "How the class is _supposed_ to be used") to have (or not) a static method. > If the common case is inheritance (of which there is no example in CM) then > there must be an interface to single out the "core" functionality. > If instead, we focus on the specificity of the implementation, a user can > always "extend" the functionality by composition (i.e. his new class will > wrap a call to the static "PearsonsCorrelation.correlation" method). > Or just add a utils class to bundle the static methods if we really think we need them. This is consistent with design elsewhere in [math]. Phil >> All of the evaluate() methods, >> for example, in the individual descriptive statistics, for example, >> could be static. Convenience methods are provided in StatUtils to >> support static invocation. I think a CorrelationUtils class for >> this, Covariance and Spearman's - which all have core implementation >> methods that "could" be static - is a better approach if we feel >> that we need to provide support for the procedural API style here. > That's not only an issue of providing procedural style API. > > > 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
