On 12/23/2015 05:39 PM, Gilles wrote:
> On Wed, 23 Dec 2015 16:26:52 +0100, Thomas Neidhart wrote:
>> On 12/21/2015 04:41 AM, Gilles wrote:
>>> On Sat, 19 Dec 2015 11:35:26 -0700, Phil Steitz wrote:
>>>> On 12/19/15 9:02 AM, Gilles wrote:
>>>>> Hi.
>>>>>
>>>>> While experimenting on
>>>>> https://issues.apache.org/jira/browse/MATH-1300
>>>>> I created a new
>>>>> JDKRandomGeneratorTest
>>>>> that inherits from
>>>>> RandomGeneratorAbstractTest
>>>>> similarly to the classes for testing all the other RNG implemented
>>>>> in CM.
>>>>>
>>>>> The following tests (implemented in the base class) failed:
>>>>> 1.
>>>>>
>>>>>
>>>>> testNextIntNeg(org.apache.commons.math4.random.JDKRandomGeneratorTest)
>>>>> Time elapsed: 0.001 sec <<< ERROR!
>>>>> java.lang.Exception: Unexpected exception,
>>>>>
>>>>>
>>>>> expected<org.apache.commons.math4.exception.MathIllegalArgumentException>
>>>>>
>>>>>
>>>>> but was<java.lang.IllegalArgumentException>
>>>>> 2.
>>>>>
>>>>>
>>>>> testNextIntIAE2(org.apache.commons.math4.random.JDKRandomGeneratorTest)
>>>>>
>>>>> Time elapsed: 0.015 sec <<< ERROR!
>>>>> java.lang.IllegalArgumentException: bound must be positive
>>>>>
>>>>> This is caused by try/catch clauses that expect a
>>>>> "MathIllegalArgumentException"
>>>>> but "JDKRandomGenerator" extends "java.util.Random" that for those
>>>>> methods throws
>>>>> "IllegalArgumentException".
>>>>>
>>>>> What to do?
>>>>
>>>> I would change the test to expect IllegalArgumentException. Most
>>>> [math] generators actually throw NotStrictlyPositiveException here,
>>>> which extends MIAE, which extends IAE, so this should work.
>>>
>>> It turns out that, in the master branch, the hierarchy is
>>>
>>> RuntimeException
>>> |
>>> MathRuntimeException
>>> |
>>> MathIllegalArgumentException
>>>
>>> as per
>>> https://issues.apache.org/jira/browse/MATH-853
>>>
>>> [And the Javadoc and "throws" clauses are not yet consistent with this
>>> in all the code base (e.g. the "RandomGenerator" interface).]
>>>
>>> So, in 4.0, "JDKRandomGenerator" should probably not inherit from
>>> "java.util.Random" but delegate to it, trap standard exceptions raised,
>>> and rethrow CM ones.
>>
>> which is probably a good indication that the current situation in CM4
>> (as per MATH-853) was not a good design decision.
>
> It was consensual.
> What you express below is far more controversial.
>
>> I applied the changes as I thought the issue was settled, but it turns
>> out that some of its implications were not fully taken into account.
>>
>> From my POV, we should stick to the existing exceptions were applicable,
>> as this is what people usually expect and is good practice. This means
>> we should not create our own MathIAE but instead throw a standard IAE. I
>> understand that the MIAE was created to support the localization of
>> exception messages, but I wonder if this is really needed in that case.
>> Usually, when an IAE is thrown it indicates a bug, as the developer did
>> not provide proper arguments as indicated per javadoc. Now I do not see
>> the value of being able to localize such exceptions as *only* developers
>> should ever see them.
>
> This is a point I made a long time ago (not "in a far, far away galaxy").
> To which I got the answer that CM must provide a
> * detailed,
> * localizable,
> * textual
> error message.
>
> IMO, for a bug pointed to by an IAE, all the developer has to know is the
> stack trace.
>
> If we want to be "standard", we shouldn't even have to check for null or
> array length on caller's input data as we know that the JVM will do the
> checks and trivially throw standard exceptions on these programming errors!
Checking input parameters and explicitly throwing exceptions *is*
considered to be good practice.
Some of the commons libraries that exist for a very long time do it
differently (especially lang and collections), but this might lead to
inconsistent behavior (check the numerous bug reports related to that)
depending on input. I understand that 15 years ago this was not yet good
practice, but it is nowadays imho.
> Javadoc and stack trace are necessary and sufficient to fix those bugs.
I agree.
>> For any other exceptions (typically converge exceptions or similar)
>> which are clearly algorithm dependent, I fully support the design of 1
>> base MathRuntimeException with localization support.
>
> This is a really tiny subset. Ole's proposal could focus on those few
> cases.
I think the other existing exceptions and the way we handle localization
is fine, and does not really need a refactoring.
The use-cases provided in Ole's proposal look weird and seem again more
suited for something like spring rather than for a low-level library as
CM. There was the case how spring handles exceptions from hibernate, but
we should compare ourselves not with spring but with hibernate, Just my
2cents.
> For the rest, I'm all for being consistently "standard"; meaning that
> in CM we'd write code similar to the following:
>
> /**
> * Comment on "evaluate".
> *
> * @paran n Comment on "n".
> * @return the result.
> * @throws IllegalArgumentException if {@code n <= 0}.
> */
> public double evaluate(int n) {
> if (n <= 0) {
> throw new IllegalArgumentException("n <= 0");
> }
>
> return compute(n);
> }
>
> KISS.
This is what I had in mind. Thanks for sharing some code snippets.
> And if somewhere else there is
> throw new IllegalArgumentException("p <= 0");
> I won't consider it duplicate code...
For very common cases there could be utility methods.
Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
