Murat Karagöz
Murat Karagöz

Reputation: 37594

TreeSet storing duplicate custom objects

Hello I probably oversaw something, but here it goes.
I have a TreeSet<CustomObject> and I do not want to have duplicates in the Set. My CustomObject class looks like this.

class CustomObject implements Comparable<CustomObject> {

    String product;
    String ean;

    public CustomObject(String ean){
      this.ean = ean; 
      // product is getting set via setter and can be null
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        CustomObject that = (CustomObject) o;

        return ean.equals(that.ean);

    }

    @Override
    public int hashCode() {
        return ean.hashCode();
    }

    @Override
    public int compareTo(CustomObject another) {
        if(equals(another)) return 0;
        if(product != null && another.product == null) return -1;
        if(product == null) return 1;

        return product.compareToIgnoreCase(another.product);
    }
}

Now I have a add Function for new objects.

private final TreeSet<CustomObject> productCatalog;

public void addObject(SomeData tag) {
    CustomObject p = new CustomObject(tag.getEan());

    if (productCatalog.contains(p)) { // <-- This checks only one entry of the Set.
        for (CustomObject temp : productCatalog) {
            if (temp.equals(p)) {
                p = temp; // I do stuff with that later which is irrelevent here
            }
        }
    } else {
       productCatalog.add(p);
    }   
} 

The method productCatalog.contains(p) calls the compareTo method from the Comparable Interface and does the comparing. The issue here is that it literally only checks I think the last object? in the set. So what happens is that only one unique CustomObject entry is present.

This is the scenario when I follow it with the debugger:

  1. productCatalog.contains(p)
  2. calls compareTo
  3. calls equals to check if ean.equals(that.ean)
  4. returns once true, but every other time false. Since it only checks the last object

How can I have it to check not only one object in step 4, but all the present objects in the Set. What am I missing?

Thx!


EDIT: These are some sample data. For the simplicity SomeData tag is basically a String.

First run:

addObject("Ean1") // success added
addObject("Ean2") // success added
addObject("Ean3") // success added
addObject("Ean4") // success added

Everything gets added into the TreeSet.

Second run:
addObject("Ean1") // failed already in the map
addObject("Ean2") // failed already in the map
addObject("Ean3") // failed already in the map
addObject("Ean5") // success added
addObject("Ean4") // success added
addObject("Ean4") // success added

For testing purpose I manually set product names depending on the String ean.

public CustomObject(String ean){
      this.ean = ean; 
       switch(ean){
            case "Ean1": product = "TestProduct"; break;
            case "Ean2": product = "ProductTest";break;
            case "Ean3": product = "Product";break;
    }

The TreeSet acts as a cache.


Edit2: This is how I solved it.

            for (CustomObject temp : productCatalog) {
                if (temp.equals(p)) {
                    p = temp; // I do stuff with that later which is irrelevent here
                }
            }

I removed the if statement with the contains method since that would always return ´1or-1in my special case. Now I simply iterate over the Set to correctly use theequals` method since the TreeSet uses compareTo() for checking every element in the Set.

The Java Docs state the following

Note that the ordering maintained by a set (whether or not an explicit comparator is provided) must be consistent with equals if it is to correctly implement the Set interface. (See Comparable or Comparator for a precise definition of consistent with equals.) This is so because 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, so two elements that are deemed equal by this method are, from the standpoint of the set, equal. The behavior of a set is well-defined even if its ordering is inconsistent with equals; it just fails to obey the general contract of the Set interface.

Upvotes: 0

Views: 446

Answers (1)

J Fabian Meier
J Fabian Meier

Reputation: 35805

The main problem:

compareTo does return 1 if both product and other.product are null. This is wrong because they are actually equal. You probably forgot to set product names for the higher ean values, like "Ean4" and "Ean5".

Old answer:

Your implementations of equals and compareTo do not fit together.

equals works on the ean and compareTo on the product. This only works if you implicitly assume that equal ean imply equal product. If this is not true in your test cases, the result will be wrong.

In either case, it is no good implementation because this can lead to a < b, b < c but a "equals" c.

Upvotes: 1

Related Questions