17
\$\begingroup\$

I recently started learning Java, and I decided to implement a basic vector system for another particle system I was building.

import java.util.*;

class Vector {
    int x;
    int y;

    public Vector(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public void add(Vector vector) {
        this.x += vector.x;
        this.y += vector.y;
    }

    public void sub(Vector vector) {
        this.x -= vector.x;
        this.y -= vector.y;
    }

    public void mul(Vector vector) {
        this.x *= vector.x;
        this.y *= vector.y;
    }

    public void div(Vector vector) {
        this.x /= vector.x;
        this.y /= vector.y;
    }
}

What can be improved? I'm new to Java, so constructive criticism on anything would be nice.

\$\endgroup\$
  • 6
    \$\begingroup\$ There's no physical or mathematical reason to do component-wise multiplication of vectors. The scalar product (a.x * b.x + a.y * b.y) is useful, and the cross-product of three-dimensional vectors, but [a.x * b.x, a.y * b.y] is not useful AFAIK. \$\endgroup\$ – kevin cline Oct 27 '14 at 21:25
  • \$\begingroup\$ @kevincline So, essentially, the only two useful functions here are add() and sub()? \$\endgroup\$ – Ethan Bierlein Oct 27 '14 at 22:30
  • 4
    \$\begingroup\$ Hello there, I want to point you to our meta, concerning: What you can and cannot do after receiving answers I rolled back your edit according to our policies ;) \$\endgroup\$ – Vogel612 Oct 28 '14 at 8:07
  • \$\begingroup\$ @Vogel612 Thanks, I didn't know bout' that. :) \$\endgroup\$ – Ethan Bierlein Oct 28 '14 at 12:42
  • 3
    \$\begingroup\$ To extend @kevincline's comment, there is no reason to do component-wise division of vectors, either. In fact, many math/physics/engineering professors would go ballistic if you said "divide vector a by vector b." :) \$\endgroup\$ – apnorton Oct 28 '14 at 20:50
20
\$\begingroup\$

Immutable objects are awesome. They are robust, predictable, and inherently thread-safe. Make the x, and y fields final, and change the operations to return the resulting Vector, for example:

class Vector {
    private final int x;
    private final int y;

    public Vector(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public Vector add(Vector other) {
        return new Vector(x + other.x, y + other.y);
    }

    public Vector sub(Vector other) {
        return new Vector(x - other.x, y - other.y);
    }

    public Vector mul(Vector other) {
        return new Vector(x * other.x, y * other.y);
    }
}

As in the example above, I recommend changing the name of the Vector parameter from vector to something else, like other, to make it perfectly clear that the method is dealing with another vector.

This way you'll be able to chain operations like this:

new Vector(1, 2).add(new Vector(3, 4)).sub(new Vector(5, 6)).mul(new Vector(7, 8));

It will be also useful to implement a custom toString method to make it easy to print a String representation of the vector, for example:

@Override
public String toString() {
    return String.format("(%s, %s)", x, y);
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I'm going to make one, comment, the reason I don't have return values for the math methods is so the user can type something like vector.add(new_vector) instead of vector = vector.add(new_vector). \$\endgroup\$ – Ethan Bierlein Oct 27 '14 at 20:59
  • 6
    \$\begingroup\$ Although object allocation is fast in java and the immutable design pattern has many advantages, the frequency with which vectors are manipulated in any kind of physics, graphics or particle system is going to put serious strain on the garbage collector and this will have a negative effect on performance. \$\endgroup\$ – Emily L. Oct 28 '14 at 0:05
  • 2
    \$\begingroup\$ Android and java game development library, libgdx, specifically advises against allocating any objects in the main rendering and processing loops but rather to allocated in the initialization stages. I believe that this is more than just speculation. I've seen this happen in high performance code first hand many times too. Going with an immutable design and realizing when your application is almost done that you need to change the API of one of the most frequently used classes is a royal pain in the rear end. I advise against an immutable design on this particular case. \$\endgroup\$ – Emily L. Oct 28 '14 at 0:16
  • 2
    \$\begingroup\$ @EmilyL. If this were a major issue, you wouldn't want to use vector objects at all; it's faster to have local x and y values, especially in an array, than to have Vector objects and array references everywhere \$\endgroup\$ – raptortech97 Oct 28 '14 at 4:36
  • 1
    \$\begingroup\$ @EmilyL. Certainly there are many examples where you need mutable design. We just don't have nearly enough code here to say that's the right way now. I didn't say immutable is the right way either. I suggested potential benefits and showed how to rewrite the OP's implementation. Without more code from the OP, I'm not going to speculate about the best final approach. It could be either way at this point. \$\endgroup\$ – janos Oct 28 '14 at 6:50
16
\$\begingroup\$

The most important points have already been mentioned:

  • Use double instead of int for the coordinates
  • Make the fields private

Concerning the recommendation to make the class immutable, I have to say that one has to really consider the possible application cases here. The Escape Analysis has significantly been improved in the recent Java versions. But for performance-critical applications, having to create possibly millions of objects may still have an impact on performance due to garbage collection (as long as there are no real value types in Java).

Apart from this possible performance impact, one should consider the intended semantics of such a class. Should such a vector really be used like a value, or should it be possible to pass a reference to another class, and let this class change the vector via this reference?


The div method is rather unusual: I can not imagine an application case where you want to do a component-wise division of the coordinates. Instead, you should consider adding operators with scalars:

public void mul(double factor) {
    this.x *= factor;
    this.y *= factor;
}

(note that you don't need a div method with a scalar, because you can simply call vector.mul(1.0/factor) to achieve the same result).


Concerning the intention to use this in a particle system / physics engine, you should consider introducing additional methods that are required frequently in such a context. For example,

public double lengthSquared() {
    return this.x*this.x+this.y*this.y;
}
public double length() {
    return Math.sqrt(lengthSquared());
}
public void normalize() {
    mul(1.0/length());
}

// When the vectors are interpreted as points, 
// you'll often need these:
// (Updated based on the comments: This is 
// basically the length of the difference of
// the two vectors)
public double distanceSquared(Vector other) {
    double dx = this.x - other.x;
    double dy = this.y - other.y;
    return dx*dx+dy*dy;
}
public double distance(Vector other) {
    return Math.sqrt(distanceSquared(other));
}

The actual set of operations (and their implementation, also in view of the question about immutability) will depend on how you intend to use this class.


But regardless of these usage-based methods: You should consider to implement the hashCode and equals methods. Otherwise, the following code would print false...

Vector v0 = new Vector(1,2);
Vector v1 = new Vector(1,2);
System.out.println(v0.equals(v1));

...although it would be reasonable to yield true here. An implementation of these methods could look like this:

@Override
public int hashCode() {
    long bits = 1L;
    bits = 31L * bits + Double.doubleToLongBits(x);
    bits = 31L * bits + Double.doubleToLongBits(y);
    return (int) (bits ^ (bits >> 32));
}

@Override
public boolean equals(Object object)
{
    if (object == null) return false;
    if (this == object) return true;
    if (!(object instanceof Vector)) return false;
    Vector other = (Vector)object;
    return this.x == other.x && this.y == other.y;
}
|improve this answer|||||
\$\endgroup\$
  • 4
    \$\begingroup\$ If they are final (immutable), they don't necessarily need to be private and it may be very convenient if they are not. a.x is a lot more readable than a.getX(). \$\endgroup\$ – kevin cline Oct 27 '14 at 21:26
  • 3
    \$\begingroup\$ Welcome to Code Review! Nice answer, I hope you will feel at home here. \$\endgroup\$ – Simon Forsberg Oct 27 '14 at 21:46
  • \$\begingroup\$ @kevincline There are different trade-offs regarding performance, readability and flexibility (and admittedly, I already spent some time thinking about these trade-offs for the particular case of 2D/3D Point/Vector classes). There is a large design space between the most abstract solution of an interface Vec2D and an interface MutableVec2D on the one hand, and the most... "pragmatic" solution of class Vec2D { public double x,y; }. I tend to make things flexible (to be able to change the implementation later), but there's certainly no silver bullet here. \$\endgroup\$ – Marco13 Oct 28 '14 at 9:29
  • 1
    \$\begingroup\$ Little nitpick: the difference between two vectors is a perfectly well-defined (and very very useful) mathematical operation. \$\endgroup\$ – David Z Oct 29 '14 at 5:26
  • \$\begingroup\$ @DavidZ You're right, that should have been "distance" (instead of "difference"), although the "distance" could simply stated to be the length of their difference - I'll edit this accordingly \$\endgroup\$ – Marco13 Oct 29 '14 at 9:20
13
\$\begingroup\$

Your code is performing integer division for the vector, which is bound to fail:

public void div(Vector vector) {
    this.x /= vector.x;
    this.y /= vector.y;
}

In this case, if the input vector is say x = 10 and y = 10, and we are x = 19 and y=19, the result will be:

this.x = 1;
this.y = 1;

which is counterintuitive.

I would recommend either not supporting div at all, or, alternatively, change your vector values to floating-point values (double).

I would also recommend that you change your model for vectors significantly, and instead of changing the current Vector with your arithmetic, that instead you return a new vector with the result:

class Vector {
    final double x;
    final double y;

    public Vector(double x, double y) {
        this.x = x;
        this.y = y;
    }

    public Vector add(Vector vector) {
        return new Vector(this.x + vector.x, this.y + v.y);
    }

    ....

}

Note how the x and y components of the vector are final, and the add returns a new vector.

|improve this answer|||||
\$\endgroup\$
13
\$\begingroup\$

Good start, but here are a few comments:

  1. The name for this class should be Vector2D rather than Vector, because you cannot represent a 3D or nD vectors using this class.
  2. int is not right type for representing points; doubles fit more

    double x;
    double y;
    
  3. By default, fields in Java have package access and it's usually a horrible idea. You should hide your fields as much as you can.

    private double x;
    private double y;
    
  4. This type of object screams immutability. Believe or not, when you start putting them in arrays and maps, it becomes really hard to deal their state, and end up copying them every time you use them.

    public Vector add(Vector vector) {
      Vector newVector = new Vector(this.x + vector.x,this.y + vector.y);
      return newVector
    }
    
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ usually the D or F refers to the double or float of the storage type (so he had a Vector2I strictly speaking) \$\endgroup\$ – ratchet freak Oct 27 '14 at 20:46
5
\$\begingroup\$

In addition to what is pointed out in the other answers, a few things:

First, Vector is a class in java.util so at the very least you shouldn't import java.util.* because there is a potential name conflict. It's better to import only the specific classes you need anyway.

Second, as pointed out in another answer, this class is only for 2D vectors so it should have a name that reflects that. This would also alleviate the potential conflict with java.util.Vector.

Continuing on the theme that the name of your class should reflect what it actually is, right now, what you've implemented is not a mathematical vector, so the name is misleading. The reason is that vectors are not numbers. The mathematical operations that work on numbers don't necessarily work on vectors; you can't divide one vector by another, for instance, and multiplication doesn't work by multiplying the components.

If you really want a class that represents a mathematical vector, you should give it methods for operations that one can actually perform on a vector. That would be some subset of the following:

  • vector addition
  • vector subtraction, which you could define as addition of the inverse
  • inversion, \$(x,y)\$ to \$(-x,-y)\$
  • scalar multiplication, \$a(x,y)\$ to \$(a x,a y)\$

and a few more operations that are not technically part of the mathematical definition of "vector", but may be useful for the applications you have in mind:

  • dot product a.k.a. scalar product, \$x_1 y_1+x_2 y_2\$
  • norm a.k.a. magnitude a.k.a. length, which is the square root of the dot product of a vector with itself (you can use Math.hypot)
  • distance between two vectors, which is just the norm/magnitude/length of the difference between the vectors (again, you can use Math.hypot)
  • unit vector a.k.a. direction, \$\bigl(\frac{x}{\text{norm}},\frac{y}{\text{norm}}\bigr)\$
  • projection on to another vector, which is the dot product of this vector with the other vector's unit vector
  • angle between this and another vector, which is the inverse cosine of the dot product of the two vectors divided by both of their norms (in fact it may be useful to have a method for the cosine of the angle, for efficiency)
  • one that might be useful: the z-component of the cross product \$x_1 y_2-x_2 y_1\$
|improve this answer|||||
\$\endgroup\$
2
\$\begingroup\$

Unless you are positive that you will be dealing with 2D system of coordinates you should implement vector as an array of coordinates rather than "x" and "y", it might have a constructor that accepts dimensions number and creates array of doubles like others suggested, or accepts array of coordinates in constructor.

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.