Is this an immutable class?

Question

I have no idea what immutable class should look like but am pretty sure this one is. Am I right? If I'm not please specify what should be added/removed.

import java.io.Serializable;

public class Triangle implements IShape, Serializable {
    private static final long serialVersionUID = 0x100;

    private Point[] points;

    public Triangle(Point a, Point b, Point c) {
        this.points = new Point[]{a, b, c};
    }

    @Override
    public Point[] getPoints() {
        return this.points;
    }

    @Override
    public boolean equals(Object obj) {
        if (obj == null) return false;
        if (this == obj) return true;
        if (getClass() != obj.getClass()) return false;
        Point[] trianglePoints = ((Triangle) obj).getPoints();
        for (int i = 0; i < points.length; i++){
            if (!points[i].equals(trianglePoints[i])) return false;
        }
        return true;
    }
}

Will this do the trick?

@Override
    public Point[] getPoints() {
        Point[] copyPoint = {
                new Point(points[0]),
                new Point(points[1]),
                new Point(points[2]),};
        return copyPoint;
    }

Point class:

import java.io.Serializable;

public class Point implements Serializable {
    private static final long serialVersionUID = 0x100;

    public int x;
    public int y;
    public int z;

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

    public Point(Point that) {
        this.x = that.x;
        this.y = that.y;
        this.z = that.z;
    }

    public boolean equals(Object obj) { 
        // assume this is a typical, safe .equals implementation
        // that compares the coordinates in this instance to the
        // other instance
        return true;
    }
}

Answer 1

No, you can change what's in the Points array. If you want to make it immutable, have the getter hand out a copy of the Points array, not the original.

try this:

Triangle triangle = new Triangle(a, b, c);
triangle.getPoints()[1] = null;
System.out.println(Arrays.toString(triangle.getPoints()));

Also Point needs to be immutable (as Nikita Rybak points out). For how to copy arrays see how to copy an array in Java.

Answer 2

No, it's not. You expose the Point[] and a caller could modify its contents. Also, your class is not final, so someone could subvert it by subclassing it.

Answer 3

Close. For one thing, an immutable class should make it's fields final, but that's not a requirement.

However, you are exposing an array through the getter, and that is not immutable. Make a defensive copy using Arrays.copyOf(array, length):

@Override
public Point[] getPoints() {
    return Arrays.copyOf(this.points,this.points.length);
}

Answer 4

In order to be an immutable class, it is not enough that your methods promise not to change the object. In addition to having all fields be private and the methods not allow changing, you must also guarantee that the subclasses have the same promise of immutability. This includes making the class itself final, and ensuring that no references to the fields are ever returned.

A short, but excellent treatment of this can be found in this article:

http://www.javaranch.com/journal/2003/04/immutable.htm

Answer 5

No, it's definitely mutable.

Not only do you expose the actual Point[] array, you don't defensive-copy (Bloch 2nd ed., Item 39) the Point objects themselves when taking them in via the constructor.

1. The Point[] array could have items removed or added to it, so it's mutable.
2. You could pass in Points a, b, and c, then call setX() or setY() on them to change their data after construction.

Answer 6

It is not immutable because ...

Triangle t1 = new Triangle(new Point(0,0), new Point(0, 10), new Point(10, 10));
Triangle t2 = t1;

System.out.println( t1.getPoints()[0] );  // -> 0

t2.getPoints()[0].x = 10;

System.out.println( t1.getPoints()[0] );  // -> 10

Thus the class is not immutable because you can change the state of an instance (internal Point[] exposed) and this also changes the state of a reference to the same instance.

To make it a true immutable class, you would need methods to separately get X and Y from each point, for example:

public int getPointX(int point) { return points[point].x; }
public int getPointY(int point) { return points[point].y; }

or

public Point getPoint(int point) { return new Point(points[point]); }

or return a copy of the points like you suggested in your edit.

Answer 7

Not only do you need to provide an immutable copy of the internalised array, you also need to make sure that the Point object is immutable.

Consider the following use of the Point class in the standard Java API:

Point a = new Point(1,1);
Point b = new Point(1,1);
Point c = new Point(1,1);
Triangle triangle = new Triangle(a, b, c);
System.out.println(Arrays.toString(triangle.getPoints()));
c.setLocation(99,99);
System.out.println(Arrays.toString(triangle.getPoints()));

Answer 8

In addition to what others have already noted, you should:

• Make your Triangle class final to prevent the creation of mutable Triangles by subclasses.
• Declare all the fields final, to catch accidental modification of fields by the class itself.

In "Effective Java," Joshua Bloch provides a list of rules for immutable classes in general, in Item 15: Minimize Mutability.

Answer 9

Here's what I'd do to make this class immutable, with the help of Guava. I see from the @Override in the code you posted that IShape seems to require a Point[] from the getPoints() method, but I'm ignoring that for the sake of example since the use of object arrays is a rather poor idea, especially if you want immutability (since they cannot be immutable and all).

public final class Triangle implements IShape, Serializable {
  private final ImmutableList<Point> points;

  public Triangle(Point a, Point b, Point c) {
    this.points = ImmutableList.of(a, b, c);
  }

  public ImmutableList<Point> getPoints() {
    return this.points;
  }

  // ...
}

Point should also be more like:

public final class Point implements Serializable {
  /*
   * Could use public final here really, but I prefer
   * consistent use of methods.
   */
  private final int x;
  private final int y;
  private final int z;

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

  // getters, etc.
}

Answer 10

1) Make members private and final - so

private Point[] points; //should be 
private final Point[] points;

2) Make class final so it cannot be sub-classed

3) Exclusive access to mutable members (array) - meaning return copy of and not the reference to mutable members

For the best treatment of this subject refer to Joshua Bloch, Effective Java- item 15

Answer 11

This could be a better Point implementation. import java.io.Serializable;

public final class Point implements Serializable {
    private static final long serialVersionUID = 0x100;

    private final int x;
    private final int y;
    private final int z;

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

    public Point(Point that) {
        this(that.x, that.y, that.z );
    }

    public boolean equals(Object obj) { 
        // assume this is a typical, safe .equals implementation
        // that compares the coordinates in this instance to the
        // other instance
        return true;
    }
}

Answer 12

Other than exposing the array (as getters are wont to do) and not being final, being serialisable is "problematic".

As a very nasty man, when deserialising, I can get another reference to the internal array. The obvious fix for this is:

private void readObject(
    ObjectInputStream in 
) throws ClassNotFoundException, IOException {
    ObjectInputStream.GetField fields = in.readFields();
    this.points = ((Point[])(fields.get("point", null)).clone();
}

That still leaves the problem of points not being final and exposing the object without points initialised (or worse, but a bit thoeretical, partially initialised). What you really want is a "serial proxy", which you can find out about on the internets...

Note: If you implement equals you should also implement hashCode, probably toString and possible Comparable.

Answer 13

Point itself doesn't have to be immutable for Triangle to be immutable. You just have to do a lot of defensive copies so that nobody has a reference to the Point objects stored in the Triangle.

Also, shouldn't triangle a-b-c equal triange b-c-a (and 4 other permutations)