0
\$\begingroup\$

I have a custom class to handle some triangle calculations that I am doing. For the most part I am happy with it, however, there is one aspect that I don't really trust. That is my implementation of HashCode.

Some quick information about my triangle- it has 3 Vector2 structs that represent points of two floats. This is an inbuilt class with the Unity engine but the simple version of it is that it is a struct with two floats and a couple math functions.

public class Triangle2D {
    public Vector2 point1;
    public Vector2 point2;
    public Vector2 point3;

    //... Some other methods that don't matter

    public override int GetHashCode() {
        var hashCode = -305700999;
        hashCode = hashCode * -1521134295 + (EqualityComparer<Vector2>.Default.GetHashCode (point1)
            + EqualityComparer<Vector2>.Default.GetHashCode (point2)
            + EqualityComparer<Vector2>.Default.GetHashCode (point3));
        return hashCode;
    }
}

The specific thing about this class that is special is that, it shouldn't matter what order points 1, 2 and 3 are in. If the points are shared, then the Triangle2D objects are the same. My equals function reflects that, but I worry that my hashCode function is not specific enough. Due to the additive nature of things, I suspect there could be a lot of collisions.

Is there some way that I could make the hashCode function have less collisions? Is it good enough as it is?

I use this object in a lot of hashSets to prevent duplicate triangles from getting listed.

\$\endgroup\$

closed as off-topic by t3chb0t, Snowbody, Sᴀᴍ Onᴇᴌᴀ, Ludisposed, Mast Dec 22 '17 at 15:19

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Snowbody, Sᴀᴍ Onᴇᴌᴀ, Ludisposed
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ There are like one million answers on Stack Overflow about calculating the hash-code. \$\endgroup\$ – t3chb0t Dec 22 '17 at 5:48
  • \$\begingroup\$ I think you actually consider your code broken, because it has too many hash collisions and you are asking how to make the function more robust. I believe it is actually off-topic for CodeReview. \$\endgroup\$ – Snowbody Dec 22 '17 at 5:51
  • \$\begingroup\$ What is the best algorithm for an overridden System.Object.GetHashCode? \$\endgroup\$ – radarbob Dec 22 '17 at 7:05
1
\$\begingroup\$

Hash code

it shouldn't matter what order points 1, 2 and 3 are in

There is a way to ensure that: Sorting. Sort your points locally within your hashcode method, first by their x value, then by their y value.


The algorithm for the hashcode could be this one, as also commented above by radarbob. I will leave it for you to adapt it to your own class.

General

Your class has public fields. It is better to make these private and grant public access to them (if needed) via properties. In this case, for something like a triangle, making it a struct instead of a class might be also suitable.


The following piece of code does not make much sense:

var hashCode = -305700999;
hashCode = hashCode * -1521134295 + [...]

I already added the link to a good implementation of the hash code method above, so I won't go into detail about the algorithm. But what you are doing in the above two lines before the addition is just calculating what could simply be a constant. The compiler will probably even precompute the -305700999 * -1521134295 part. Multiplying the hash code with a prime number (like 23 or 31) should happen before every addition of another value.

\$\endgroup\$
  • \$\begingroup\$ +1 Now that you mention public fields, one should be very careful when deriving a hash code from mutable fields (or properties). It can cause subtle problems such as dictionary or hash set lookups failing. \$\endgroup\$ – Pieter Witvoet Dec 22 '17 at 11:53
  • \$\begingroup\$ Thanks for that public fields thing... I'm just being lazy here. I never actually change those values, but nothing in my code prevents it. \$\endgroup\$ – S. Buda Dec 22 '17 at 15:51
  • \$\begingroup\$ Originally I had this as a struct, but I changed it to a class at some point. I can't remember why, but at the time I thought that I had a good reason. I'll check back over my code and see if I was just making dumb. \$\endgroup\$ – S. Buda Dec 22 '17 at 15:52
  • \$\begingroup\$ Another question, as I am still fairly new to C# (Just made the jump over from Java). With my 3 sorted points, since I only ever get these points, would it be better to overload a getter, have an accessor function or even override [] for index access on the points? \$\endgroup\$ – S. Buda Dec 22 '17 at 15:59
  • \$\begingroup\$ @S.Buda You could add a property that gives you an IEnumerable so that the order doesn't matter. Then the caller can iterate over the points, for example to draw them on a canvas. \$\endgroup\$ – Raimund Krämer Dec 23 '17 at 13:05
-3
\$\begingroup\$

Overall this looks good, but I have one question, why don't you write this as

public override int GetHashCode() {
    var hashCode = -305700999 * -1521134295;
    hashCode += (EqualityComparer<Vector2>.Default.GetHashCode (point1)
        + EqualityComparer<Vector2>.Default.GetHashCode (point2)
        + EqualityComparer<Vector2>.Default.GetHashCode (point3));
    return hashCode;
}

This should be the same, and it seems cleaner. Also, what are those random constants?

\$\endgroup\$
  • \$\begingroup\$ This isn't really an improvement as it repeats the same mistakes as OP's code. \$\endgroup\$ – t3chb0t Dec 22 '17 at 7:31
  • 3
    \$\begingroup\$ Also, answers shouldn't be questions. Asking for further information should be done in comments on the question. \$\endgroup\$ – Raimund Krämer Dec 22 '17 at 9:46

Not the answer you're looking for? Browse other questions tagged or ask your own question.