commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luc Maisonobe <>
Subject Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Date Mon, 02 May 2011 17:58:59 GMT
Le 02/05/2011 17:07, Phil Steitz a écrit :
> On 5/1/11 3:02 PM, Gilles Sadowski wrote:
>> On Sun, May 01, 2011 at 08:34:31AM -0700, Phil Steitz wrote:
>>> On 5/1/11 6:00 AM, Gilles Sadowski wrote:
>>>> On Sun, May 01, 2011 at 12:48:14PM +0200, 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
>>>>>>>> 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"
>>>>>>> for adding messages and context information.
>>>>>> I guess I misunderstood and after really seeing the consequences
>>>>>> 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.
>>>> There is no one single best API design. What counts most IMO is that it is
>>>> consistent and leads to clean and lean code.
>>>> The old exception factories à la
>>>> -----
>>>>    MathRuntimeException.createIllegalArgumentException("Error with an argument
{0}", x);
>>>> -----
>>>> failed on all accounts.
>>>> Now, I would like to settle this issue once for all. Should all CM
>>>> exceptions inherit from the standard Java exceptions hierarchies (rooted
>>>> IAE, UOE, NPE, AE, etc.)?  Although it had been answered "no" previously,
>>>> was not my preferred choice. I could make it "yes" now but I'd rather not
>>>> changed that back and forth again and again.'
>>> I apologize sincerely for opening this up again.  I don't think
>>> *all* exceptions thrown by [math] should derive from standard
>>> exceptions, other than I guess Exception itself.  I do think,
>>> however, that [math] *should* throw standard exceptions directly
>>> when appropriate and in general favor standard exceptions.  In
>>> particular, I would prefer that we revert to the 1.0-2.2 behavior of
>>> throwing IllegalArgumentException when preconditions are violated.
>>> I personally have no problem with using the exception factories and
>>> will volunteer to retrofit the code if we can agree to this change.
>> We agreed to provide enhanced "context-enabled" exceptions as a feature to
>> users. Throwing plain standard exceptions contradicts that goal.
>> I propose to rework the hierarchies so that MathIAE will inherit from the
>> standard IAE. This will solve the annoyance you would have had when
>> upgrading your code. [And we'll keep the "addMessage" feature together with
>> accessors like "getArgument" and "getMax" etc.]
>> The exception factories were a bad design idea (IMHO of course; plenty of
>> arguments given elsewhere in the last 6-8 months). One of the goals of the
>> exceptions refactoring was to get rid of them.
>>> Once again, I hate to flip-flop, but this is an important and
>>> impactful decision and I have now experienced the upgrade pain
>>> (unexpected RTEs when upgrading) directly so would like to ask that
>>> we revisit it.
>> Not "unexpected RTEs". Incompatibilities are to be expected.
>> Anyways, this won't happen with the new-new solution.
>>> To be clear, my opinion is that for all of the RTEs currently
>>> generated by MathRuntimeException.createXxxException, we should
>>> generate and throw these exceptions directly, as appropriate, using
>>> the factory to enable localization.
>> No, I like the current design; I didn't like the one you want to revert to.
>> Consistency implies that *all* exceptions thrown from CM must behave the
>> same way. I thus propose to add an interface like (maybe a better name?):
>> ---
>> interface ContextedException {
>>    void addMessage(Localizable pattern,
>>                    Object ... arguments);
>>    void setContext(String key, Object value);
>>    Object getContext(String key);
>>    Set<String>  getContextKeys();
>>    String getMessage(final Locale locale);
>>    String getMessage(final Locale locale,
>>                      final String separator);
>> }
>> And all CM exceptions will implement this interface. [Instead of
>> automatically inheriting the behaviour by being subclasses of
>> "MathRuntimeException".]
> I would prefer as stated above to revert to actual RTEs per 2.x
> behavior.  Above would be an improvement, as at least the unexpected
> RTEs at upgrade would not bite (as they did me), but I see no reason
> to add this machinery which is no less complex than what we had in
> 2.x.  Lets see what others think.

Do the above mean we would have:

   public class MathIllegalArgumentException
     extends IllegalArgumentException
     implements ContextedException

If so, then I am OK with this.


> Phil
>> Gilles
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail:
>> For additional commands, e-mail:
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:

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

View raw message