views:

433

answers:

3

I have a class Odp. I want to use TreeSet to keep a sorted collection of Odp objects. However, I've been having problems.

public class OdpStorage {

    private TreeSet<Odp> collection = new TreeSet<Odp>(); 

    public addOdp(Odp o) {
          return collection.add(o);
    }

    public int size() {
          return collection.size();
    }

}

collection.add(Odp o) is supposed to do nothing if it's already in the tree, right? Somehow, this unit test fails:

OdpStorage ts = new OdpStorage();    
Odp ftw = new Odp("LOL");
 Odp ktr = new Odp("OMG");

    ts.addOdp(ftw);

 ts.addOdp(ftw); //should do nothing
 ts.addOdp(ftw); //should do nothing
 ts.addOdp(ftw); //should do nothing
 ts.addOdp(ktr);

assertEquals(2, ts.size());

The assertion fails. It expects 2, but the return value is 5. Why? Could the odp.equals() function be messed up?

Similarly, calling collection.contains(o) fails, even when the there is an object in the set X for which o.equals(X) returns true.

The .equals() function of Odp: (generated by Eclipse)

public boolean equals(Object obj) {
 if (this == obj)
  return true;
 if (obj == null)
  return false;
 if (!(obj instanceof Odp))
  return false;
 Gene other = (Odp) obj;
 if (sequence == null) {
  if (other.sequence != null)
   return false;
 } else if (!sequence.equals(other.sequence))
  return false;
 return true;
}

compareTo:

/**
 * this = g0
 * if they are equal, g1 is presumed to come first
 * 
 *  @return -1 if g0 comes before g1; 1 if g0 comes after g1
 */
@Override
public int compareTo(Odp g1) {

 if (sequence.length() < g1.getSeq().length()) {
  return -1;
 }
 else if (sequence.length() > g1.getSeq().length()) {
  return 1;
 }

 if (sequence.compareTo(g1.getSeq()) < 0) {
  return -1;
 }

 return 1;
}

hashCode() is not overridden. Problem?

UPDATE hashCode() is as follows:

@Override
public int hashCode() {
 final int prime = 31;
 int result = 1;
 result = prime * result
   + ((sequence == null) ? 0 : sequence.hashCode());
 return result;
}

But that still doesn't solve the problem.

+1  A: 

It appears that your collection.add(o) is failing to find the object in the backing TreeMap. Does your Odp implement Comparable or are you setting a default Comparable on your TreeSet whose compare method you have implemented? If so, you will need to ensure that your compareTo (for the Comparable), or your Comparator compare method will return 0 if the objects passed in are equals.

EDIT (in response to your comment to the original post):

It is recommended that you override HashCode() whenever you override equals()

EDIT2 in response to your compareTo implementation:

If g0 and g1 are equal, you should return 0. This is the root of the problem.

akf
Odp does indeed implement Comparable.
Rosarch
I would target the `compareTo` method in your debugging first. Post it if you would like a review.
akf
When I was using a TreeMap instead of TreeSet, it was sorting just fine. This leads me to believe that it's not an issue with implementing the Comparable interface.
Rosarch
+1. TreeSet and TreeMap both use compareTo() (or supplied Comparator) instead of equals()
ChssPly76
that is correct. Thank you.
Rosarch
+2  A: 

Your compareTo implementation never returns 0. It should return 0 when the object instances are equal.

Cwmur
A: 

Mate cleanup your equals, its got too many if/elses. replace it with a nice do/while with lots of condition tests. If all the tests pass then reutrn true...Yes its got "goto" statements but its very easy to read and even easier to insert new conditions as necessary without lots of nesting. Nesting if/elses is evil. Using "elses" is evil and almost always never needed.

@Override
public boolean equals(final Object object) {
    boolean equals = false;

    do {
        if (this == object) {
            equals = true;
            break;
        }
        if (false == super.equals(object)) {
            break;
        }
        final DocumentView view = Unsafe.cast(object);
        if (false == this.document.equals(view.document)) {
            break;
        }
        if (this.revision != view.revision) {
            break;
        }
        if (false == this.user.equals(view.user)) {
            break;
        }
        if (false == this.timestamp.equals(view.timestamp)) {
            break;
        }
        equals = true;
    } while (false);

    return equals;
}
mP
if (!...) { return false;}return true;is much prettier and without the do while.
It's better to haveone return, it's easier to set a single breakpoint or guard rather than having to set many.
mP
If you ever have to come back it's quicker to set a single breakpoint than the time to type the wrapping do while. In short don't be lazy and smart using returns rather than type a bit more and have a single return.
mP