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. 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.
> 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*. 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 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.
>>>> 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-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
