2
\$\begingroup\$

I have made a Point2D.Double wrapper so I can add my own functionality (like a dominate method) and alter some (like toString) and control some (like not having public access to x and y).

Point class:

public class Point implements Serializable {

    private final Point2D.Double point2D;

    public Point(double x, double y) {
        this.point2D = new Point2D.Double(x, y);
    }

    public Point(String line, String delimiter) {
        line = line.trim();
        String[] lineArray = line.split(delimiter);
        double x = Double.parseDouble(lineArray[0]);
        double y = Double.parseDouble(lineArray[1]);
        this.point2D = new Point2D.Double(x, y);
    }

    public boolean dominates(Point point) {
        return (this.getX() <= point.getX() && this.getY() < point.getY()) 
                || (this.getY() <= point.getY() && this.getX() < point.getX());
    }

    public double getX() {
        return point2D.getX();
    }

    public double getY() {
        return point2D.getY();
    }

    @Override
    public boolean equals(Object o) {
        return point2D.equals(o);
    }

    @Override
    public int hashCode() {
        return point2D.hashCode();
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append('(').append(point2D.getX()).append(", ").append(point2D.getY()).append(')');
        return builder.toString();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Can you be a little more specific as to what you would like to get feedback about? Also why not extends Point2D.Double? \$\endgroup\$ – Nihathrael Nov 26 '14 at 10:14
  • \$\begingroup\$ @Nihathrael naming conventions, method separation, proper clean usage of equals and hashcode in case of wrapper, possible other ways I can do this that are cleaner. \$\endgroup\$ – Aki K Nov 26 '14 at 10:16
2
\$\begingroup\$

I took a look and came up with a couple of suggestions:

Extend rather than wrap

I think it will be more useful if you extend the Point2D.Double class, instead of writing a wrapper around it. This way you will be able to use your custom point class everywhere that a normal Point2D.Double is expected. It also spares you from implementing and overriding many of the Double's methods like getX() and the hashCode() and toString() methods.

Use static method for construction from line

When subclassing Double it is not possible to have a constructor that takes a line, does some calculations and then calls super(), as super has to be the first call in the constructor. Therefor we need to add a static utility class to create a Point from a line. See the fromLine() method below.

I think this is not a problem, as it leads to very readable line:

Point start = Point.fromLine(line);

Documentation

Your code does not include any documentation in form of comments, so that for example the input format for the fromLine() function is not entirely clear. Also it only uses one point from a line (which should be two points?), so maybe it should be called fromLineStart() or fromLineEnd() depending on which you mean.

Class Naming

The current name Point doesn't tell us a lot about what the class does. Maybe use something a little more descriptive like PointDouble2D? Maybe add some indication as that it does more than the normal Point2D.Double, by naming it: PointDouble2DAdvanced or something like that, not totally sure myself :)

Final Result

Here is the entire code example, without documentation comments:

import java.awt.geom.Point2D;

public class PointDouble2D extends Point2D.Double {

    public Point(double x, double y) {
        super(x, y);
    }

    public boolean dominates(Point2D.Double point) {
        return (getX() <= point.getX() && getY() < point.getY())
                || (getY() <= point.getY() && getX() < point.getX());
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append('(').append(getX()).append(", ").append(getY())
                .append(')');
        return builder.toString();
    }

    public static PointDouble2D fromLineStart(String line, String delimiter) {
        line = line.trim();
        String[] lineArray = line.split(delimiter);
        double x = java.lang.Double.parseDouble(lineArray[0]);
        double y = java.lang.Double.parseDouble(lineArray[1]);
        return new PointDouble2D(x, y);
    }
}

What do you think?

\$\endgroup\$
  • \$\begingroup\$ I too suggested to extend at codereview.stackexchange.com/a/70878/58906 but OP apparently didn't like it. Do you have another idea how we can convince him not re-inventing the Point2D.Double-wheel? \$\endgroup\$ – GeroldBroser reinstates Monica Nov 26 '14 at 13:14
  • 1
    \$\begingroup\$ In case he wants to limit the API, that could be a valid reason. But since all methods except for setLocation of the original Point2D.Double are implemented in the wrapper, I do not see any benefit of having a wrapper. Especially as extending the class makes the new PointDouble2D easier to use, uses less code and provides more flexibility of use for the PointDouble2D class. \$\endgroup\$ – Nihathrael Nov 26 '14 at 13:54
  • \$\begingroup\$ @GeroldBroser Point2D.Double gives public access to x and y. I don't want that, it can easily cause problems and it is completely unnecessary for what I am doing. It also is inconsistent with the style of my code therefore can easily cause future users/readers of my Point class to get confused or use it wrong. That is my only gripe, the x and y being public. \$\endgroup\$ – Aki K Nov 26 '14 at 15:16
  • 1
    \$\begingroup\$ @DimitrisK I agree that having the x and y public is not very nice. I don't think I'd go through the hassle of not extending Point2D.Double though, as this is a general Problem with the Point2D.Double. This is of course unless you are designing a public API (which is to be released and used as such [think: apache commons]), then using a wrapper is probably worth considering. But for "private" or "in project" use, I'd probably go with the simplistic approach of subclassing it. \$\endgroup\$ – Nihathrael Nov 27 '14 at 7:31
  • 1
    \$\begingroup\$ This is mostly for project use so I guess you are right and I should extend it rather than wrap it both in this case and my other question. \$\endgroup\$ – Aki K Nov 27 '14 at 15:27

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.