You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Phil Steitz <ph...@steitz.com> on 2004/07/25 20:13:30 UTC
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
Re: [math] Design review pre 1.0
Posted by Al Chou <ho...@yahoo.com>.
--- Stephen Colebourne <sc...@btopenworld.com> wrote:
> Congratulations on the work put into [math]!
>
> Stephen
>
> ----- Original Message -----
> From: "Phil Steitz" <ph...@steitz.com>
> To: "Jakarta Commons Developers List" <co...@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...
Yes, many thanks to Phil for so much work done! I'm sorry I haven't had time
to contribute more.
Al
=====
Albert Davidson Chou
Get answers to Mac questions at http://www.Mac-Mgrs.org/ .
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [math] Design review pre 1.0
Posted by Stephen Colebourne <sc...@btopenworld.com>.
Congratulations on the work put into [math]!
Stephen
----- Original Message -----
From: "Phil Steitz" <ph...@steitz.com>
To: "Jakarta Commons Developers List" <co...@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