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 23:59:34 GMT
Hello.

> [...]
> > 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.

If you are going to create new classes in "linear" so that the current CM
code only throws "...MatrixException"s from inside that package, I'm not
going argue anymore about moving those exceptions to "linear". But, if this
situation arises again, then we will create a incompatibility by moving the
exception from "linear" to "exception"... That's why I think that it's safer
(from a stability point-of-view) to have them all in "exception".

> >>> [...]
> >>
> >> 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.

With a "checkNotNull" function, it should not be necessary.
But I didn't quite understood your reasoning concerning such a method and
whether it's OK to implement or not (cf. below).

> > 
> >>>
> >>> 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.

I'd say that they return most of the time, and that's fortunate... :-)

> 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 {
>     throwNPE();
>   }
> 
>   // 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.

Personally, I don't like this kind of syntax. The good programming rule is
to assign at declaration. [Unfortunately, one is sometimes forced to write
in this way and most often than not because of checked exceptions!]

> 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
>        };
>     }
>   }

How is this different from my version above?

> Then the caller would do:
> 
>  checkNonNull(obj);
>  int var = obj.getVar();

That's exactly the intended usage!


Gilles

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


Mime
View raw message