commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luc Maisonobe <Luc.Maison...@free.fr>
Subject Re: [Math - 403] Never propagate a "NullPointerException" resulting from bad usage of the API
Date Sun, 06 Mar 2011 13:37:10 GMT
Le 06/03/2011 14:31, Gilles Sadowski a écrit :
> Hello Luc.

Hi,

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

OK, I'll wait until you have implemented the map feature.

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

Yes.

best regards,
Luc

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


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


Mime
View raw message