A Library Class : Point

Question

I am trying to create a library of my own which contains (among others), a class called Point. As the name suggests, it is intended to encapsulate a point represented in 2D space. This is what I've come up with:

package geom;

import static java.lang.Math.atan2;
import static java.lang.Math.cos;
import static java.lang.Math.pow;
import static java.lang.Math.sin;
import static java.lang.Math.sqrt;
import static java.lang.Math.toDegrees;
import static java.lang.Math.toRadians;

/**
 * A class that provides factory methods for creation of {@code Points}, utility
 * functions based on operations regarding points.
 * <p>
 * The constructor has been made {@code private} because of the following
 * reason: A {@code Point} can be initialized in <i>two</i> different ways:
 * <ul>
 * <li>Using the <b>Cartesian System</b> of using two variables to represent the
 * coordinates of the {@code Point}.</li>
 * <li>Using the <b>Polar System</b> of using the distance from the original and
 * the angle between the line joining the point and the origin and the X-axis to
 * represent the location of the {@code Point}.</li>
 * </ul>
 * Since there are <i>two</i> parameters in both cases, and each is of the type
 * {@code double}, an ambiguity arises.
 * <p>
 * A solution to the above problem is to use <em>Factory Methods</em>.<br>
 * Factory methods are {@code public static} methods which can accessed through
 * class reference, like:
 * <p>
 * <code>Point point = Point.getCartesian(10, 20);</code><p>
 * The advantages of using Factory Methods are many; a few of them are:
 * <ol><li>Through the use of Factory Methods with descriptive names, the
 * ambiguity is removed.</li>
 * <li>These methods return a {@code new Point} if it has not been created
 * before.<br>
 * If a pre-existing point exists, the {@code Point} is returned from the
 * {@code PointTracker}.</li>
 * </ol>
 *
 *
 * @author ambigram_maker
 * @version 1.0
 * @version 2014-04-02
 */
public class Point {

    /*
     * This variable represents the state of the output.
     * It is static because any change to this variable is reflected in all the 
     * instances.
     */
    private static State state;
    /**
     * The Point object representing the reference point at the intersection of
     * the coordinate axes. Essentially, it is a point whose coordinates are
     * <pre>(0,0)</pre>.
     */
    public static final Point ORIGIN = getCartesianPoint(0, 0);

    /**
     * This factory method returns a {@code Point} object with the (Cartesian)
     * coordinates passed as parameters.
     *
     * @param x The X-coordinate of the {@code Point} object.
     * @param y The Y-coordinate of the {@code Point} object.
     * @return The required {@code Point} object.
     */
    public static Point getCartesianPoint(double x, double y) {
        Point p = new Point();
        p.x = x;
        p.y = y;
        p.radius = sqrt(x * x + y * y);
        p.angleR = atan2(x, y);
        p.angleD = toDegrees(p.getAngleRadians());
        return p;
    }

    /**
     * This factory method returns a {@code Point} object with the (Polar)
     * coordinates passed as the parameters.
     *
     * @param radius The distance of the required {@code Point} from the origin
     * (0,0)
     * @param degrees The angle between the line joining the required
     * {@code Point} and the origin and the X-axis (in degrees i.e. from 0 to
     * 360).
     * @return The required {@code Point} object.
     */
    public static Point getPolarDegreesPoint(double radius, double degrees) {
        Point p = new Point();
        p.radius = radius;
        p.angleD = degrees;
        p.angleR = toRadians(degrees);
        initPolar(p);
        return p;
    }

    /**
     * This factory method returns a {@code Point} object with the (Polar)
     * coordinates passed as the parameters.
     *
     * @param radius The distance of the required {@code Point} from the origin
     * (0,0)
     * @param radians The angle between the line joining the required
     * {@code Point} and the origin and the X-axis (in radians i.e. from 0 to
     * 360).
     * @return The required {@code Point} object.
     */
    public static Point getPolarRadiansPoint(double radius, double radians) {
        Point p = new Point();
        p.radius = radius;
        p.angleR = radians;
        p.angleD = toDegrees(radians);
        initPolar(p);
        return p;
    }

    /*
     * This method is common to both the 'getPolar_______Point()' methods.
     */
    private static void initPolar(Point point) {
        double angle = point.getAngleRadians();
        point.x = point.getRadius() * cos(angle);
        point.y = point.getRadius() * sin(angle);
    }

    /**
     * This method is used to change the form of representation of <em>ALL</em>
     * {@code Point} objects.
     *
     * @see State
     * @param state The {@code State} constant to set.
     */
    public static void setState(State state) {
        Point.state = state;
    }

    /*
     * The coordinates of this Point in the Cartesian system.
     */
    private double x;       // The perpendicular distance from the Y-axis.
    private double y;       // The perpendicular distance from the X-axis.

    /*
     * The coordinates of this Point in the Polar System.
     */
    private double radius;  // The distance from the Origin (0,0).
    private double angleR;  // The angle in Radians.
    private double angleD;  // The angle in Degrees.

    private Quadrant location;
    /*
     * Instances of Point are not meant to be created through the default
     * contructor. Use the Factory Methods instead.
     */

    private Point() {
        // Provision to add itself to the PointTracker.
    }

    /**
     * Returns the <i>distance</i> between {@code this Point} and the one passed
     * in the parameter.
     *
     * @param other The <i>other</i> {@code Point} which acts as the reference
     * for calculating the distance.
     * @return The distance {@code this Point} and the <i>other</i> one.
     */
    public double distanceFrom(Point other) {
        return other.equals(Point.ORIGIN) ? radius
                : sqrt(pow(this.getX() - other.getX(), 2)
                        + pow(this.getY() - other.getY(), 2));
    }

    /**
     * This method returns the {@code Point} which is a reflection of
     * {@code this Point} in the X-axis.
     *
     * @return Returns the {@code Point} which is a reflection of
     * {@code this Point} in the X-axis.
     */
    public Point reflectionXAxis() {
        return getCartesianPoint(getX(), -getY());
    }

    /**
     * This method returns the {@code Point} which is a reflection of
     * {@code this Point} in the Y-axis.
     *
     * @return Returns the {@code Point} which is a reflection of
     * {@code this Point} in the Y-axis.
     */
    public Point reflectionYAxis() {
        return getCartesianPoint(-getX(), getY());
    }

    /**
     * This method returns the {@code Point} which is a reflection of
     * {@code this Point} in the Origin.
     *
     * @return Returns the {@code Point} which is a reflection of
     * {@code this Point} in the Origin.
     */
    public Point reflectionOrigin() {
        return getCartesianPoint(-getX(), -getY());
    }

    /**
     * This method returns the {@code Point} which is a reflection of
     * {@code this Point} in the {@code Point} passed as a parameter.
     *
     * @param other The reference for calculating the reflection of
     * {@code this Point}
     * @return Returns the {@code Point} which is a reflection of
     * {@code this Point} in the X-axis.
     */
    public Point reflectionFrom(Point other) {
        if (other.equals(Point.ORIGIN)) {
            return reflectionOrigin();
        }
        return getCartesianPoint(
                2 * other.getX() - this.getX(),
                2 * other.getY() - this.getY());

    }

    /**
     * Returns the X-coordinate of {@code this Point}.
     * <p>
     * The magnitude of the X-coordinate is the perpendicular distance between
     * {@code this Point} and the Y-axis. If {@code this Point} is above the
     * X-axis, the X-coordinate is positive. If {@code this Point} is below the
     * X-axis, the X-coordinate is negative.
     *
     * @return the X coordinate of {@code this Point}.
     */
    public double getX() {
        return x;
    }

    /**
     * Returns the Y-coordinate of {@code this Point}.
     * <p>
     * The magnitude of the Y-coordinate is the perpendicular distance between
     * {@code this Point} and the X-axis. If {@code this Point} is above the
     * Y-axis, the Y-coordinate is positive. If {@code this Point} is below the
     * Y-axis, the Y-coordinate is negative.
     *
     * @return the Y coordinate of {@code this Point}.
     */
    public double getY() {
        return y;
    }

    /**
     * Returns the distance between the origin (0,0) and {@code this Point}.
     * Considering the origin to be at the center and {@code this Point} at the
     * circumference, this distance is the <i>radius</i>.
     *
     * @return The Distance between the origin and {@code this Point}.
     */
    public double getRadius() {
        return radius;
    }

    /**
     * Returns the angle between the line joining {@code this Point} and the
     * origin, and the X-axis in Radians.
     *
     * @return The angle between the line joining {@code this Point} and the
     * origin, and the X-axis in Radians.
     */
    public double getAngleRadians() {
        return angleR;
    }

    /**
     * Returns the angle between the line joining {@code this Point} and the
     * origin, and the X-axis in Degrees.
     *
     * @return The angle between the line joining {@code this Point} and the
     * origin, and the X-axis in Degrees.
     */
    public double getAngleDegrees() {
        return angleD;
    }

    /**
     * Returns the <i>location</i> of {@code this Point} in a broader
     *
     * @return
     */
    public Quadrant getLocation() {
        if (location == null) {
            if (this.equals(Point.ORIGIN)) {
                location = Quadrant.ON_ORIGIN;
            } else if (x == 0) {
                location = Quadrant.ON_Y_AXIS;
            } else if (y == 0) {
                location = Quadrant.ON_X_AXIS;
            } else if (x > 0 && y > 0) {
                location = Quadrant.FIRST_QUADRANT;
            } else if (x < 0 && y > 0) {
                location = Quadrant.SECOND_QUADRANT;
            } else if (x < 0 && y < 0) {
                location = Quadrant.THIRD_QUADRANT;
            } else if (x > 0 && y < 0) {
                location = Quadrant.FOURTH_QUADRANT;
            }
        }
        return location;
    }

    /**
     * This method is used to check if two instances of {@code Point} are equal.
     * This method checks the {@code Point}s using their hashcodes.
     *
     * @see Point#hashCode()
     * @param o The {@code Object} to compare this instance with.
     * @return {@code true} if the {@code  Object} passed as parameter is an
     * instance of type {@code Point} and the two {@code Point}s are
     * <i>equal</i>
     */
    @Override
    public boolean equals(Object o) {
        if (o instanceof Point) {
            Point p = (Point) o;
            return this.hashCode() == p.hashCode();
        }
        return false;
    }

    @Override
    public int hashCode() {
        int hash = 0;
        hash += (int) (Double.doubleToLongBits(this.getX())
                ^ (Double.doubleToLongBits(this.getX()) >>> 32));
        hash += (int) (Double.doubleToLongBits(this.getY())
                ^ (Double.doubleToLongBits(this.getY()) >>> 32));
        return hash;
    }

    @Override
    public String toString() {
        Thread t = new Thread();
        String summary = "\tCartesian:\t( x\t: " + x + ", y\t: " + y + " )";
        if (state == null) {
            setState(State.SHORT_SUMMARY);
        }
        if (!state.equals(State.NO_SUMMARY)) {
            summary += "\n\tPolar:\n\t\tDegrees\t( radius\t: " + radius + ", angle\t: "
                    + angleD + " )\n";
            summary += "\t\tRadians\t( radius\t: " + radius + ", angle\t: "
                    + angleR + " )\n";
        }
        if (state.equals(State.LONG_SUMMARY)) {
            summary += "\tQuadrant\t: " + getLocation();
            // summary += "\n\t" + Integer.toHexString(hashCode());
        }
        return summary;
    }

    /**
     *
     */
    @SuppressWarnings("PublicInnerClass")
    public static enum State {

        /**
         * If the {@code state} of a {@code Point} is set to this value, then
         * the {@code toString()} will display:
         * <ol>
         * <li>Cartesian Representation : (x,y)</li>
         * <li>Polar Representation (r,θ) in :
         * <ul><li>Degrees</li><li>Radians</li></ul></li>
         * <li>The quadrant in which the {@code Point} is located.</li>
         * </ol>
         */
        LONG_SUMMARY,
        /**
         * If the {@code state} of a {@code Point} is set to this value, then
         * the {@code toString()} will display:
         * <ol>
         * <li>Cartesian Representation : (x,y)</li>
         * <li>Polar Representation (r,θ) in :
         * <ul><li>Degrees</li><li>Radians</li></ul></li>
         * </ol>
         *
         */
        SHORT_SUMMARY,
        /**
         * If the {@code state} of a {@code Point} is set to this value, then
         * the {@code toString()} will display the Cartesian Representation :
         * (x,y) of this {@code Point}.
         */
        NO_SUMMARY;
    }

    @SuppressWarnings("PublicInnerClass")
    public static enum Quadrant {

        FIRST_QUADRANT,
        SECOND_QUADRANT,
        THIRD_QUADRANT,
        FOURTH_QUADRANT,
        ON_X_AXIS,
        ON_Y_AXIS,
        ON_ORIGIN;
    }
}

---

I used NetBeans 8.0 to create the above class, so the arrangement, the warning suppression has been suggested by this software. I need people to criticize and discuss upon:

1. The effectiveness of the code.
2. The organization of the code.
3. Possible ways to improve the performance, readability, simplicity, etc.
4. Is the hashing good enough?
5. Any errors in the documentation (technical, accidental) of the code.
6. Any other aspect focused upon improvement of my programming skills.

through downvotes (and upvotes?¹), comments and answers (of course!). Please note that this is a library class that anyone can use or modify, but inform me first.

---

EDIT:

After [encountering] so many varied answers, I have decided to change these aspects:

1. As suggested by RoToRa in this answer, I'll get rid of the State idea completely, because it is actually temporary.
2. From the same answer, I'll change the name of the factory methods to start with create....
3. Fix the bug pertaining to the angle (from the same answer).
4. Modify the hashcode() and equals() method. (I have done it here).
5. As suggested by coredump in this answer, I'll change initPolar() (or maybe even get rid of it).
6. As suggested by Eike Schulte in this answer, I'll fix the atan2(y,x) bug.
7. As suggested by ChrisW over here, I'll use pow(..) throughout.
8. Use a 5-parameter constructor (for use by the factory methods).

(These changes haven't been applied to the code above.)

• In addition to these, I'm thinking of adding a scale variable (double) that is used to decide the degree of accuracy (and overcome the floating-point issues). Is it really practical?
• Justification for PointTracker:
  I'll add a post later on about the class PointTracker after some time. This class is (as the name suggests) supposed to track (all?) the Points created during runtime. It'll add support for yet another library class: Line. (Hence, you can expect quite a few posts related to this topic.)
• Last, but not the least I invite more answers pertaining to topics other than those resolved above, so that this Point is the ideal class. I also invite demonstrations of just-in-time implementations of the inter-conversion of the angles or other related operations.

Please guys, remember that this is for you; so give suggestions to make it more personal.

---

^{1 : Help CodeReview graduate!}

Answer 1

Use use of a "state" to distinguish between different output formats is very wrong. If you need different formats, use multiple output methods (have toString just for debug output), or even better, write a separate class for converting Points to a string representation.

I think you have overdone it with the JavaDoc. IMHO it's neither the place for theoretical excursus on how coordinate systems work, nor the place to discuss implementation decisions ("Since there are two parameters in both cases, and each is of the type {@code double}, an ambiguity arises.").

The names of the factory methods shouldn't begin with get... since they are not getters. Use create... instead.

You should modulo the degrees to 360 (and the radians to 2π) otherwise you get:

Point p1 = Point.getPolarDegreesPoint(10,45);
Point p2 = Point.getPolarDegreesPoint(10,405);
System.out.println(p1.equals(p2)); // prints false, but should be true.

The use of the hash code inside the equals not good. One of the functions of equals is to distinguish between two objects with the same hash code - which will happen, considering you are "squeezing" two 64 bit double values into a 32 bit integer.

Answer 2

1. Why static ?

I have a minor issue with the following:

private static void initPolar(Point point) {

Why do you define a static function when the argument is an instance of a Point and thus could be a method?

private void initPolar() {
    double angle = getAngleRadians();
    x = getRadius() * cos(angle);
    y = getRadius() * sin(angle);
}

2. Do you need caching ?

If a pre-existing point exists, the {@code Point} is returned from the {@code PointTracker}

This looks like premature optimization to me. What is the intended benefits of storing and looking up points in a cache (which takes time and memory) instead of creating multiple instances of points. How likely it is that points will be created at the exact same location?

How are you going to implement the tracker? using a hash map? will you create a temporary point just to check whether it already belongs to the set and discard it in order to return the stored instance?

Besides, if you really need caching, this is not the responsability of the Point class to do it. The class exposes a hash function so that other classes can use it.

3. Class hierarchy

It has already been suggested, but you can define two kind of points, "polar" and "cartesian" ones, both having well-defined public constructors:

 public CartesionPoint (double x, y) ...
 public CartesionPoint (PolarPoint pp) ...

 public PolarPoint (double radius, radians) ...
 public PolarPoint (CartesionPoint cp) ...

If you really want to build PolarPoint with either radians/degrees, maybe your constructor can take an optional unit parameter (e.g. RADIANS or DEGREES, defined in an enum type).

I hope I don't sound too harsh. Good luck :-)

Answer 3

The Math.atan2 function is defined rather strangely (not only in Java, but in every programming language I know) and the correct use is atan2(y, x).

Answer 4

This will be an incomplete review because I don't really know Java.

Instead of State maybe call it SummaryType or FormatType.

In distanceFrom I don't see why you have a special case for other.equals(Point.ORIGIN); and given that you do, I don't see why you don't also have a special case for this.equals(Point.ORIGIN).

Your implementation of equals might be buggy: if points are equal then their hash codes are equal; but different points might be able to have the same hash code.

You're carrying redundant/duplicate information in each instance: e.g. the angle in degrees and in radians. You could store only one and calculate the other just-in-time if it's asked for. Similarly you're pre-calculating and storing its cartesian and its polar representation: would it be better to store one and calculate the other just-in-time; or have two different classes (CartesianPoint and PolarPoint) for the two different representations?

Sometimes you use pow(..., 2) and sometimes you use x * x: is that deliberate?

Are there values (along an axis) for which atan2 doesn't work well because its behaviour is asymptotic?

Calculated points risk being never-equal due to rounding errors. It's usual with floating-point to arithmetic to decide they're 'equal' if they're "close enough". That could interfere with hashing though (if it's required that equal values must have equal hashes). So you might want to define another function "nearlyEqual" which returns true if distanceFrom is less than some small epsilon.

Because of rounding errors you may find anomalies e.g. that a point initialized using polar coordinates doesn't return the right getLocation() value. If you had two classes then the getLocation method for the polar class could do its test using the angle instead of using the x and y values.

Do you want to use double for your x and y types? It might be better (e.g. so that you don't have trouble comparing 1 with 1.0000000000000001) to use int instead, if that's appropriate for example if x and y are measures of integer pixel value.

I'd like to see a list of unit-tests, so that I know what values you have tested it for.

This comment is untrue (not implemented):

If a pre-existing point exists, the {@code Point} is returned from the {@code PointTracker}.

It might be better if the data members were final. You could implement this even when you have multiple factory methods, by:

• Having a constructor which takes all values via parameters
• Calculate all those parameter values in the static factory methods, and then call the constructor.

Answer 5

One line review

I don't see any use of Thread t = new Thread() in toString method.

Answer 6

If a class is supposed to serve as a data holder, two instances which report themselves as equal should have the same properties; further, values corresponding to constructor/factory parameters should match the values used in creating the object. It would be possible to have a polar-coordinate type whose properties were the rho and theta values, or an xy coordinate type, whose properties were x and y, or perhaps even an abstract coordinate type with subtypes that hold degree-polar coordinates, radian-polar coordinates, and xy coordinates (each of which would store a different set of properties). Defining how the latter should work, though, could be tricky.

Consider what happens, for example, if one constructs a point with polar coordinates (angle=45 degrees, radius=2.0) and then constructs another point whose xy coordinates match the first. Any possible behavior by the class is apt to be "surprising", since the xy-constructed object will have a radius of 2.0000000000000004; this means that either:

• The xy-constructed object won't be equal to another object whose coordinates are reportedly the same, or
• The xy-constructed object, though supposedly equal to the polar-constructed one, will have a different radius, or
• The radius of the polar-constructed object won't match the specified value.

Of these, perhaps the first would be most justifiable if one says that degree-polar objects are unequal to any xy object except when the angle is a multiple of 90 degrees, and radian-polar objects are unequal to any other object except when the angle is zero. Even though the closest double to the polar-degree object's x coordinate is 1.4142135623730951, the actual x coordinate is closer to 1.4142135623730950488016887242097, so it won't quite match any point whose xy coordinates are specified using a double.

Unless there's some particular reason for using polar coordinates, I'd suggest having the Point type explicitly encapsulate an x and y, both of type double. Use public final fields for those and set them in a private constructor. Even if you have other methods which can construct a point for a given radius and angle, or report the radius and angle associated with a point, make it clear that reported values are not inherent properties of the object, but simply calculations based upon its x and y.