commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles Sadowski <gil...@harfang.homelinux.org>
Subject Re: [Math - 403] Never propagate a "NullPointerException" resulting from bad usage of the API
Date Fri, 04 Mar 2011 12:55:45 GMT
> >>>> SingularMatrixException, NonSymmetricMatrixException and the likes are
> >>>> really tightly bound to linear algebra and could be in the linear
> >>>> package where they are triggered. They could appear in the signatures
of
> >>>> 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 that,
> >>> if they were the majority of exception classes, I'd prefer to see them in
> >>> the "linear" package). However, I argue that when there exists an
> >>> "exception" to contain the general exceptions, then it is clearer to have
> >>> 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 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
in
> >>>>>>     user-provided code (for solvers, ode, matrix visitors ...)
but
> >>>>>>     ask them in documentation to use their own unchecked exception
> >>>>>>     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
would
> >>>>>> 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
NPE.
> >>>
> >>> 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.

> > 
> > 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)?


Regards,
Gilles

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


Mime
View raw message