commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <>
Subject Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Date Sun, 01 May 2011 05:53:30 GMT
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.
>> 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
>> 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.


To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message