next »

We have a class Vector2 (which contains a double "x", a double "y" and some methods), which is widely used by the 2d game engine. At the moment some methods that use it accept/return a clone, and some a reference. This is confusing and inconsistent, so I want to change it. Now, however I don't know which system to choose:

- Make the class immutable and always pass a reference
++ Good for memory
-- Many new allocations if a vector needs to be changed every frame (because it can't be modified)

OR

- Keep the class mutable, but always make a clone when accepting/returning an instance of it
++ It can be modified directly (for example, the position of an object which changes every frame
-- Every copy takes memory, and clones need to be made every time it's passed to another method

So, if there are/will be any advanced Java programmers here, what do you think is the fastest/best way?

Why not make it mutable AND pass a reference?  Maybe I'm not seeing the big picture here, but I see no reason why we can't adopt the best of both worlds.  Worst case is we have to build our own vector class and functions, but that's not overly tough in two dimensions.

We do have our own Vector class.
I could explain it all again here, but here's a link:
http://www.javagaming.org/forums/index.php?topic=17034.0

I think i can write 10 pages about why both immutable and cloneable are bad design decisions in this context, as it is in most contexts. As a general rule don't use immutability or cloning unless you have a fantastic reason. 

A quick google could give you the idea that I disagree with Joshua Bloch on this topic. if you read http://www.javapractices.com/Topic29.cjp you will see the example of the planet, and the fact that is immutable.

However the immutability here is used in good reason. The mass of a planet doesn't change. Thus it's a safe bet noone will use your setter anyway. A vector however is a higly dynamical concept. Vectors represent locations, speed, orientation, .... Any physical object easily undergoes changes on these values.
A vector itself has absolutely no meaning, a vector (5, -7) tells you nothing. Only when the vector is used by another object it's state has a meaning.

When you design a class, you should always try to see it as a virtual representation of a physical concept. While your birthdate will never change, your weight does.

The fact that String, Integer and other base classes are immutable, has nothing to do with the fact that they represent a physical concept. It's because they denote the difference between primitives and Objects .

consider the following code:

int i = 5;
int j = i;
i = 10 * i;

what's the value of j?

clearly the value should be 5 and not 50. Although this would be exactly the value if ints wouldn't be immutable. (regardless the fact that java doesn't support operator overloading)

Another reason is that in your post, you say yourself the fact that "the vector needs to be changed every frame", and this is exactly the reason it should be mutable. Because you want to change it.

My other objection is the using clone() in your code. In my opinion cloning an object should be the responsibility of whoever is dealing with the object (read, whoever calls your method).

The natural behaviour of getting an object is that you are talking to the actual object. If you change this by cloning the object, all calls that change the state of the object have no effect. It would force developers using your functions to read the documentation for every method to see if they receive a cloned object or not.

*getting a bit bored of writing, so I will stop here*

Last thing i would like to say is that the 2 approaches you present, both hurt performance and memory ugly.

approach 1: it's not good for memory at all, since you are allocating a new vector at EVERY frame, and you have no control over the lifetime of the object. Both memory and performance are targeted.

approach 2: not only does it take memory, it also takes time to create the objects.

I don't understand why you worry so much about changing the state of the vector, it seems natural to me.

btw, this seems to be a personal choice, im going into discussion with my room mate now, who think exactly the opposite

Ill keep you up to date on our discussion

please do, the vector class is used so much it's a pretty important decision

I don't understand why you worry so much about changing the state of the vector, it seems natural to me

imagine doing object1.setPosition(object2.getPosition()). the natural expectation is that afterwards object1 will not move when object2 is moved, but because they use the same Vector object it will
either way it's choosing between two bad things, and i chose one (maybe the wrong one, i'm still learning a lot)

ok, I was wrong and changed my mind. I was thinking too much in c++ terms and operator overloading. (which would actually creates a new object anyway)

It is obviously better to choose immutable or cloning because of referencing.

so looking back at the 2 possibilities

A)
- Make the class immutable and always pass a reference
++ Good for memory
-- Many new allocations if a vector needs to be changed every frame (because it can't be modified)
OR
B)
- Keep the class mutable, but always make a clone when accepting/returning an instance of it
++ It can be modified directly (for example, the position of an object which changes every frame
-- Every copy takes memory, and clones need to be made every time it's passed to another method


So basically the design choice here depends on what you expect happens less, setting/getting a vector, or modifying one.

I would think object1.setPosition(object2.getPosition()) will happen less frequent than changing the vector, since this must be done every frame for nearly every vector. In that case memory wouldn't be that big an issue anyway since only limited objects could have the same position. (further you could create a position class that has a vector, that would allow sharing anyway.)

This way changing a vector would be fast without allocations.

Also, because you have no control over the lifetime of an object (since this is done by the garbage collector). Allocating a new vector at every frame will probably hurt performance and memory more. I can imagine the memory building up fast into peaks, followed by a garbage collecting phase every so many frames which in turn probably hurts performance.

I should really try to make shorter sentences 

Ok, so Vector (and other geometry classes) should be mutable and every method/constructor that accepts and stores one should clone it?

This seems the best approach to me. But be sure to document your code.

not every method has to clone it, as long if it is a read-only method.

eg.

Vector2d.add(Vector v1) {
    this.x += v1.getx()
    this.y += v1.gety()
}

sorry, you already mentioned *store* in your description so my last comment was redundant, just wanted to be complete

[null]

Ok, so here is how it works:

- Methods that return a Vector2, don't clone it

Code:

public Vector2 getPosition() {
   return this.position;
}

- Constructors that accept a Vector2, clone it

Code:

public SomeObject(Vector2 position) {
  this.position = position.clone();
}

- Methods that accept a Vector2 and store it, use Vector2.set(Vector2 v)

Code:

public void setPosition(Vector2 position) {
  this.position.set(position);
}

- To ensure the above always works, Vector2 fields generally cannot be null

Code:

public SomeObject() {
  this.position = new Vector2();
}

- Methods that accept a Vector2 but don't store it, don't clone it

Code:

public double getDistanceTo(Vector2 otherpos) {
  return this.position.distanceTo(otherpos);
}

by the way, we have an "edit" button, which you can use to make changes to your previous posts

Ok I've update the engine and it now uses mutable vectors