commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles Sadowski <gil...@harfang.homelinux.org>
Subject Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Date Sun, 01 May 2011 10:48:14 GMT
On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
> > On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
> >> Converting some of my code to use trunk, I discovered that the
> >> binomialCoefficient methods no longer throw IllegalArgumentException
> >> when parameters are invalid.
> > The consensus was a singly rooted hierarchy ("MathRuntimeException").
> > The advantage being commonly agreed on was to offer the "map" functionality
> > for adding messages and context information.
> I guess I misunderstood and after really seeing the consequences in
> my own code, I am going to have to ask that we reopen that
> discussion - i.e., I would like us to throw IAE and other standard
> exceptions where appropriate, as in this case, as we have up to and
> through 2.2.  I know I said before that I did not see this as worth
> arguing about, but I really think this change is a bad API design
> decision that will cause needless hassle and surprising RTEs for
> users upgrading to the new version.

I'm astonished, and for the time being, will refrain from other comments.

> >> The javadoc asserts that
> >> MathIllegalArgumentException will be thrown in these cases, but that
> >> is not correct,
> > I don't understand; the code for "checkBinomial" can throw
> > "NumberIsTooLargeException" or "NotPositiveException" and they are
> > subclasses of "MathIllegalArgumentException".
> >
> Sorry. my mistake.
> >> since what is actually thrown now can differ
> >> depending on the parameter problem
> > That's a feature, naturally: Different problem, different exception.
> >
> That's where I disagree.  I see zero value added and in fact
> confusing complexity introduced by these exceptions.  When you ask
> for B(n,k) where k is not less than or equal to n, a standard IAE
> with a message that says precisely that (which the current message
> does) is clear and *better* that a "NumberIsTooLargeException". 
> What number?  I guess it must be k?  To figure it out you have to
> look at the exception message, which is *exactly the same message*
> that the old code reported.  If we really think we need to
> specialize and report different exceptions for every precondition
> violation (which I do not agree with), then these exceptions should
> be meaningful in the context of the API that is using them.  So
> here, for example, we would have to throw something like
> "CombinationSizeTooLargeForSetException." 

Then, we do _not_ disagree _now_. From the start, I stated that a consistent
design would be to define a specific exception for each specific that must
be reported, especially if it must contain complex functionality like
localization.
IIRC, either you or Luc (or both) did not want a large number of exceptions.
To keep the number down, we reuse less specific exception types (like
"NumberIsTooLargeException" in several contexts) and rely on the message(s)
for context information. Nothing lost from the previous situation (when one
*had* to rely solely on the message)!

To answer your question above: No, you don't have to "guess" which number is
too large; there is an accessor "getArgument()" that returns the number that
triggered the exception and another "getMax()" that informs you about the
maximum allowed number.

> >> and the resulting exceptions are
> >> neither standard IAEs nor descendents of MathIAE.
> > >From what I see, they are descendents of MathIAE.
> >
> >> I have patched
> >> the code to return a standard IAE (with localized message).  Per
> >> discussion in [1] it looks like we were close to consensus to favor
> >> standard exceptions and in this case,
> > No, that thread was discussing
> >   throwing standard "NullPointerException"
> > vs
> >   throwing a CM-specific "NullArgumentException" (subtype of MathIAE)
> > vs
> >   not checking for null pointer at all.
> > [I don't think that a consensus has been reached on that issue.]
> >
> > For all the other cases of invalid parameters passed to methods, it had
> > been settled already that "MathIllegalArgumentException" (or subclasses
> > thereof) would been thrown.
> >
> >> I would much rather return a
> >> standard IAE with meaningful error message rather than a
> >> non-standard RTE (with exactly the same error message and generally
> >> confusing type - e.g. "NumberIsTooSmall" when n, k parameters are
> >> not in the right order) and keep the javadoc simple.  Otherwise, the
> >> main method javadoc has to be rewritten to conform to what the code
> >> now does.
> > The Javadoc "@throws" tag is not incorrect:
> > -----
> >    * @throws MathIllegalArgumentException if preconditions are not met
> > -----
> > But it is not as precise as it could (by mentioning the types actually
> > thrown by "checkBinomial").
> > The main description is indeed a remnant of the old behaviour and it is yet
> > another proof that it is not good documentation practise to repeat the
> > (supposedly) same information several times.
> > Documentation for methods "binomialCoefficientDouble" and
> > "binomialCoefficientLog" also refer to the old behaviour and must be
> > updated.
> Regardless of how we settle this, we *must* keep the javadoc
> consistent with what the code does and we *must* document fully both
> preconditions and exceptions thrown.

Certainly, please open a JIRA ticket for this specific issue.


Gilles

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message