commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Date Sun, 01 May 2011 22:18:20 GMT
On 5/1/11 2:29 PM, Gilles Sadowski wrote:
> On Sun, May 01, 2011 at 08:11:00AM -0700, Phil Steitz wrote:
>> On 5/1/11 3:48 AM, Gilles Sadowski wrote:
>>> 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)!
>> But in this case, my opinion is that IAE is just fine - there is
>> nothing more "specific" to communicate unless you want to define
>> something meaningful in the context of the API. "NumberIsTooLarge"
>> has no value here.
> I don't agree. This is the concept that describes the problem: indeed, the
> precondition is that "k" must be smaller than "n".
> This has the same value as the "out of range" concept where you give the
> value of some parameter and the values of the bounds.
>

No, it is actually a different concept.  In mathematical terms, it
is a binary relation that is failing here:  k < n.  What
"NumberIsTooLarge" (or "NumberIsTooSmall" which could also be
applied here, to n instead of k) conveys is unary.
>>  As I said, if we feel we need to make this
>> particular exception due to precondition violation more precise, we
>> would need to define an exception that refers to subset relation
>> size or somesuch, which I don't see as necessary or valuable.
> In principle, I've nothing against devising a deeper hierarchy that
> would make the type of the exception refer to the exact problem. However,
> there would indeed be not much practical value added if all use cases are
> about letting the exception bubble upwards until it aborts the program (at
> which point a human will read the error mesage).

Sounds like you are arguing here to just leave it as IAE, which is
fine by me.

>>> 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.
>> No, all that is reported is the *value*.
> Well, of course: CM is a numerical library, not a symbolic one.
>
>> To make this actually
>> work, you would have to do something like store and report the
>> formal parameter name.  I don't see the point in this in general
> Me neither.
>

Then I would claim "NumberIsTooLarge" and "NumberIsTooSmall" provide
no value.  You need to look at the exception message, which
thankfully we have preserved in this case, to understand what the
actual problem is.  The abstraction itself is worthless at least in
this case, IMO.

>> and
>> certainly not in this case, as what is problematic is the order
>> relation among the two parameters - not one is "too small" or the
>> other is "too large" but that they violate the stated preconditions
>> of the method.
> I really don't understand what bothers you!
> The precondition
>   k <= n
> means that if k > n, then the *value* of k is too large.
> You can elaborate on the context if you wish, but that does not change the
> basic problem.

You are missing the point, stated above.  The problem is that we
have a precondition that is a *relation* between the two parameters
and that precondition has been violated.  Throwing in a
context-unaware unary predicate that says "one of the parameters is
too large" adds nothing to precise the problem.

Phil
 

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


Mime
View raw message