CODEWITHSUNDEEP
c#
Carlo
Carlo

Reputation: 25959

Using GetHashCode in == operator override

Just wondering if what I mentioned in the title is a good practice. It makes sense to me, we're overriding GetHashCode to return a value based on two properties that if match, the two objects should be treated as equal. Logic seems fine and the code works, but I don't know if it can cause other problems.

This is using GetHashCode:

public static bool operator ==(CartesianCoordinates a, CartesianCoordinates b)
{
    return a.GetHashCode() == b.GetHashCode(); // Using GetHashCode here
}

public static bool operator !=(CartesianCoordinates a, CartesianCoordinates b)
{
    return !(a == b);
}

public override bool Equals(object obj)
{
    return this == (CartesianCoordinates)obj; // This just uses the == override
}

public override int GetHashCode()
{
    return (this.X + this.Y.ToLower()).GetHashCode(); // GetHashCode hashes the two properties we care about
}

And this is how I had it before:

public static bool operator ==(CartesianCoordinates a, CartesianCoordinates b)
{
    return a.X == b.X && string.Equals(a.Y, b.Y, StringComparison.CurrentCultureIgnoreCase); // The difference is here
}

public static bool operator !=(CartesianCoordinates a, CartesianCoordinates b)
{
    return !(a == b);
}

public override bool Equals(object obj)
{
    return this == (CartesianCoordinates)obj;
}

public override int GetHashCode()
{
    return (this.X + this.Y.ToLower()).GetHashCode();
}

Important Note:

In the CartesianCoordinates object, X is an int and Y is a string:

public int X { get; set; }
public string Y { get; set; }

Lmk, thanks in advance!

Upvotes: 1

Views: 412

Answers (6)

Tetsujin no Oni
Tetsujin no Oni

Reputation: 7367

Your GetHashCode will return the same value for {3,1} and {1,3}. I suspect this is not intended.

If your GetHashCode method returned a better hashcode, I'd still recommend against using GetHashCode as an equality comparison. The GetHashCode contract says that two objects with the same tracked properties should generate the same hashcode, not that two objects with equal hashcodes will be equal.

Edit to add:

See this question for details on what a better implementation looks like, particularly the Josh Bloch / Jon Skeet answer.

Upvotes: 0

Thomas Levesque
Thomas Levesque

Reputation: 292425

Doing this is not only bad practice, it's just wrong! Two objects that are equal must have the same hashcode, but the opposite is not true: two different objects can have the same hashcode (and often will). So if you use the hashcode to decide whether or not the objects are equal, in some case you will consider them equal when they're actually different but just happen to have the same hashcode. A hashcode is not a unique identifier...

Based on your GetHashCode implementation, objects with coordinates (x, y) and (y, x) will be considered equal (since x + y == y + x)

Upvotes: 8

Andrew Cooper
Andrew Cooper

Reputation: 32576

Your GetHashCode() will give the same result for (2,4) and (4,2). Probably not what you want.

GetHashCode() is also generally not a good test for equality. Even a really good hash is only a 32-bit integer. To use that to test for equality in the cartesian plane will result in many (an infinite number) of collisions.

Upvotes: 0

sepp2k
sepp2k

Reputation: 370142

No, it's not good practice (or even correct) to define equality using the hash code since different objects may have the same hash code (and in the case of String.GetHashCode that can definitely happen).

In your particular case it should also be noted that your GetHashCode method doesn't even match your Equals method because your GetHashCode will return different hash codes for strings that are equal except for case, while your Equals will return true in that case.

Upvotes: 0

Miserable Variable
Miserable Variable

Reputation: 28752

GetHashCode() can return same value for two CartesianCoordinates that are not equal, e.g. for two objects c1 and c2 such that c1.x == c2.y and c1.y == c2.x.

For more complex objects and if the hashCode is pre-computed it can be used to short-circuit but eventually you will need to compare all fields. Here the comparison is trivial

Upvotes: 3

SLaks
SLaks

Reputation: 887433

That is very wrong.
GetHashCode() is not unique.

Your particular example is even worse, since your GetHashCode() is commutative.

Upvotes: 4

Related Questions