3

I have a class called Employee which has employeeName and employeeId as its member variables.I am creating new Employee objects and then adding it into a TreeSet where I want to sort it based on the employeeId. But I consider 2 Employee objects to be equal if they have the same employeeName. Set does not allow duplicates. But here I can observe a strange behavior. This is my code.(I am not using getters and setters here. I am directly accessing the member variables.)

package secondOne;

import java.util.Set;
import java.util.TreeSet;

class Employee implements Comparable<Employee> {

    String employeeName;
    int employeeId;

    public Employee(String name, int id) {
        this.employeeName = name;
        this.employeeId = id;
    }

    public int compareTo(Employee emp) {
        //return this.employeeName.compareTo(emp.employeeName);
        return (this.employeeId - emp.employeeId);
    }

    @Override
    public String toString() {
        return ("Name is: " + employeeName + " Emp id is: " + employeeId);
    }

    @Override
    public boolean equals(Object emp) {
        if (emp instanceof Employee && ((Employee) emp).employeeName == this.employeeName) {
            return true;
        }
        return false;
    }

}

public class TestingSetsWithComparable {
    /**
     * @param args
     */
    public static void main(String[] args) {
        Employee e1 = new Employee("A", 1);
        Employee e2 = new Employee("A", 2);
        Employee e3 = new Employee("B", 3);

        Set<Employee> set = new TreeSet<Employee>();
        set.add(e1);
        set.add(e2);
        set.add(e3);
        System.out.println(set);
    }
}

Here the output for the above code is,
[Name is: A Emp id is: 1, Name is: A Emp id is: 2, Name is: B Emp id is: 3]

My first question is, in equals() method, I consider 2 Employee objests to be equal if they have same employeeName but in compareTo method, I am using employeeId to sort. In this case, output is showing 2 entries for employeeName 'A'. How is TreeSet allowing a duplicate entry when I consider 2 objects to be same if they have the same employeeName. How is this possible..? And Second question is, in the compareTo method, if I use employeeName to sort, then I dont get the second repeated entry for the same name. The output in this second case is
[Name is: A Emp id is: 1, Name is: B Emp id is: 3]

Why is it so..?

Erwin Bolwidt
  • 30,799
  • 15
  • 56
  • 79
Prasad
  • 368
  • 1
  • 6
  • 16
  • What happen when you uncomment this //return this.employeeName.compareTo(emp.employeeName);, Does this not work ? – Piyuesh Jul 12 '13 at 06:19
  • If you are using Eclipse, you can simply generate equals() and hashCode(). In other good IDEs as well, of course;-) – boskop Jul 12 '13 at 06:21
  • Even when i use equals() method i still get the same output. I am still getting the duplicate entry. I have changed == to equals in the code. – Prasad Jul 12 '13 at 06:36

3 Answers3

8

The problem is here:

((Employee)emp).employeeName== this.employeeName

You must compare Strings using equals method:

((Employee)emp).employeeName.equals(this.employeeName)

Refer to How do I compare strings in Java?

Also, since you're overriding equals method, it would be good if you override hashCode method as well, as stated in Object#equals contract:

Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.

Additional: Since you're using TreeSet, it will use the compareTo method instead of the equals and hashCode methods. This is because TreeSet implements SortedSet interface. Refer to SortedSet javadoc (emphasis mine):

A Set that further provides a total ordering on its elements. The elements are ordered using their natural ordering(i.e. implementing Comparable<T>), or by a Comparator typically provided at sorted set creation time.

You should implement this method in the accordingly to your needs:

public int compareTo(Employee emp) {
    if (this.employeeName.equals(emp.employeeName)) {
        return 0;
    }
    //removed the comparison by subtraction since it will behave wrongly on int overflow
    return new Integer(this.employeeId).compareTo(emp.employeeId);
}

Since you're comparing Strings, I would recommend using StringUtils class from Apache Commons Lang that provides helper methods to avoid null checks and others.

Community
  • 1
  • 1
Luiggi Mendoza
  • 85,076
  • 16
  • 154
  • 332
  • I edited the code which uses equals method to compare 2 strings instead of ==. But still i am getting the same output. In the compareTo method, i dont want to compare 2 employees based on their name, my requirement is to sort employees based on their Id. So I am using their ids to sort in the compare to method. And before sorting, why did Set allow a duplicate entry when i say 2 objects are equal if their name matches..? – Prasad Jul 12 '13 at 06:31
  • To serve my requests, which structure should i use..? – Prasad Jul 12 '13 at 06:42
  • 1
    @Prasad answer updated after refreshing my face and rethinking the problem. Note the new implementation of the `compareTo` method. – Luiggi Mendoza Jul 12 '13 at 06:44
  • the compareTo method implementation you provided worked fine. I didnt get this part. if (this.employeeName.equals(emp.employeeName)) { return 0; } what does this achieve..? what will happen if we return a zero when names are equal..? Can you kindly explain..? – Prasad Jul 12 '13 at 06:52
  • 2
    @Prasad it means that you're first comparing if the incoming `Employee` object has the same name as the current `Employee` object. If so, returns that they are equals (that's the meaning of 0 by [`Comparable#compareTo`](http://docs.oracle.com/javase/7/docs/api/java/lang/Comparable.html#compareTo(T)) javadoc. Since the element is already present in the `TreeSet`, it won't be inserted in the structure (defined in [`Set#add`](http://docs.oracle.com/javase/7/docs/api/java/util/Set.html#add(E)). – Luiggi Mendoza Jul 12 '13 at 06:59
  • @Prasad I already added a comment explaining this. If you need something more to be explained, feel free to ask. – Luiggi Mendoza Jul 12 '13 at 07:00
  • @LuiggiMendoza I am not quite convinced with the answer. You are not highlighting why equals method is **not** used by `TreeSet` and why should `TreeSet` use `compareTo` to check the **equality**? and how does `hashCode` comes into picture for `TreeSet` ? – sanbhat Jul 12 '13 at 07:08
  • 1
    @sanbhat `TreeSet` implements `NavigableSet` that extends `SortedSet` that works with *natural ordering* instead of `equals` and `hashCode`. I've posted the relevant javadoc in my answer. Not sure what do you mean here: * You are not highlighting why equals method is not used by TreeSet and why should TreeSet use compareTo to check the equality?* – Luiggi Mendoza Jul 12 '13 at 07:24
  • @LuiggiMendoza i got my answer thanks http://stackoverflow.com/questions/17609419/treeset-violating-set-contract#17609488 – sanbhat Jul 12 '13 at 07:25
  • @Prasad check the link I've posted in my last comment. You need to mark the check below the rep (there's an image about it). – Luiggi Mendoza Jul 12 '13 at 07:36
  • Thanks @LuiggiMendoza This discussion gave a whole new dimension to my collection knowledge :) – Prasad Jul 12 '13 at 08:02
  • Thanks @sanbhat: his discussion gave a whole new dimension to my collection knowledge :) – Prasad Jul 12 '13 at 08:02
1

you should compare string not with ==, but with equals() method, and also you should override your compareTo method to compare with employeeName not with employeeId, if you want it in that way.

(Employee)emp).employeeName.equals(this.employeeName)

and

public int compareTo(Employee emp) {

  return (this.employeeName-emp.employeeName);
}
ridoy
  • 6,274
  • 2
  • 29
  • 60
Onur A.
  • 3,007
  • 3
  • 22
  • 37
  • Even when i use equals() method i still get the same output. I am still getting the duplicate entry. I have changed == to equals in the code. – Prasad Jul 12 '13 at 06:38
  • how about changing your compareTo method to the on i indicated in my answer ? – Onur A. Jul 12 '13 at 06:38
  • If i want to sort using employeeName, then it works fine. But my requirement is i want to sort based on the employeeId rather than on their name. My another question is, how did it even allow a duplicate entry in the first place before sorting. – Prasad Jul 12 '13 at 06:45
  • before duplicate means different things to you and TreeSet, for you duplicate is the employees with same name but for TreeSet apparently not employeeName, possibly employeeId or memory address of each object, you should better use a Set – Onur A. Jul 12 '13 at 07:52
0

The way you are comparing string is wrong. See How to compare 2 Strings in Java

(Employee)emp).employeeName== this.employeeName

should be

(Employee)emp).employeeName.equals(this.employeeName)
Community
  • 1
  • 1
sanbhat
  • 17,522
  • 6
  • 48
  • 64
  • Even when i use equals method i still get the same output. I am still getting the duplicate entry. I have changed == to equals in the code. – Prasad Jul 12 '13 at 06:22
  • 1
    `TreeSet` doesn't use `equals` method for element comparison. – Luiggi Mendoza Jul 12 '13 at 06:48
  • @LuiggiMendoza it does.. thats what TreeSet.add documentation tells?http://docs.oracle.com/javase/6/docs/api/java/util/TreeSet.html#add(E) – sanbhat Jul 12 '13 at 06:52
  • Please refer to my answer to see further details on the subject, I don't like to repeat things :) – Luiggi Mendoza Jul 12 '13 at 06:57
  • @sanbhat: Luiggi Mendoza is right. It is not at all using equals method to compare. Even i checked it now. Remove the equals method, and insert the compareTo method given by Luiggi in his answer description. It works properly. – Prasad Jul 12 '13 at 06:58