Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Received-SPF: pass (athena.apache.org: domain of phil.steitz@gmail.com
 designates 209.85.212.174 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:user-agent:mime-version:to:subject:references
         :in-reply-to:content-type:content-transfer-encoding;
        b=LRvEXTo2ttfak+FRD+EbCXNvM8jkW7ClZYg92c8iHSEWohMO4kHJw5NoGWIT7Xi/+1
         DkM1lLYUptqWhPw/yIfO4sMvVL4j10NlPQwpct26LIKYitI/edih6hmtZ+Tu59MGLhto
         2udHZUXa632BF+NtdNMJkFQKtN1SmHlrhA25k=
Message-ID: <4DBD7804.7020902@gmail.com>
Date: Sun, 01 May 2011 08:11:00 -0700
From: Phil Steitz <phil.steitz@gmail.com>
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US;
 rv:1.9.2.17) Gecko/20110414 Lightning/1.0b2 Thunderbird/3.1.10
MIME-Version: 1.0
To: Commons Developers List <dev@commons.apache.org>
Subject: Re: [math]  Restoring IAE to MathUtils#binomialCoefficient methods
References: <4DBC3460.1080407@gmail.com>
 <20110430233323.GI16459@dusk.harfang.homelinux.org>
 <4DBCF55A.70005@gmail.com>
 <20110501104814.GM16459@dusk.harfang.homelinux.org>
In-Reply-To: <20110501104814.GM16459@dusk.harfang.homelinux.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

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


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