Potential bug in setNormal of Plane

In the following overloaded method of the Plane class:



[java]public void setNormal(float x, float y, float z) {

if (normal == null) {

throw new IllegalArgumentException(“normal cannot be null”);

}

this.normal.set(x,y,z);

}[/java]



The check for a null could actually throw an invalid exception in the case where a user instantiates the Plane with the default constructor and sets the normal of the plane using this method. I think it’s just leftover copy pasta from the other method:



[java]public void setNormal(Vector3f normal) {

if (normal == null) {

throw new IllegalArgumentException(“normal cannot be null”);

}

this.normal.set(x,y,z);

}[/java]



I know that the odds of this happening is pretty slim, but it’s a possibility.



-Feen

the null check should be removed, why spend 1 whole cpu circle to check it, if it is gonna crash anyway.

@tralala said:
the null check should be removed, why spend 1 whole cpu circle to check it, if it is gonna crash anyway.


This kind of micro-optimization rarely has any benefit at all where as an appropriate error message can save lots of time.

"normal cannot be null" is a way better message than the alternative which would effectively be "random null pointer, please look at the JME source code to debug your problem." No thanks. ;)

Yeah, a check against a null value is almost always worth the effort and works as it should in the other setNormal method.



The biggest problem with this specific case is that it checks the class member against null as opposed to the method parameter. This, in the circumstance I supplied in my post, would definitely cause an unnecessary crash that would be hard to narrow down without looking through the source.



-Feen

Yeah, the solution in this case is to create the normal, I think… don’t throw the exception.

Actually “this.normal” can never be null because its initialized to a new vector and the user cannot change it. Still I removed the check as its unnecessary.