Updated response interspersed. Pls correct / comment as necessary...
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?
Let's just say we have done our best ;-)
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.
All serializable classes have serialVersionUIDs. Some serialization incompatible changes may be introduced in .x releases (prior to 2.0); but the current impls work correctly and have working tests.
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
Javadocs have been improved and formatting fixed. Class javadoc may be thin for classes that just implement a simple interface (e.g. some of the distribution classes); but method javadoc is complete. User Guide is also now complete.
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.
Decided not to separate within packages.
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)
Decided to wait on adding strictfp support to later release, as if we do this, users should be able to choose, since performance impact can be significant.
6) ComplexFormat doesn't extend the JDK Format class
Fixed.
7) ComplexMath is a utils class, which would be named ComplexMathUtils in
lang or collections (although the JDK Math class sets an alternate
precedent)
Fixed
The constructor is private, which I would recommend is changed to
public (aids tools like Velocity). Also Beta/Erf/Gamma.
Not changed (no consensus to change).
8) @author tags are often missing
Not changed (consensus is no author tags). If "anonymous" tags are j-c standard, need to write a script to add these.
9) Factorys use the method newInstance() to obtain an instance - getInstance() seems more typical of commons, and allows you to return a cached value.
Factories are dynamically configurable, so consensus was to let the client handle caching factory instances (stay with newInstance() semantics). Internal clients have been changed to cache (default) factory instances.
10) SummaryStatistics/DescriptiveStatistics are factories but aren't named as such.
Consensus was that these are not really factories, so names have not been changed.
They also use Class.forName() which is dubious in a class loader
environment.
Fixed.
11) Ensure there are no TODO comments in the Javadoc (OK in code) For example GammaDistributionImpl
Fixed
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)
Fixed.
13) Ensure every class has a constructor - never rely on the auto generated one, for example BivariateRegression
Fixed.
14) Some import statements are rather screwed visually, eg Product
Fixed.
15) Dependencies!!!!
No (hard) runtime dependencies left. [discovery] and [logging] are required to compile [math] and to use the factory configuration features that they support. Factory methods that use [discovery] have been modified to catch NoClassDefFound and return instances of default implementations.
Phil
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
