Is it ugly design that I overload a method depending on whether the argument is subtype or supertype?

Question

I intend to implent a super class A and a subclass B, and the objects can compare with each other.

compareTo is overloaded twice both in class A and class B, and the code seems a little messy. It feels like ugly design. I'm not sure if there is a more elegant approach.

class A implements Comparable<A> {
    private Integer x;

    public A(Integer i) {
        x = i;
    }

    public Integer getX() {
        return x;
    }

    public int compareTo(A other) {
        return x.compareTo(other.getX());
    }

    public int compareTo(B other) {
        return x.compareTo(other.getX() + other.getY());
    }
}

class B extends A {
    private Integer y;

    public B(Integer a, Integer b) {
        super(a);
        y = b;
    }

    public Integer getY() {
        return y;
    }

    @Override
    public int compareTo(A other) {
        return Integer.compare(this.getX() + y, other.getX());
    }

    @Override
    public int compareTo(B other) {
        return Integer.compare(this.getX() + y, other.getX() + other.getY());
    }
}

OldCurmudgeon · Accepted Answer · 2017-12-08T16:39:01.087

Yes it is a bad design. Something like this would be better.

class A implements Comparable<A> {
    private Integer x;

    public A(Integer i) {
        x = i;
    }

    public Integer getX() {
        return getX();
    }

    protected Integer compareValue() {
        return getX();
    }

    @Override
    public int compareTo(A other) {
        return compareValue().compareTo(other.compareValue());
    }
}

class B extends A {
    Integer y;

    public B(Integer a, Integer b) {
        super(a);
        y = b;
    }

    public Integer getY() {
        return y;
    }

    @Override
    protected Integer compareValue() {
        return getX() + getY();
    }
}

score 2 · Answer 2 · answered Dec 08 '17 at 15:50

It's bad design, yes. Specifically, it violates transitivity. From the Comparable Javadocs:

The implementor must also ensure that the relation is transitive: (x.compareTo(y)>0 && y.compareTo(z)>0) implies x.compareTo(z)>0.

When you introduce inheritance, you lose transitivity. That's because your A.compareTo(B) method doesn't actually override anything, and so if you passed a List<A> to Collections.sort() which contained both A and B, it would end up using the A.compareTo(A) method for the A instances and the B.compareTo(A) method for the B instances. As you've written them, they are not transitive, and therefore would result in unpredictable ordering since reversing the comparison can change the sort order.

I wrote a similar response w.r.t. Object.equals and why you can't overload it a while back, which you can find here.

Thanks. That's very helpful. May I know how to re-structure the code to improve? Thank you — CodingNow, Dec 08 '17 at 16:01
OldCurmudgeon's restructure would work as long as none of the subclasses override your `compareTo` method to do something different. You could enforce that by making the `compareTo` method `final`. — Brian, Dec 08 '17 at 16:04

Is it ugly design that I overload a method depending on whether the argument is subtype or supertype?

2 Answers2