Overriding hashCode() not working

Question

I'm trying to make class Point work correctly with a HashSet. Here is my Point class:

class Point {

    int x;
    int y;

    Point(int x, int y) {
        x = x;
        y = y;
    }

    @Override
    public int hashCode() {
        int hash = 1;
        hash = hash * 17 + x;
        hash = hash * 31 + y;
        return hash;
    }

    @Override
    public boolean equals(Object o) {
        if (o == null) {
            return false;
        }
        Point p = (Point) o;
        return x == p.x && y == p.y;
    }
}

When I test it out and do

    HashSet<Point> h = new HashSet<Point>();
    h.add(new Point(0, 0));
    Point g = new Point(0, 1);
    System.out.println(h.equals(g));
    System.out.println(h.contains(g));

The output is this

false
true

Why is my hashCode not working?

@JeroenVannevel doh. I wasn't thinking. Let me fix it and test — user1529956, Mar 02 '14 at 23:09
Your bug is in the constructor. It does nothing. (And Sotirios beat me to it ... again) — Dawood ibn Kareem, Mar 02 '14 at 23:10
Your equals() method is incorrect. It should not throw a ClassCastException if something other than a Point is passed as argument. It should return false instead. — JB Nizet, Mar 02 '14 at 23:10
Question - does the constructor work? Doesn't defining the parameters as x and y override (or is it shadow/hide?) the global class variables? Wouldn't it then be like assigning a variable to itself? — asaini007, Mar 02 '14 at 23:11

score 7 · Answer 1 · edited May 23 '17 at 12:28

7

In

Point(int x, int y) {
    x = x;
    y = y;
}

You are assigning x, the local parameter variable, to itself. Same for y. These are no-ops.

Use

Point(int x, int y) {
    this.x = x;
    this.y = y;
}

so that you assign the parameter value to the field.

As others have noted, you shouldn't do

Point p = (Point) o;

without knowing if o is a Point or not. It will throw a ClassCastException if it is not assignable to a Point. Instead use

if (o instanceof Point)
    return false;

or

if (o.getClass() != Point.class) 
    return false;

before casting. Note that the two methods above are not equivalent. You can use the first in most cases, but use the second if Point is meant to have sub classes.

edited May 23 '17 at 12:28

Community

1
1

answered Mar 02 '14 at 23:09

Sotirios Delimanolis

274,122
60
696
724

Thanks! I was being careless > – user1529956 Mar 02 '14 at 23:11
+1, @OP: Use an IDE, that warns you, to prevent this in future. – chromanoid Mar 02 '14 at 23:11
Fixed the instanceof. Thanks again guys – user1529956 Mar 02 '14 at 23:16
Mostly a really good answer. But I have to disagree with the last sentence. Whether `Point` is meant to have subclasses should not be the criterion for choosing whether to use `instanceof` or `getClass()`. It's whether it makes sense for an object of one of those subclasses to be equal to a `Point`. As an example, if you look at the `equals` method of `AbstractList`, it uses `instanceof`, and `AbstractList` definitely has subclasses. But this is deliberate, because it makes sense for an `ArrayList` to be equal to a `LinkedList`, if the elements are all the same. – Dawood ibn Kareem Mar 03 '14 at 07:10
@DavidWallace I'll link to [this](http://stackoverflow.com/questions/13162188/java-equals-method-in-base-class-and-in-subclasses) to explain the reasoning for my suggestion. As for `List`, I think it's a special case. The `List` interface declares a `equals()` method with javadoc saying `Returns true if and only if the specified object is also a list, both lists have the same size, and all corresponding pairs of elements in the two lists are equal.` So, based on the spec, any `List` can be equal to any other `List`. This might not be true for other types where other fields might play into it. – Sotirios Delimanolis Mar 03 '14 at 07:20
Actually, I think of `List` as being the rule, rather than the exception. I would expect this kind of behaviour whenever we have multiple implementations of the same interface. If two objects are both of types that implement a particular interface, AND the two objects are indistinguishable in terms of methods from that interface, then they're effectively equal, and should return `true` when compare with `equals()`, even if their implementations are different. Looked at another way, the behaviour of `equals()` should be defined by what the interface is, not by what the implementation is. – Dawood ibn Kareem Mar 03 '14 at 08:39
@DavidWallace I accept all of that. `List` says what the rule is for its implementations. And it's easy enough to do that with `List` because of the various interface methods. But is a `Point2D` equal to a `Point3D` and vice versa? If you do `point2d.equals(point3d)` where they have equal x and y coordinates, the method might return true, but the inverse is not true, since a `Point3D` would probably want to compare the z dimension as well. – Sotirios Delimanolis Mar 03 '14 at 18:37
1

I think you and I are actually in loud agreement. `Point` and `Point3D` are basically value objects - they're not alternative implementations of a common interface. – Dawood ibn Kareem Mar 03 '14 at 19:15

Overriding hashCode() not working

1 Answers1