# A Library Class : Point

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;

/**
* 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);
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.angleD = 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.
*/
Point p = new Point();
initPolar(p);
return p;
}

/*
* This method is common to both the 'getPolar_______Point()' methods.
*/
private static void initPolar(Point point) {
}

/**
* 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.

/*
* 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) {
: 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}.
*/
}

/**
* 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.
*/
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
*/
if (location == null) {
if (this.equals(Point.ORIGIN)) {
} else if (x == 0) {
} else if (y == 0) {
} else if (x > 0 && y > 0) {
} else if (x < 0 && y > 0) {
} else if (x < 0 && y < 0) {
} else if (x > 0 && y < 0) {
}
}
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() {
String summary = "\tCartesian:\t( x\t: " + x + ", y\t: " + y + " )";
if (state == null) {
setState(State.SHORT_SUMMARY);
}
if (!state.equals(State.NO_SUMMARY)) {
+ angleD + " )\n";
+ 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 :
* <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 :
* </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")

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.

## 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.

• The hashing could be more effective but it's actually fine. The equals depending on hashCode is very bad. We would not expect (1, 0) and (0, 1) to be considered equal and right now they are. – Radiodef Apr 3 '14 at 22:30
• Why bother special-casing the origin in reflectionFrom? – user2357112 supports Monica Apr 4 '14 at 17:14
• Why is it mutable? – Axel Apr 4 '14 at 18:59
• @user2357112 As I've commented below ChrisW's answer: "Using this.equals(ORIGIN) helps save some time coz, the dist. b/w this Point and Point.ORIGIN is already stored --> radius". But then again, I don't know if it's practical. – Hungry Blue Dev Apr 5 '14 at 4:34
• @Alex I didn't want it to be, but I had to make it mutable to implement the factory methods. Now, I can make x, y, radius, etc. final thanks to all the generous people of CodeReview. :-) – Hungry Blue Dev Apr 5 '14 at 4:35

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.

• Thanks for the loophole about the angle... And could (please?) demonstrate a better way to hash the Points? – Hungry Blue Dev Apr 3 '14 at 14:45
• The hash is ok. Just don't use it in the equals() method. – RoToRa Apr 3 '14 at 14:46
• I have edited the question, so please take a look. – Hungry Blue Dev Apr 4 '14 at 8:07

## 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() {
}


## 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 (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 :-)

• As for #2, you are absolutely right: I will create a PointTracker class for the same. – Hungry Blue Dev Apr 3 '14 at 14:44
• @ambigram_maker That's good, but I honestly don't understand why you need a PointTracker; is it really necessary? – coredump Apr 3 '14 at 15:24
• I have edited the question, so please take a look. – Hungry Blue Dev Apr 4 '14 at 8:08
• @ambigram_maker Let's try another approach: in the standard library, I see no FileTracker, DoubleTracker nor InputStreamTracker (there is a MediaTracker class, though). If anyone wants to track points in their applications, they will do it. But you can't do this in a generic way: will you use spatial hashing? or simply lists? you can't know what is good for everyone; I'am afraid you are over-architecting this part of the library. I'd suggest writing applications to test its API: try a bottom-up approach instead of a top-down one. – coredump Apr 4 '14 at 8:37
• Very nice answer! Feel free to join us in our chat room sometime! – syb0rg Apr 10 '14 at 22:27

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).

• I'm not familiar with atan2 or anything in math, but why would it be the correct use ? Should you call the Java version with atan2(y, x) or not ? – Marc-Andre Apr 3 '14 at 18:32
• @Marc-Andre Java's atan2 wants y, x: but the OP wrote x, y which therefore seems to be a bug. – ChrisW Apr 3 '14 at 18:43
• I have edited the question, so please take a look. – Hungry Blue Dev Apr 4 '14 at 8:09

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.
• My package is incomplete, just like your review. (-> JOKE: No Offence!) :-) I have yet to implement my PointTracker (see the body of my constructor (but that's another post). – Hungry Blue Dev Apr 3 '14 at 11:48
• I'm prefering factory methods to the constructor because of the ambiguity issue: read (or try to :-) )the documentation just above the main class for justification. – Hungry Blue Dev Apr 3 '14 at 11:49
• I'm intentionally using double for x and y, instead of int (that's why I need some hint regarding hashing and equality). – Hungry Blue Dev Apr 3 '14 at 11:52
• Using this.equals(ORIGIN) helps save some time coz, the dist. b/w this Point and Point.ORIGIN is already stored --> radius. – Hungry Blue Dev Apr 3 '14 at 11:54
• @ambigram_maker I understand why you have factory methods (though it might be better to have different classes, one for Cartesian and one for Polar). If your members are final (which perhaps they could/should be) then I think you initialize them in one place e.g. in a constructor. Having a non-trivial constructor is not incompatible with having factory methods: you can have a constructor which takes all 5 parameters, and calculate those 5 parameter values in the factory method. – ChrisW Apr 3 '14 at 12:13

One line review

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

• I have edited the question, so please take a look. – Hungry Blue Dev Apr 4 '14 at 8:10
• @ambigram_maker The point is that you SHOULDN'T. – user11153 Apr 4 '14 at 11:49
• @ambigram_maker ehhh what are you saying? you have done it. See your code. – Anirban Nag 'tintinmj' Apr 4 '14 at 11:51
• :-O. Man... I've gone crazy! – Hungry Blue Dev Apr 4 '14 at 12:10

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.

• I have edited the question, so please take a look. – Hungry Blue Dev Apr 4 '14 at 8:10
• Your edit doesn't address the biggest issue, which is whether a request to construct a point with (angle=45deg; radius=2.0) should yield a point with exactly that angle and radius, or whether it should yield the nearest point whose x and y coordinates are representable. As for PointTracker, I would suggest that Point itself should handle caching (with the assistance of one or more other classes), but allow allow the factory methods' callers to indicate when caching is particularly likely to be helpful. With regard to caching of computations, I would suggest... – supercat Apr 4 '14 at 15:08
• ...that it might be useful to have your Point hold an X and Y, and a reference to a PointInfo object which could have additional information, including possibly a sequence number and the identity of another lower-sequenced PointInfo which is known to be equal. Using such an approach would mean that if two points are compared and found to be equal, the result of any computation upon either (whether performed before or after the comparison) would be cached for both. Further, if A has found equal to B and B to C, then computations on A would be cached for C. – supercat Apr 4 '14 at 15:13
• There are some wrinkles one needs to be careful of in such a design to avoid having things go bonkers if points are accessed in different threads, but it could still be an interesting approach. – supercat Apr 4 '14 at 15:14
• Well... as for the accuracy issue: " I'm thinking of adding a scale variable" as I've said in the edit. This ought to take care of minor adjustments (like rounding) but has yet to be implemented. – Hungry Blue Dev Apr 5 '14 at 4:32