views:

207

answers:

3

I have the following object Node:

    private class Node implements Comparable<Node>(){
         private String guid();

         ...

         public boolean equals(Node o){
             return (this == o);
         }

         public int hashCode(){
              return guid.hashCode();
         }

         public int compareTo(Node o){
            return (this.hashCode() - o.hashCode());
         }

         ...

    }

And I use it in the following TreeMap:

TreeMap<Node, TreeSet<Edge>> nodes = new TreeMap<Node, TreeSet<Edge>>();

Now, the tree map is used in a class called Graph to store nodes currently in the graph, along with a set of their edges (from the class Edge). My problem is when I try to execute:

   public containsNode(n){
        for (Node x : nodes.keySet()) {
            System.out.println("HASH CODE: ");
            System.out.print(x.hashCode() == n.hashCode());
            System.out.println("EQUALS: ");
            System.out.print(x.equals(n));
            System.out.println("CONTAINS: ");
            System.out.print(nodes.containsKey(n));
            System.out.println("N: " + n);
            System.out.println("X: " + x);
            System.out.println("COMPARES: ");
            System.out.println(n.compareTo(x));
            }
        }

I sometimes get the following:

HASHCODE: true EQUALS: true CONTAINS: false N: foo X: foo COMPARES: 0

Anyone have an idea as to what I'm doing wrong? I'm still new to all this, so I apologize in advance if I'm overlooking something simple (I know hashCode() doesn't really matter for TreeMap, but I figured I'd include it).

edit1: added compareTo() method information.

A: 

Check your comparator.

The containsKey() calls getEntry() which might rely on comparator. If it is broken, you can expect inconsistent results.

mindas
I've added `System.out.println(nodes.comparator())` and it returns `null`. So this means it's using natural ordering, which in this case means using `Node.compareTo()` correct? I've added my `compareTo()` method above.
smessing
+1  A: 

TreeSet does not use equals() to determine equality. It uses the Comparator (or Comparable) instead. In order to make it work right, you must follow the consistency with equals rule:

"The ordering imposed by a comparator c on a set of elements S is said to be consistent with equals if and only if c.compare(e1, e2)==0 has the same boolean value as e1.equals(e2) for every e1 and e2 in S".

I guess you are not following this rule (you didn't provide the implementation of the compareTo method). When the rule is not followed, the tree set will not have a normal behavior of a Set.

For details see http://eyalsch.wordpress.com/2009/11/23/comparators/.

--EDIT--

Now that you provided your compareTo implementation, is is clear that it has a flaw. It may return 0 for 2 nodes which are not equal (and have the same hashcode). As a result of this, you can't add 2 items with the same hash code into your TreeSet!

Eyal Schneider
I've added my `compareTo()` method and what happens when `n.compareTo(x)` is called above. It consistently returns 0, and still I'm having trouble..
smessing
That is a flaw, definitely, but to be honest it doesn't matter. For this project, I know going into it that each Node will have a *distinct* `guid`. Plus, fixing that wouldn't solve my current problem, correct?
smessing
Distinct guid != distinct hashcode, unless it is guaranteed that every guid has a distinct hashcode (which I find unlikely). Two strings can be distinct, but collide with the same hashcode.
Brandon Bodnár
Distinct hash code of the guid is important. But it is the birthday problem on a planet with four billion days in the year. I think integer overflow is more likely.
Tom Hawtin - tackline
Another flaw (which is also unrelated to your problem) is that your equals(..) method does NOT override Object's equals(). The argument must be of type Object.
Eyal Schneider
+3  A: 

There are a couple of things wrong here.

  • You have not overriden Object.equals. Use @Override public boolean equals(Object obj).
  • There is a potential integer overflow error in compareTo. This is probably the cause of this particular error. It will upset the sorting, and hence the search may well not succeed.
  • The compareTo method claims two instances are equal if the hash code happens to match (could be a difficult error to catch, without code review).

For integer overflow issue, see question Why is my simple comparator broken?

Tom Hawtin - tackline
Thanks for the comments. For the `compareTo` claiming two instances are equal... that's okay because I know that each node will have a *distinct* `guid`, it's a fact of the problem I'm solving.As for `equals(Object obj)`, I will implement that, but could that be the source of my problem, since I'm always calling `equals(Node)`? I know it's a good programming practice issue, but beyond that, could it actually be generating my issue?
smessing
As comment on other answer, distinct guid does not imply distinct hash code of guid. I think it's the integer overflow that is causing the problem.
Tom Hawtin - tackline
The int overflow is likely the cause. I encountered this issue before in code I inherited, where the node is in the tree but due to the int overflow problems the compareTo function lost its transitive property, causing it to go the wrong way in the tree when doing the contains search. I would change the compareTo function to use the result of `this.guid.compareTo(other.guid)` instead. Also if same guid means same node, then the equals method should also make use of `this.guid.equals(other.guid)`.
Brandon Bodnár
+1 for the int overflow problem.
ILMTitan
Integer overflow looks like it was the culprit, thanks guys!
smessing