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 12:44:50 GMT
Le 05/03/2011 00:59, Gilles Sadowski a écrit :
> Hello.

Hi Gilles,

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

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

best regards,
Luc

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


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


Mime
View raw message