Congratulations on the work put into [math]! Stephen
----- Original Message ----- From: "Phil Steitz" <[EMAIL PROTECTED]> To: "Jakarta Commons Developers List" <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
