A few drawbacks to having IAE thrown by CM is that it complicates and blurres
things for those designing a handler that catches all CM exceptions. CM
advertising a factory that throws each exception 'type' under globally unique
conditions minimizes root cause analysis time and indirection.
This:
if (n <= 0) {
throw new IllegalArgumentException("n <= 0");
}
Misses out on the factory benefit of closing over the condition that checks and
throws the exception. It also makes the explanation for developing and using
CM longer...Is it a NOT_STRICTLY_POSITIVE_EXCEPTION or IAE that actually is a
NOT_STRICTLY_POSITIVE_EXCEPTION exception?
If I know that it's a NOT_STRICTLY_POSITIVE_EXCEPTION then I'm one step ahead.
Maybe I can simply set the argument to zero and try again, or just throw that
step away and continue.
If we standardizes on using Factory.checkNotStrictlyPositiveException(key, n)
the client developer can also grab the key and the n value and reconstruct the
message.
Also, this:
Factory.checkNotStrictlyPositiveException(key, n)
Is easier, more semantic, less error prone, and faster to write than:
if (n <= 0) {
throw new IllegalArgumentException("n <= 0");
}
And it provides more benefits:
- Parameter name(s)
- Parameter values
- More semantic
- Almost instant path to root cause
- Exception thrown by class (One method call - no unrolling of the cause stack)
- Exception thrown by method (One method call - no unrolling of the cause stack)
Also, if CM modularizes, then the Factory approach standardizes exception
generation and handling across the entire ecosystem.
Cheers,
Ole
On 12/23/2015 10:39 AM, 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!
Javadoc and stack trace are necessary and sufficient to fix those bugs.
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.
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.
And if somewhere else there is
throw new IllegalArgumentException("p <= 0");
I won't consider it duplicate code...
Gilles
Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
