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 (nike.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=ZN2DWZg0bk4U0IIKkJ+HnMjSCd6+OjIMAnrW0tEDxTjF1WS5Sq7/yRThkTi68aECas
         Ven6UsQlVH+ng5VI2ekDsbT7LKLXX2xqteT2ABJJVZD/mJI86TK2H/zmnQ5lzXJXH4XM
         U9elHV2hv3gJVgh1DzEoXAfMjI1enLqW0t9Po=
Message-ID: <4DBCF55A.70005@gmail.com>
Date: Sat, 30 Apr 2011 22:53:30 -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>
In-Reply-To: <20110430233323.GI16459@dusk.harfang.homelinux.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

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
"CombinationSizeTooLargeForSetException." 
>> 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.

Phil
 

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