Stephen,
This ROCKS, but its going to take me all weekend to respond accurately... ;-) I have some interesting comments for many of these issues.
-Mark
Stephen Colebourne wrote:
I have performed a lightweight review of [math] from a code/style POV (not technically as I'm not mathematical...)
1) Remember your public API that you must maintain includes both public and protected classes/methods/fields. Are you confident that every method/field (especially fields) are correctly and suitably named for you to want to keep?
2) Wherever there is implements Serializable you should have a serialVersionUID static field. And are you confident that the implementations are well enough established to allow for serialization? What about transient fields? Serialization is definitely not about adding the implements clause to everything.
3) Javadocs are sometimes thin. On occasions, they are written as paragraphs visually but without the HTML <p> tag. (eg. UnivariateRealSolver) or missing, eg StandardDeviation
4) You might want to consider separating interfaces from implementations. Perhaps all interfaces in the base package, or impl subpackages. Then again the current layout may be best.
5) Is double suitable for these calculations? Should the strictfp flag be used? (I have no idea as to the answer, but I have to ask)
6) ComplexFormat doesn't extend the JDK Format class
7) ComplexMath is a utils class, which would be named ComplexMathUtils in lang or collections (although the JDK Math class sets an alternate precedent) The constructor is private, which I would recommend is changed to public (aids tools like Velocity). Also Beta/Erf/Gamma.
8) @author tags are often missing
9) Factorys use the method newInstance() to obtain an instance - getInstance() seems more typical of commons, and allows you to return a cached value.
10) SummaryStatistics/DescriptiveStatistics are factories but aren't named as such. They also use Class.forName() which is dubious in a class loader environment.
11) Ensure there are no TODO comments in the Javadoc (OK in code) For example GammaDistributionImpl
12) Should EmpiricalDistribution really have methods taking File and String filePath - is File not enough? The implementation actually seems to do something different (beware file encodings)
13) Ensure every class has a constructor - never rely on the auto generated one, for example BivariateRegression
14) Some import statements are rather screwed visually, eg Product
15) Dependencies!!!! commons-discovery can and should be an optional dependency. In fact it may already be, as each call to DiscoveryClass is in an Exception catch, which ought to trap ClassNotFoundException. However, I don't think that the default implementation behaviour is actually working which needs fixing.
commons-lang is only used for NestableException. This is a dubious dependency - the relevant code can be copied, and [lang]s ExceptionUtils is perfectly able to handle extracting cause parameters by reflection. (Don't copy NestableException - copy the code within)
commons-logging is not used directly, just via beanutils and discovery (and is a classic example of commons chained dependencies that should be avoided)
commons-beanutils doesn't seem to be used, so shouldn't be in project xml
commons-collections TreeBag is used by Frequency - it might be worth considering removing this dependency, but it is the hardest. (CollectionUtils.NATURAL_COMPARATOR is a simple class to copy)
Anyway, I hope this focuses attention on what needs doing (boring-wise) before 1.0.
Stephen
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
