commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <p...@steitz.com>
Subject Re: [math] Design review pre 1.0
Date Sun, 25 Jul 2004 18:13:30 GMT
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: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message