commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles <gil...@harfang.homelinux.org>
Subject Re: [Math] Exceptions from "JDKRandomGenerator"
Date Mon, 21 Dec 2015 18:16:06 GMT
On Mon, 21 Dec 2015 10:01:33 -0700, Phil Steitz wrote:
> On 12/21/15 8:21 AM, Gilles wrote:
>> On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:
>>> On 12/20/15 8:41 PM, 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.
>>>
>>> I guess that is the only way out if we are going to stick with the
>>> MATH-853 setup.  This example illustrates a drawback identified in
>>> the thread mentioned in the ticket.  I would personally be happy
>>> reverting master back to the 3.x setup.
>>
>> We can't throw (!) the many discussions that led to MATH-853 by just
>> reverting on the basis of the current issue.
>>
>> Perhaps that the multiple hierarchies are better, maybe not.
>> But we have to figure out arguments that are more convincing than
>> nostalgia.
>>
>> For instance, IMO, we have contradicting wishes:
>> * Have CM exception inherits from the JDK ones with the same
>> semantics.
>> * Have all CM exceptions provide the same additional functionality
>> (the
>>   "ExceptionContext").
>
> In 3.x, MIAE extends IAE and we have all the context stuff working.
>
> I guess you are concerned about the "duplicated code" in the
> multiple roots extending the standard exceptions.  Here is the full
> extent of it:
>
>  /**
>      * @param pattern Message pattern explaining the cause of the 
> error.
>      * @param args Arguments.
>      */
>     public MathIllegalArgumentException(Localizable pattern,
>                                         Object ... args) {
>         context = new ExceptionContext(this);
>         context.addMessage(pattern, args);
>     }
>
>     /** {@inheritDoc} */
>     public ExceptionContext getContext() {
>         return context;
>     }
>
>     /** {@inheritDoc} */
>     @Override
>     public String getMessage() {
>         return context.getMessage();
>     }
>
>     /** {@inheritDoc} */
>     @Override
>     public String getLocalizedMessage() {
>         return context.getLocalizedMessage();
>     }
>
> I see that as NOT_A_PROBLEM.  In 3.x, we have 6 classes that need
> this little bit of similar code.  That is after 10 years of
> development.  I doubt we will have more than a few more needed at
> the top level.

It's not a problem of how many lines of code must be duplicated.

The problem is the duplication.
Duplication is not a best practice AFAIK.

>> The first looks nice from a user POV (as in "I know this already").
>> The second is an internal requirement to avoid code duplication.
>>
>> IMO, it always comes back to those opposite views we have
>> concerning the
>> place where human-readable messages should be generated: my view
>> is that
>> if a low-level library message bubbles up to the console, then
>> it's not
>> our fault, but the application programmer's.
> Meaningful exception messages are a best practice in Java.  They are
> a very useful aid in debugging and production support.
>>
>> Please assume that for a moment; then CM could have its algorithms
>> throw
>> some internal subclass of "MathRuntimeException" (whose type is
>> known to
>> the caller) and the caller can decide whether he must trap it in
>> order
>> to rethrow a standard type, or his own type (tailored to the
>> context which
>> he knows but CM doesn't).
>>
>> In that scenario, it is much easier for the caller when the 
>> low-level
>> library's exceptions hierarchy is unique (especially when he uses
>> several
>> libraries).
>> And it's also easier for us because we can avoid code duplication.
>
> Easiest is if the library follows the Java best practice, which is
> to favor standard exceptions.

We do not favour standard exceptions because we _have_ to define our
own.
And we have done this for various "good" reasons in line with other
best practices, subject to internal requirements.

> Then these can be caught without
> rummaging through library-specific javadoc to figure out what
> unchecked exception means what.  This is especially true for
> unchecked exceptions.

A standard exception does not mean anything specific.
So why bother with the "implementation detail"?

try {
   /** any code that deep down may or may not call CM */
} catch (RuntimeException e) {
   // Deal with it or rethrow.
}

will work, always.  User does not need to know anything to print
and read the meaningful message, only JDK's "RuntimeException" here.

The problem is _not_ there.

>> You never convinced me that you were aware of this type of
>> scenario and
>> why, somehow, it was trumped by a nicely formatted message on the
>> console.
>> The more so that a single hierarchy does not prevent the latter:
>> at the point where the message is printed, the exception type is
>> useless.
>
> I think we can have it both ways.  The 3.x setup allows us to extend
> standard exceptions *and* provide good exception messages.

The question is: Why extending standard exceptions?
We came to another answer in MATH-853.

So the problem cannot be solved as if it did not exist.

>>
>> To summarize, if the top-level message matters most, then I'd be
>> -1 to revert
>> (because that's an orthogonal issue); if having standard types
>> matters most,
>> I'd wait for use-cases that show how these types are used in the
>> caller's
>> code before trying to figure out how to satisfy them (while taking
>> our
>> internal requirements in to account too).
>
> Well, we have a nice example in front of us in our own test case
> here.  But the bigger deal is the pain for every user who when
> migrating from 3.x to 4 has to stop catching standard IAE if they
> did this and replace it with MIAE.  Note that the compiler will be
> no help there - failure to "comply" will cause the client app to "go
> boom."
>

All this has been said again and again.

I'd like that we do not take our own assumptions for granted
(be they the scenario which I described, or that compatibility
is the ultimate goal, even if the design is crap) but instead
that we could talk about what a live project CM can be, looking
to what can be done in possibly novel ways.

A new major version is an opportunity to legally break
compatibility in order to try new things.
Instead we have full-time discussions to avoid changing anything.

Users are never forced to follow development.

Gilles


> Phil
>>
>> In conclusion, let's not jump to conclusions. ;-)
>>
>>
>> Gilles


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


Mime
View raw message