Library Class: Point - Follow up

Question

This is a follow up of this post.

Changes:

1. I tried harder to make my code readable yet concise.
2. All Point instances are immutable and unique.
3. New, useful features have been added (like reflection, rotation, translation, etc.)

This class depends upon this class. Line will follow soon. For the time being assume that Out.format(double) uses String.format() internally to make the output organised.

package library.geometry;

import com.sun.istack.internal.NotNull;
import library.Out;
import library.util.Numbers;

import java.util.Arrays;

/**
 * This class is an abstraction of a point in the 2D, X-Y plane. A point has
 * an abscissa (the X-coordinate) and an ordinate (the Y-coordinate) in the
 * cartesian system of representation. In the polar representation, it has a
 * radial distance (from the origin) and an angle (with the positive
 * direction of the X-axis).
 * <p>
 * In order to avoid ambiguity, the constructor has been made private. Instead,
 * convenient factory methods are provided in the {@code Factory} inner class.
 * <p>
 * <b>NOTE:</b> All instances of this class are immutable. All methods return
 * new instances with (usually) different co-ordinates. Remember to store the
 * result in a new variable, don't just invoke a method. <i>There are no
 * {@code void} methods in this {@code class}.</i>
 *
 * @author Subhomoy Haldar (ambigram_maker)
 * @version 2.0
 */
public class Point {

    /**
     * The abscissa or X-coordinate.
     */
    private final double x;
    /**
     * The ordinate or Y-coordinate.
     */
    private final double y;

    /**
     * This inner class provides the interface for creating new instances of
     * the class {@code Point}.
     */
    public static final class Factory {

        /**
         * Returns a point with the given cartesian co-ordinates.
         *
         * @param x The abscissa.
         * @param y The ordinate.
         * @return A point with the given cartesian co-ordinates.
         */
        public static Point cartesianForm(double x, double y) {
            return new Point(x, y);
        }

        /**
         * Returns a point with the given polar co-ordinates.
         *
         * @param r     The distance from the origin.
         * @param theta The angle (in radians) made with the positive X-axis.
         * @return A point with the given polar co-ordinates.
         */
        public static Point polarForm(double r, double theta) {
            return new Point(r * Math.cos(theta), r * Math.sin(theta));
        }
    }

    /**
     * The canonical constructor that takes cartesian co-ordinates.
     *
     * @param x The abscissa.
     * @param y The ordinate.
     */
    private Point(double x, double y) {
        this.x = x;
        this.y = y;
    }

    /**
     * Returns the distance between the invoking point and the point passed as
     * the argument.
     *
     * @param other The other point to measure the distance from.
     * @return The distance between the invoking point and the point passed as
     * the argument.
     */
    public double getDistanceFrom(@NotNull Point other) {
        double deltaX = this.x - other.x;
        double deltaY = this.y - other.y;
        return Math.sqrt(deltaX * deltaX + deltaY * deltaY);
    }

    /**
     * Returns the abscissa of the point.
     *
     * @return The abscissa of the point.
     */
    public double getX() {
        return x;
    }

    /**
     * Returns the ordinate of the point.
     *
     * @return The ordinate of the point.
     */
    public double getY() {
        return y;
    }

    /**
     * Returns the radial distance of the point (i.e distance from the origin).
     *
     * @return The radial distance of the point (i.e distance from the origin).
     */
    public double getR() {
        return Math.sqrt(x * x + y * y);
    }

    /**
     * Returns the angle (in radians) made by the line joining this point
     * and the origin with the positive direction of the X-axis.
     *
     * @return The angle (in radians) made by the line joining this point
     * and the origin with the positive direction of the X-axis.
     */
    public double getTheta() {
        return Math.atan2(y, x);
    }

    /**
     * Returns a new instance of {@code Point} that is the result of
     * <i>translating</i> the current point by a specified amount in the
     * positive direction of the axes.
     *
     * @param dx The distance which the point moves along the positive X-axis.
     * @param dy The distance which the point moves along the positive Y-axis.
     * @return A new instance of {@code Point} that is the result of
     * <i>translating</i> the current point by a specified amount in the
     * positive direction of the axes.
     */
    @NotNull
    public Point translate(double dx, double dy) {
        return new Point(x + dx, y + dy);
    }

    /**
     * Returns a new {@code Point} that would represent this point if the
     * co-ordinate axes are rotated (in the anti-clockwise sense) by the
     * specified angle (in radians).
     *
     * @param theta The angle (in radians) to rotate the axes by.
     * @return A new {@code Point} that would represent this point if the
     * co-ordinate axes are rotated (in the anti-clockwise sense) by the
     * specified angle (in radians).
     */
    @NotNull
    public Point rotateAxesBy(double theta) {
        double sin = Math.sin(theta);
        double cos = Math.cos(theta);
        double X = y * sin + x * cos;
        double Y = y * cos - x * sin;
        return new Point(X, Y);
    }

    /**
     * Returns a new instance of {@code Point} that has the same co-ordinates
     * as {@code this} one.
     *
     * @return A new instance of {@code Point} that has the same co-ordinates
     * as {@code this} one.
     */
    @NotNull
    public Point copy() {
        return new Point(x, y);
    }

    /**
     * Returns the image of this {@code Point} that is obtained by reflection in
     * the given {@code Line}.
     *
     * @param line The <i>surface</i> for reflection.
     * @return The image of this {@code Point} that is obtained by reflection in
     * the given {@code Line}.
     */
    public Point getReflectionFrom(Line line) {
        Line perpendicular = line.getPerpendicularAt(this);
        Point intersection = line.getIntersectionPointWith(perpendicular);
        double deltaX = intersection.x - this.x;
        double deltaY = intersection.y - this.y;
        return intersection.translate(deltaX, deltaY);
    }

    /**
     * Returns {@code true} if the invoking instance and the argument
     * represent the same {@code Point}.
     * <p>
     * Due to the use of the floating-point data-type - {@code double}, there
     * are some inherent rounding errors. Therefore, the co-ordinates are
     * compared within a tolerance limit to ensure proper functioning.
     *
     * @param other The other {@code Object} to compare with.
     * @return {@code true} if the invoking instance and the argument
     * represent the same {@code Point}.
     */
    @Override
    public boolean equals(Object other) {
        if (other instanceof Point) {
            Point term = (Point) other;
            return this == term ||
                    Numbers.areEqual(this.x, term.x) &&
                            Numbers.areEqual(this.y, term.y);
        }
        return false;
    }

    /**
     * Returns the hash code for {@code this Point}.
     *
     * @return The hash code for {@code this Point}.
     */
    @Override
    public int hashCode() {
        return Arrays.hashCode(new double[]{x, y, super.hashCode()});
    }

    /**
     * Returns the String representation of {@code this Point}.
     *
     * @return The String representation of {@code this Point}.
     */
    @Override
    public String toString() {
        return "(" + Out.format(x) + "," + Out.format(y) + ")";
    }
}

The Points is for Point as Paths is for Path:

package library.geometry;

import com.sun.istack.internal.NotNull;

/**
 * This class has all the static methods that are necessary for creation and
 * manipulation of instances of the class {@code Point}.
 *
 * @author Subhomoy Haldar
 * @version 1.0
 */
public class Points {

    /**
     * This static variable defines the origin (i.e. the point (0, 0)).
     */
    public static final Point ORIGIN = Point.Factory.cartesianForm(0, 0);

    /**
     * Returns {@code true} if the {@code Point}s passed as arguments are
     * collinear. This means that there is a single line possible that
     * passes through all the {@code Point}s.
     * <p>
     *
     * @param points The {@code Point}s to check for collinearity.
     * @return {@code true} if the {@code Point}s passed as arguments are
     * collinear.
     */
    public static boolean areCollinear(@NotNull Point... points) {
        int length = points.length;
        if (length < 3) {
            throw new IllegalArgumentException("There must at least 3 Points.");
        }
        Line line = Line.Factory.twoPointForm(points[0], points[1]);
        for (int i = 2; i < length; i++) {
            if (!line.passesThrough(points[i])) {
                return false;
            }
        }
        return true;
    }
}

It doesn't boast much. It will evolve as time goes.

The tests are written rather hastily:

package library.geometry;

import org.junit.Test;
import library.util.Numbers;


import static org.junit.Assert.*;

/**
 * @author Subhomoy Haldar
 * @version 1.0
 */
public class PointTest {

    @Test
    public void testGetCartesian() throws Exception {
        Point point1 = Point.Factory.cartesianForm(3, 4);
        Point point2 = Point.Factory.polarForm(5, Math.atan2(4.0, 3.0));
        assertEquals(point1, point2);
    }

    @Test
    public void testEquality1() throws Exception {
        Point point1 = Points.ORIGIN;
        Point point2 = Point.Factory.cartesianForm(2, 2).translate(-2, -2);
        assertEquals(point1, point2);
    }

    @Test
    public void testDistanceFrom() throws Exception {
        Point origin = Points.ORIGIN;
        Point point1 = Point.Factory.cartesianForm(3, 4);
        double distance = origin.getDistanceFrom(point1);
        assertTrue(Numbers.areEqual(distance, 5));
    }

    @Test
    public void testRotation() throws Exception {
        Point point1 = Point.Factory.polarForm(5, Math.toRadians(30));
        Point point2 = Point.Factory.polarForm(5, Math.toRadians(60));
        assertEquals(point2.rotateAxesBy(Math.toRadians(30)), point1);
    }

    @Test
    public void testCollinear() throws Exception {
        Point point1 = Point.Factory.cartesianForm(0, 0);
        Point point2 = Point.Factory.cartesianForm(3, 4);
        Point point3 = Point.Factory.polarForm(5, Math.atan2(4, 3));
        assertTrue(Points.areCollinear(point1, point2, point3));
    }

    @Test
    public void testReflection() throws Exception {
        Point point = Point.Factory.cartesianForm(4, 4);
        Line line1 = Line.Factory.generalForm(1, 1, 0);
        Line line2 = Line.Factory.generalForm(1, 0, -2);
        assertTrue(point.getReflectionFrom(line1)
                .equals(Point.Factory.cartesianForm(-4, -4)));
        assertTrue(point.getReflectionFrom(line2)
                .equals(Point.Factory.cartesianForm(0, 4)));
    }
}

I uploaded this class for (complete) review as well as to explain the use case of Numbers. Hopefully, the code is better than what I wrote last time. More examples for test cases will be appreciated.

Answer 1

All Point instances are immutable

Make Point final to be truly immutable. Optionnaly (disclaimer: it's more my personnal belief) every method should be either abstract or final. Since you don't have any abstract methods, all the others should be marked as final.

---

All Point instances are unique

Not sure I correctly understand this statement. Does it mean it shouldn't be to instance of Point with the same coordinates stored in memory at the same time ?

---

public double getDistanceFrom(@NotNull final Point other) {
    ...
}

You could mark all the arguments given by reference as final to indicate to the caller the reference won't be changed in the method.

---

@NotNull
public Point copy() {
    return new Point(x, y);
}

This method is useless since Point is immutable. If the caller want to "duplicate" a Point, he could just duplicate the reference: Point p2 = p1; If you really want to provide a copy() method (maybe because the potential user of your library is not familiar with immutability), implements it as follow:

@NotNull
public Point copy() {
    return this;
}

---

As rolfl said in his post, you need to remove super.hashCode() from you hashCode() method.

Plus, your equals method relies on your Numbers library which holds a global state (the tolerance) that can be changed arbitrarly by the user.

Therefore, calling p1.equals(p2) twice at different moments could either return true or false !! See my first comment on this answer on how to avoid this global state... and yes I know it means for you dropping the use of varargs (replacing by an array), or renaming some methods or refactoring equals in a way it doesn't use Numbers anymore.

Example:

Point p1 = new Point(1, 2);
Point p2 = new Point(1.1, 2);
System.out.println(p1.equals(p2)); //prints false
Numbers.setDoubleTolerance(1);
System.out.println(p1.equals(p2)); //prints true

---

You must break the Law of Demeter to create an instance of Point. Why not using 2 static methods in Point (instead of an inner class) ?

public static Point fromCartesian(double x, double y) {
    return new Point(x, y);
}

public static Point fromPolar(double r, double theta) {
    return new Point(r * Math.cos(theta), r * Math.sin(theta));
}

Answer 2

This class breaks the contract of Equals and HashCode.... when two objects compare as .equals(...) to each other, then their hashCode() values should be the same.

Since you have tolerances on the equals method that are not consistently applied, you could have many technically different points all being equals, but they may all have different hashCode results.

Instead of forcing your equals(...) method to have a constant, or predefined tolerance, you should instead have a parameter you can pass in which allow a custom method that is independent of the HashCode/Equals contract.

In addition, your hashCode() method is broken too:

public int hashCode() {
    return Arrays.hashCode(new double[]{x, y, super.hashCode()});
}

You add the Object.hashCode() result in to the mix too. Since that method produces almost certainly unique values, even if you create two 'identical' points, say, fromCartesian(0,0) and fromCartesian(0,0), they will Still be different hashCodes even though they are at the exact same cartesian coordinate. Drop the super.hashCode();