Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have class point that represents point in 2D and I want to sort these points once based on their x-coordinate and second based on their y coordinate. Since x and y are double values my code looks like this:

public static class Point{
    int id;
    public double x;
    public double y;
    Point(int id, double x, double y){
        this.id = id;
        this.x = x;
        this.y = y;
    }
    public static Comparator<Point> xComparator = new Comparator<Point>(){
        @Override
        public int compare(Point t1, Point t2) {
            // TODO Auto-generated method stub
            if (t1.x > t2.x){
                return 1;
            }
            if (t1.x < t2.x){
                return -1;
            }
            else{
                return 0;
            }
        }
    };
    public static Comparator<Point> yComparator = new Comparator<Point>(){
            public int compare(Point t1, Point t2) {
                // TODO Auto-generated method stub
                if (t1.x > t2.x){
                    return 1;
                }
                if (t1.x < t2.x){
                    return -1;
                }
                else{
                    return 0;
                }
            }
        };
}

Is this a good way for defining Comparator? Is there better solution for sorting based on x and on y coordinate? Is this the only way to deal with compare method based on two double values?

share|improve this question
3  
How do you call this method? How do you actually perform the compare, because, as it stands you can only sort with either the x or the y point, but not both. –  rolfl Jul 5 at 13:59
add comment

3 Answers 3

Your question does not show how you call/use the Comparators you have defined. Despite that, there are a number of issues I see with your code, and feel I should point out....

When you have data that is sorted, the data tends to last around for a while, and sorted-data is often a very important assumption for complex processing (i.e. if the data was not sorted correctly the computations will fail).

Whenever you have sorted data, and Comparable data, the data should almost always be immutable (cannot possibly change). If the data can change after the sort happens, then the sorted data is no longer sorted.

So, your Point class should probably be an immutable class. At the moment, it has a number of problems with that:

  1. The member fields should be final.
  2. There should be 'getters' for the fields, and the fields themselves should be private (not public like you have).

Your class should look more like:

public static final class Point{
    private final int id;
    private final double x;
    private final double y;

    Point(int id, double x, double y){
        this.id = id;
        this.x = x;
        this.y = y;
    }

    public int getID() {
        return id;
    }

    public double getX() {
        return x;
    }

    public double getY() {
        return y;
    }

    .....

ahhh, I see skiwi just answered a bunch... I'll cover some other stuff from here on....

Reiterating his comments:

  • the equals and hashCode method he suggests are very important (I was about to add that too).

Right, when you have value-based items like this, it is often convenient to make the Point have a 'natural' ordering. This means that the Point class should implement Comparable<Point>, and then implement the compareTo(Point other) method.

Having two comparators is a problem, you only want one, that does both coordinates.

Consider a compareTo() method like:

public static class Point implements Comparable<Point> {

    ....

    public int compareTo(final Point other) {

        if (other == null) {
            // non null always comes after null
            return 1;
        }

        final int xcomp = Double.compare(this.x, other.x);

        return xcomp == 0 ? Double.compare(this.y, other.y) : xcomp;
    }

This will be one comparator that does both axes.

Then, you can use this Point as a natural sort order, and use it as a key in a TreeMap, and or things like Arrays.sort(pointarray), etc.

Note, I reused the comparison function Double.compare() to do the heavy lifting of the compare.

share|improve this answer
    
+1, but IMHO it's ok to have public fields in cases like this one (x and y of the Point class as long as they are final and the class is immutable). –  Axel Jul 6 at 13:59
add comment
  1. You could change

        @Override
        public int compare(Point t1, Point t2) {
            // TODO Auto-generated method stub
            if (t1.x > t2.x){
                return 1;
            }
            if (t1.x < t2.x){
                return -1;
            }
            else{
                return 0;
            }
        }
    

    to

    @Override
    public int compare(Point t1, Point t2) {
        return Double.compare(t1.x, t2.x);
    }
    

    (It's supported by Java 7 too.)

  2. The xComparator and yComparator fields are not final. Consider the following:

    Point.xComparator = new Comparator<Point>() {
    
        @Override
        public int compare(Point o1, Point o2) {
            // something evil
            return 1;
        }
    };
    
  3. For this:

    // TODO Auto-generated method stub
    

    If you have something to do then do it, otherwise remove the comment. TODO comments does not suggest professional work since it looks unfinished. Currently it's just noise. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

share|improve this answer
add comment

(First of all: Your code does not compile, unless I take the assumption that Point is an inner class)

This seems like an almost proper way to handle it.
The improvement I can think about though is to provide a getXComparator() method, and hence hiding implementation details.

There are however some other concerns I have about your code:

  • General readability: You are ought to leave a blank between ) and {, like if (cond) {.
  • Your attributes are not final. I think they should be as the best design would be to have your class immutable.
  • Your attributes are public, this is a bad design as you always want to have them private and provide the appropiate getter-methods for it.

If you are looking for an all time improvement, then consider using Java 8. Even if this part of the answer doesn't suit you, it could help others:

public Comparator<Point> getXComparator() {
    return Comparator.comparingDouble(Point::getX);
}

public Comparator<Point> getYComparator() {
    return Comparator.comparingDouble(Point::getY);
}

This is as simple as it can be, it returns a Comparator<Point> that compares the x-values of your points. I use a method reference Point::getX here, which can be written as the lambda point -> point.getX(). It essentially extracts the x-attribute out of every point, and compares on that attribute.

Your full class, I would write like this, also note the inclusion of equals and hashcode (which are autogenerated by the IDE):

private static class Point {
    private final int id;
    private final double x;
    private final double y;

    Point(final int id, final double x, double y) {
        this.id = id;
        this.x = x;
        this.y = y;
    }

    public int getId() {
        return id;
    }

    public double getX() {
        return x;
    }

    public double getY() {
        return y;
    }

    public Comparator<Point> getXComparator() {
        return Comparator.comparing(Point::getX);
    }

    public Comparator<Point> getYComparator() {
        return Comparator.comparing(Point::getY);
    }

    @Override
    public int hashCode() {
        int hash = 3;
        hash = 29 * hash + this.id;
        hash = 29 * hash + (int) (Double.doubleToLongBits(this.x) ^ (Double.doubleToLongBits(this.x) >>> 32));
        hash = 29 * hash + (int) (Double.doubleToLongBits(this.y) ^ (Double.doubleToLongBits(this.y) >>> 32));
        return hash;
    }

    @Override
    public boolean equals(Object obj) {
        if (obj == null) {
            return false;
        }
        if (getClass() != obj.getClass()) {
            return false;
        }
        final Point other = (Point) obj;
        if (this.id != other.id) {
            return false;
        }
        if (Double.doubleToLongBits(this.x) != Double.doubleToLongBits(other.x)) {
            return false;
        }
        if (Double.doubleToLongBits(this.y) != Double.doubleToLongBits(other.y)) {
            return false;
        }
        return true;
    }
}
share|improve this answer
    
And of course the missing piece of the puzzle, Comparator.comparing(Point::getX).thenComparing(Point::getY). –  Trejkaz Jul 6 at 10:07
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.