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 Sun, 06 Mar 2011 13:31:25 GMT
Hello Luc.

> > 
> >> [...]
> >>> 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".
> 
> I don't think this will happen very often.
> So let's create the new classes (I'll open a Jira issue for that) and
> move the linear algebra related exceptions back to the linear package.

I'll do the move. Please, don't touch anything in the "exception" package at
the moment: I'm currently implementing the "map" change.

> >>>>> [...]
> >>>>
> >>>> 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... :-)
> 
> Sorry, I misunderstood what you were writing. I was still talking about
> the changing the createXxxException (which only creates) into
> throwXxxException (which creates and throws), not about
> checkXxxCondition (which checks, creates and throws).
> 
> > 
> >> 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?
> 
> It's the same, I simply get confused. So go for it, but including the
> specialization and localization part that was in the former
> createxxxException. So basically we add an if (obj == null) wrapper
> aroud the 2.1 code.

So to be sure we say the same thing, you'd like to have (in "MathUtils"):
---
public static void checkNotNull(Object o,
                                Localizable pattern,
                                Object ... args) {
  if (o == null) {
    throw new NullArgumentException(pattern, args);
  }
}
---
Where we keep the _non-standard_ "NullArgumentException". [In line with what
we agreed on before: That it made sense (for the various reasons we dicussed
at length) to have a singly rooted hierarchy and thus depart from the
standard exceptions sub-classes.]

And I'll also add
---
public static void checkNotNull(Object o) {
  if (o == null) {
    throw new NullArgumentException();
  }
}   
---
for those developers who will be content with the default message ("null is
not allowed").

OK?


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