commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luc Maisonobe <>
Subject Re: [Math - 403] Never propagate a "NullPointerException" resulting from bad usage of the API
Date Fri, 04 Mar 2011 15:47:08 GMT
Le 04/03/2011 13:55, Gilles Sadowski a écrit :
>>>>>> SingularMatrixException, NonSymmetricMatrixException and the likes
>>>>>> really tightly bound to linear algebra and could be in the linear
>>>>>> package where they are triggered. They could appear in the signatures
>>>>>> algorithms in other package that do call linear algebra, but this
is not
>>>>>> sufficient to put them in a general package.
>>>>> Yes, the "...MatrixException" classes are the borderline case (meaning
>>>>> if they were the majority of exception classes, I'd prefer to see them
>>>>> the "linear" package). However, I argue that when there exists an
>>>>> "exception" to contain the general exceptions, then it is clearer to
>>>>> them all there.
>>>>> Moreover "NonPositiveDefiniteMatrixException" is already a counter-example
>>>>> as it is used in "linear" and in "random".
>>>> It is used in random because the classes in the random package do use
>>>> the linear package to perform a Cholesky decomposition, which triggers
>>>> the exception. This is an example of the last sentence in my rationale
>>>> above. This appearance is not sufficient to put the exception out of linear.
>>> The class "CorrelatedRandomVectorGenerator" does not use the
>>> "CholeskyDecomposition" from package "linear"; it has its own "decompose"
>>> method which explicitly throws "NonPositiveDefiniteMatrixException"
>>> instances (at lines 214 and 222).
>> Right. I did not remember that (despite I wrote this code).
> So, what is the lesson from having this case? Is the whole method a duplicate
> that should be replace by a call to something in "linear"? Or is it a proof
> that some exceptions might be appropriately used outside of "linear"?

I think the method should be moved to a new
TruncatedCholeskyDecomposition or RectangularCholeskyDecomposition. I
don't think we can add it has a new implementation or it should be added
to the existing CholeskyDecomposition because the decomposition matrix
shape is not triangular.

>>> [...]
>>> I propose to extend the "MathThrowable" interface with methods declarations
>>> similar to what is done in [Lang] and implement the functionality in
>>> "MathRuntimeException". [While looking at it, I notice that [Lang] has
>>> a dedicated "exception" package...]
>> Fine.
> I'll create a JIRA issue.
>>>>>>>>  6) don't provide any top-level exception for errors occurring
>>>>>>>>     user-provided code (for solvers, ode, matrix visitors
...) but
>>>>>>>>     ask them in documentation to use their own unchecked
>>>>>>>>     that [math] will never see (i.e. remove FunctionEvaluationException,
>>>>>>>>     DerivativeException, MatrixVisitorException)
>>>>>>> +1 for removing all exceptions for which there doesn't exist
any "throw"
>>>>>>>    statement within CM. And also "MathUserException" (the few
uses of it in
>>>>>>>    trunk should be replaced).
>>>>>>>> I'm not sure for NullPointer/NullArgument. Perhaps our own
>>>>>>>> NullArgumentException with the [lang] exception context principle
>>>>>>>> be fine. I doubt we should check everything by ourselves
(it would be a
>>>>>>>> real performance killer), so whatever we choose there will
be untracked
>>>>>>>> NPE. We should check some arguments where it makes sense,
which is what
>>>>>>>> we already do at some places.
>>>>>>> +1 for dropping "NullArgumentException" and letting the JVM raise
>>>>> I'll also create a JIRA issue for reaching a conclusion on this one.
>>>> OK, I think we agree here. Phil preferred the way we created runtime
>>>> exceptions as of 2.1, though. Perhaps we could revive a simple
>>>> createNullPointerException without arguments ?
>>> But why would one prefer to write
>>> ---
>>>   throw MathRuntimeException.createNullPointerException();
>>> ---
>>> instead of
>>> ---
>>>   throw new NullPointerException();
>>> ---
>>> ?
>> For example to have a single point where to put breakpoints for
>> debugging. I used that a lot as such exceptions mainly occur during
>> development phase.
> Thus, IIUC, this is a developer feature.
> I understand the convenience, but I wouldn't trade code consistency for it.
> I'd rather keep "NullArgumentException" (or a "MathNullPointerException" if
> we want it to extend the standard NPE) so that you can put your breakpoint
> in the constructor of that exception. Thus you have the convenience without
> the code asymmetry.

OK for MathNullPointerException.

>>> Not only is this inconvenient, it is also dangerous: I was bitten more than
>>> once writing this:
>>> ---
>>>   MathRuntimeException.createNullPointerException();
>>> ---
>>> which the compiler happily accepted.
>> Of course, as it would have accepted:
>>   new NullPointerException();
>> Unfortunately, we cannot put the throw inside the method. Trying to do
>> this prevent the optimizer to wor properly as it does not know in the
>> caller that the method would never return, and it brings lots of
>> warnings from code checking tools for the same reason.
> The method alternative looses much of its appeal because it doesn't perform
> the complete action ("throw" included), as I had initially assumed when
> wrongly leaving out the "throw" statement. I wouldn't have written
>   new NullPointerException();
> as, to me, it would have _obviously_ looked wrong.
> I don't understand the problem with the "optimizer". There are several
> methods in CM that seem to perform similarly (i.e. they check something and
> throw an exception). An example is "checkOrder" in "MathUtils".
> In fact we could add a "checkNotNull" in "MathUtils":
> ---
> public checkNotNull(Object obj) {
>   if (obj == null) {
>     throw new NullPointerException();
>   }
> }
> ---
> Wouldn't that satisfy all needs (ability to set a breakpoint and no factory
> methods)?

No the check may succeed or not, so sometimes these methods do return.

What I had in mind was something like:

  public static void throwNPE() {
     throw new MySpecialNPE extends NullPointerException() {
        // my special code here

Such a method *never* returns but on the caller side, the compiler, and
the checking tools don't know it. Typically when you have code like:

  int var;
  if (obj != null) {
    var = obj.getVar();
  } else {

  // use var

Then tools will complain that var may not be initialized, which is
false. However it is only because throwNPE never returns that it is false.

Another way to put it that would probably work is:

  public static void checkNonNull(Object o) {
    if (o == null) {
       throw new MySpecialNPE extends NullPointerException() {
          // my special code here

Then the caller would do:

 int var = obj.getVar();


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

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

View raw message