1

I have a base BusinessObject abstract class which implements Comparable by comparing their long id fields. Now imagine I extend it with, say, Person and then I extend Person with Worker. So we have:

BusinessObject < Person < Worker

So now I override the compareTo(BusinessObject) in business object in Person (compare the names) and Worker (compares the job name, then person name).

Now I do something like:

List<BusinessObject> collection = new ArrayList<>();
collection.add(new Worker(1L, "Steve", "janitor"));
collection.add(new Worker(2L, "Mark", "plumber"));
collection.add(new Person(3L, "Dave"));

Collections.sort(collection);
System.out.println(collection);

By logging I can see the calls that are made:

  1. Worker.compareTo()
  2. Person.compareTo()

So, it means sorting methods are mixed which is obviously not good. So what is the correct way to implement Comparable with inheritance so that the method called depends on the generic type of the collection:

  1. if collection is a List then always use BusinessObject.compareTo()
  2. if collection is a List then always use Person.compareTo()
  3. if collection is a List then always use Worker.compareTo()

Here's my code:

public abstract class BusinessObject implements HasId, HasText, Comparable<BusinessObject> {
    protected @Nullable Long id;

    public BusinessObject(@Nullable Long id) {
        this.id = id;
    }

    @Override
    public @Nullable Long getId() {
        return id;
    }

    public void setId(long id) {
        this.id = id;
    }

    @Override
    public int compareTo(@NonNull BusinessObject o) {
        System.out.println("compareTo BusinessObject");
        return Long.compare(id, o.id);
    }

    @Override
    public String toString() {
        return String.format("BusinessObject#%d", id);
    }
}

public class Person extends BusinessObject {
    protected final String name;

    public Person(@Nullable Long id, String name) {
        super(id);
        this.name = name;
    }

    @NonNull
    @Override
    public String getText(Context context) {
        return null;
    }

    @Override
    public String toString() {
        return String.format("Person#%d (%s)", id, name);
    }

    @Override
    public int compareTo(@NonNull BusinessObject o) {
        if (o instanceof Person) {
            System.out.println("compareTo Person");
            Person po = (Person) o;
            return name.compareTo(po.name);
        }
        else {
            return super.compareTo(o);
        }
    }
}

public class Worker extends Person {
    protected final String job;

    public Worker(@Nullable Long id, String name, String job) {
        super(id, name);
        this.job = job;
    }

    @Override
    public String toString() {
        return String.format("Worker#%d (%s-%s)", id, name, job);
    }

    @Override
    public int compareTo(@NonNull BusinessObject o) {
        if (o instanceof Worker) {
            System.out.println("compareTo Worker");
            Worker wo = (Worker) o;
            return String.format("%s%s", name, job).compareTo(String.format("%s%s", wo.name, wo.job));
        }
        else {
            return super.compareTo(o);
        }
    }
}
10
  • 1
    Please paste your Person and Worker code Commented Jun 12, 2017 at 9:15
  • 2
    The obvious way is not to mix two different types of objects in a single list. If you mix them, you are bound to get these kind of results as you are comparing apples and apple juice. Commented Jun 12, 2017 at 9:16
  • Please clarify "if collection is a List then always use BusinessObject.compareTo() if collection is a List then always use Person.compareTo() if collection is a List then always use Worker.compareTo()". Commented Jun 12, 2017 at 9:17
  • If you're implementing Comparable you do not have any information about the other elements of the list (except the elements to get compared with). So you cannot differ which comparison to use Commented Jun 12, 2017 at 9:18
  • 3
    And rather than Comparable, you know that Collections can also be sorted with Comparator ? That should help you achieve want you want. Commented Jun 12, 2017 at 9:34

2 Answers 2

6

Your design is inherently flawed.

First, you have BusinessObject which implements Comparable<BusinessObject>. What this says is that business objects have a natural order, dictated by their ID.

Then you override the method in Person to compare the name. What you are saying here is that "well, if both business objects being compared are persons, then the natural order is different for them".

That doesn't make sense. Either sorting by ID is natural for business objects, or it isn't. But it can't be natural for some of them, but not for others.

More formally, your override breaks the transitivity requirement of Comparable. Imagine you have a list consisting of three business objects: Worker Alice (ID 3), Worker Bob (ID 1), and Company ACME (ID 2). Transitivity says that if x < y && y < z then x < z must also be true. However, your compare method will give Bob < ACME (compare by IDs) and ACME < Alice (compare by IDs), but the transitive Bob < Alice is false (compare by names). You have violated the requirement documented in the compareTo method:

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

Bottom line: Comparable is not meant to work with open inheritance. Don't do that. Instead, use explicit Comparators for your collections where they make sense.

As a side note, and this may just be due to shortened code in your examples, but you should always override equals to be consistent with compareTo (i.e. x.equals(y) implies x.compareTo(y) == 0 and vice versa). Anything else means your objects behave in an unintuitive manner. (And then you need to override hashCode to fulfill its own contract w.r.t. equals. Overriding equals but not hashCode is a wonderful way to introduce really subtle bugs into Java programs.)

Sign up to request clarification or add additional context in comments.

1 Comment

You're right, I realized it after a few more tests. I'm dropping the implements Comparable<BusinessObject> in BusinessObject, I'll do my sortings using ad hoc Comparator depending on the situation. Thank you for your explanation.
1

I think it is simply not possible to do, what you want to do. The generic type does and can not have any effect on the methods used from the items.

But you might be able to solve your problem. Just do not use the compareTo methods but instead implement Comparators as needed.

There is a version of Collections.sort() which takes a Comparator as parameter.

Obviously you want to sort the items with different sort criterias depending on the list you are using. So I think the Comparator would be the best solution.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.