Unsafe: Quaternion.IDENTITY ?!

I was browsing your JavaDocs and noticed something, which I call ‘bad coding style’ so I was interested in your opinion:



In the math package, the Quaternion class has a static, final attribute of itself, representing the IDENTITY: (x=0,y=0,z=0.w=1). Futher the class is not immutable.



Therefore, someone can code somewhere the following lines:



Quaternion id = Quaternion.IDENTITY;

id.x = …;

id.y = …;





which has the bad side effect that if the some programmer or someone else uses



Quaternion.IDENTITY;



it will not be the identity quaternion as expected. Therefore you should no use a public static attribute, but a static method returning a new instance:



Quaternion Quaternion.identity() { return new Quaternion(0,0,0,1);

Mostly that value is there for comparison purposes or to pass the identity quat into a function.



I'm not against removing it in favor of an isIndentity() method.  I believe we already have setIdentity

Hi,



I'd assume it's static to avoid your very 'solution', that is creating a new instance every time.



I don't know anything though :stuck_out_tongue:




  • Chris

Like Hal says, that would be bad performance and heavy. I think it's pretty easily understood that it shouldn't be modified.  We can only hold the devs hands so much. If they are determined to write bad code there isn't anything we can do to stop them.

Gentleman Hal said:

I'd assume it's static to avoid your very 'solution', that is creating a new instance every time.


I thought creating a new instance is o.k. because almost every method is overloaded with one an additional argument as storage and the as creates a new instance. For completeness, a method with a store attribute could be added as well.

public static Quaterion toIdentity(Quaternion store) { store.x = store.y = store.z = 0; store.w = 1;

isIdentity sounds very well as replacement.

I've added this locally.  In doing so, I also discovered a few places where we potentially could have been modifying IDENTITY. Expect to see it in the next cvs dump from nca.



fwiw, we should probably do this for Vector3f and Matrix4f as well. (remove statics I mean)