2

I am trying to check for and remove elements, using a HashMap in Java. Its keys are a type I created called ClusterKey, and its values are a type I created called ClusterValue.

Here is the code that is causing issues:

ClusterKey ck = new ClusterKey(Long.parseLong(split[0].split("=")[1]), 
                        Integer.parseInt(split[1].split("ey")[1]));
if (arg == 0) keys.put(ck, new ClusterValue(index, false));
if (arg == 1) {
    if (keys.containsKey(ck)) {
        index = keys.get(ck).messageNo;
        keys.remove(ck);
    }
    keys.put(ck, new ClusterValue(index, true));
}

The problem is that even when the ClusterKey is the same as an existing ClusterKey, containsKey() and remove() do not seem to recognize it as equal. I have implemented equals() in class ClusterKey to override Java's equals() method, as follows:

class ClusterKey {
    long firstKey;
    int secondKey;
    public ClusterKey(long firstKey, int secondKey) {
        this.firstKey = firstKey;
        this.secondKey = secondKey;
    } public boolean equals(Object otherKey) {
        return this.firstKey == ((ClusterKey) otherKey).firstKey && this.secondKey == ((ClusterKey) otherKey).secondKey;
    }
}

So, I'm quite confused. Thanks so much for your help.

Regards, Rebecca

UPDATE: Thank you for your advice and feedback on my code. I was able to solve the problem by adding hashCode() to ClusterKey, as follows:

    } public boolean equals(Object otherKey) {
        return this.firstKey == ((ClusterKey) otherKey).firstKey && this.secondKey == ((ClusterKey) otherKey).secondKey;
    } public int hashCode() {
        return (int) firstKey + secondKey;
    } 
La-comadreja
  • 5,627
  • 11
  • 36
  • 64
  • 1
    Did you implement `hashCode`? That `hash` part is important. – Dave Newton Jul 16 '13 at 18:33
  • 2
    Right, _always_ implement `equals()` and `hashCode()` together. – GriffeyDog Jul 16 '13 at 18:41
  • You should be able to just call `put()`, without removing the key first (it'll overwrite, once you deal with `hashCode()`). Beyond that, I'm a little suspicious about the use of `arg`, as it looks like you're using it as a boolean argument, which isn't good practice. – Clockwork-Muse Jul 16 '13 at 18:43
  • Get a copy of *Effective Java* (2nd ed. if possible) by Joshua Bloch and read Items 8 and 9 on `hashCode()` and `equals()`. There are a lot of nuances to be aware of, and Bloch covers them thoroughly. – Chad Jul 16 '13 at 18:41

2 Answers2

3

You will need to implement hashCode(), not just equals, for your ClusterKey to work well as a key in a HashMap.

Specifically, quoting from the linked Javadocs:

If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.

rgettman
  • 176,041
  • 30
  • 275
  • 357
  • And the `equals()` method, to be compliant with the contract, should accept nulls and any kinds of objects. Not just ClusterKey instances. It's not supposed to throw ClassCastException or NullPointerException. – JB Nizet Jul 16 '13 at 18:36
3

For any Hash enabled data structure (like HashMap, HashSet) to work correctly its elements must override hashCode() in addition to the equals() method. The reason being that the hash code is used to identify the bucket in which to put the element (during insertion) or search in (using equals() during a lookup).

If you do not override hashCode(), the default implementation from Object#hashCode() is used which would return different values even for the objects that you consider equivalent (the equals() method returns true for).

This is why your

 hashMap.containsKey(ClusterKey key)

calls are failing in spite of the key already being present. Since, the hash codes don't match the HashMap never looks for the key in the right bucket. Hence, your equals() never gets called here.

Ravi K Thapliyal
  • 51,095
  • 9
  • 76
  • 89