TreeSet doesn't work, what's wrong with the following code?

Question

The following code should print 3 persons, but it actually print 4 persons, why? Person("a", 1) and Person("a", 4) should be treated as the same, but they are not.

import java.util.TreeSet;
public class Person implements Comparable<Person>{
    public Person(String name, int age){
        this.name = name;
        this.age = age;
    }
    public String name;
    public int age;

    @Override
    public int compareTo(Person o) {
        if(this.name.equals(o.name)){
            return 0;
        }else if(this.age <= o.age){
            return -1;
        }else {
            return 1;
        }
    }
    @Override public boolean equals(Object other) {
        return name.equals(((Person)other).name);
    }

    @Override public int hashCode() {
        return age * 31; 
    }
    public static void main(String[] args) {
        TreeSet<Person> persons = new TreeSet<Person>();
        persons.add(new Person("a", 1));
        persons.add(new Person("b", 3));
        persons.add(new Person("c", 2));
        persons.add(new Person("a", 4));//This should be ignored.

        /*Here should print the first 3 persons, but it prints 4 persons*/
        for(Person h: persons){
            System.out.println(h.name + ":" +h.age);
        }
    }
}

Result:
a:1
c:2
b:3
a:4

Note that your compareTo doesn't respect its contract either: if a and b have the same name and the same age, a < b and b < a at the same time. — JB Nizet, Aug 22 '14 at 14:39
Thanks very much to JB Nizet & Sotirios Delimanolis & Malt for your great answers. — ruchun huang, Aug 22 '14 at 15:29

Malt · Accepted Answer · 2014-08-22T15:47:51.180

4

The problem is the compareTo method, which performs comparisons based on age, but determines equality based on the Person's name, making it inconsistent with itself.

After inserting the first 3 Persons, the tree looks something like this (forgive the poor ASCII art):

    (c,2)
      |
     /\
 (a,1) (b,3)

Then (a,4) is inserted, it's compared to (c,2) based on the age. The age (4) is bigger than (2), so we go to the right. The next comparison is with (b,3). (a,4) is again bigger, so it's added as a new node to the tree (since there's nothing else to compare to). If instead of adding (a,4) you'd add (a,0), then there would be a comparison with (a,1), and the result would be:

a:1
c:2
b:3

edited Aug 22 '14 at 15:47

answered Aug 22 '14 at 14:36

Malt

28,965
9
65
105

Very nice explained, but it's missing the most important part : **WHY** that happens. – Daniel Aug 22 '14 at 15:00
Thanks very much, could you help me: if I want to order by age, and ignore the Persons with the same name, how to implement? Thanks. – ruchun huang Aug 22 '14 at 15:21
@ruchunhuang Have your compareTo first compare based on age, and if the age is equal, then return the result of the string comparison: return this.name.compareTo(o.name) – Malt Aug 22 '14 at 15:25
@Malt: that will only ignore duplicate names of persons having the same age. – JB Nizet Aug 22 '14 at 15:37
@Malt But if the age doesn't equal, then the person whose name already exists in TreeSet will be added again to TreeSet, how to resolve this problem? – ruchun huang Aug 22 '14 at 15:37

score 3 · Answer 2 · answered Aug 22 '14 at 14:45

3

A TreeSet is implemented with a TreeMap which is a red black tree. When you add the first element, it becomes root

(A, 1)

When you add the second, it has a differdnt name and a bigger age, so it becomes the root's right child.

(A, 1)
    \
    (B, 2)

When you add the third, it's again bigger than those. The tree reorganizes itself for balance.

    (B, 2)
    /    \
(A, 1)  (C, 3)

You then add your final element. 4 is bigger than 2, so search goes down the right branch and doesn't find an element with name "a", so it adds a new entry.

    (B, 2)
    /    \
(A, 1)  (C, 3)
            \ 
            (A, 4)

answered Aug 22 '14 at 14:45

Sotirios Delimanolis

274,122
60
696
724

Although what you wrote seems logic I can't understand how it relates to what can be found in the `TreeSet` API doc for `add()` method: "More formally, adds the specified element e to this set if the set contains no element e2 such that `(e==null ? e2==null : e.equals(e2))`". From the question clearly `(new Person("a", 1)).equals(new Person("a", 4))`. I haven't found anything about red black tree in the doc. Can you supply some link please? I'm refering to this: http://docs.oracle.com/javase/7/docs/api/java/util/TreeSet.html#add%28E%29 – Michał Schielmann Aug 22 '14 at 15:01
@mic It's hard for me to give you links since I'm on my phone. Take a look at the class javadoc (the doc at the top of thr javadoc). `TreeSet` uses a `TreeMap` which is implemented as a red black tree. It seems to me like the add method javadoc simply hasn't been changed from the `Set` interface. – Sotirios Delimanolis Aug 22 '14 at 15:06
1

This is the default javadoc of the Set api. It's actually incorrect for TreeSet. The class javadoc of TreeSet explains it clearly: *the Set interface is defined in terms of the equals operation, but a TreeSet instance performs all element comparisons using its compareTo (or compare) method*. – JB Nizet Aug 22 '14 at 15:07
Thanks for clearing it up for me. But this raises a question - if I cannot trust Oracle API doc where should i find answers? I guess that SO is only right place ;) – Michał Schielmann Aug 22 '14 at 15:24

score 3 · Answer 3 · answered Aug 22 '14 at 14:47

3

Your compareTo method doesn't respect the contract of Comparable.compareTo().

It should be symmetric: if a < b, then b > a. That is not the case since if a and b have the same name and the same age, a.compareTo(b) is -1, and b.compareTo(a) is -1 as well. So a is bigger than b and smaller than b at the same time.

It should be transitive: if a < b and b < c, then a < c. This is not the case:

(a, 3) < (b, 7),
(b, 7) < (a, 10)
but (a, 3) = (a, 10)

Given that the contract is not respected, you can't expect anything from TreeSet, which assumes that the contract is respected to work.

answered Aug 22 '14 at 14:47

JB Nizet

678,734
91
1,224
1,255

Great answer, thank you! By the way, if I want a TreeSet to order by age, and ignore the Person whose name already exists in TreeSet, how to implement this idea? Thanks – ruchun huang Aug 22 '14 at 15:14
You can't do it just with a compareTo method. Define a comparator which orders by age then by name and use it for your TreeSet. But before adding it, Iterate through the Set to make sure it doesn't already have a person with the given name (or use a parallel Set storing all the names of the persons that have been added to the set, in order to avoid the iteration). – JB Nizet Aug 22 '14 at 15:22

score 1 · Answer 4 · answered Aug 22 '14 at 14:27

1

Your implementation of compareTo doesn't match your implementation of equals. TreeSet operates on compareTo.

As a side note: The implementation of hashCode() is also wrong, as it's not in line with equals either.

answered Aug 22 '14 at 14:27

Ray

3,084
2
19
27

That doesn't explain why the 4th person isn't ignored. – JB Nizet Aug 22 '14 at 14:34
How does their implementation of `compareTo` explain the results. – Sotirios Delimanolis Aug 22 '14 at 14:35
Sorry, equals & hashCode methods are written in a hurry. – ruchun huang Aug 22 '14 at 15:00

TreeSet doesn't work, what's wrong with the following code?

4 Answers4