commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Stephen Colebourne" <scolebou...@btopenworld.com>
Subject Re: [math] Design review pre 1.0
Date Sun, 25 Jul 2004 19:13:46 GMT
Congratulations on the work put into [math]!

Stephen


----- Original Message -----
From: "Phil Steitz" <phil@steitz.com>
To: "Jakarta Commons Developers List" <commons-dev@jakarta.apache.org>
Sent: Sunday, July 25, 2004 7:13 PM
Subject: Re: [math] Design review pre 1.0


> 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
>


---------------------------------------------------------------------
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