3
\$\begingroup\$

Is this a good implementation for Equals and GetHashCode for a base class in C#? If it's not good enough, can you suggest improvements for it, please?

public abstract class Entity<TKey>    //TKey = Type of the Key
{
    private string FullClassName;
    private bool KeyIsNullable;
    private Type BaseClassType;
    private bool KeyIsComplex;

    public abstract TKey Key { get; }    //Key of the object, which determine it's uniqueness

    public Entity()
    {
        FullClassName = GetType().FullName + "#";
        KeyIsNullable = typeof(TKey).IsAssignableFrom(typeof(Nullable));
        BaseClassType = typeof(Entity<TKey>);
        KeyIsComplex= !typeof(TKey).IsPrimitive;
    }

    public override bool Equals(object obj)
    {
        bool result = BaseClassType.IsAssignableFrom(obj.GetType());
        result
            = result
                && (
                        (
                            (
                                !KeyIsNullable
                                ||
                                (Key != null && ((Entity<TKey>)obj).Key != null
                            )
                        )
                        &&
                        Key.Equals(((Entity<TKey>)obj).Key )
                    )    // The key is not nullable, or (it's nullable but) both aren't null, and also equal
                    ||
                    (
                        KeyIsNullable
                        &&
                        Key == null
                        &&
                        ((Entity<TKey>)obj).Key == null
                    )
                ); // Or the key is nullable, and both are null
        return result;
    }

    public override int GetHashCode()
    {
        if ((KeyIsNullable&& Key == null) || (!KeyIsNullable&& Key .Equals(default(TKey))))
        {
            return base.GetHashCode();
        }
        string stringRepresentation = FullClassName + ((KeyIsComplex)? Key.GetHashCode().ToString() : Key.ToString());
        return stringRepresentation.GetHashCode();
    }
}

Example of a derived class:

public class Foo : Entity<int>
{
    public virtual int FooId { set; get; }
    public virtual string FooDescription { set; get; }

    public override int Key { get { return FooId; } }
}

Specific and special details to the proposal:

  1. Any instance of a derived class is considered equal to a instance of the base class if they have the same key.
  2. The key could be null.
  3. If the key of the current class and the key of the comparing object are both null, the two objects are considered equal. This is because I am planning to handle just one new object at a time, and if the key is nullable, it will be null for the new object. So if I have two instances with a null key, I will consider them as the same entity.
\$\endgroup\$
  • 1
    \$\begingroup\$ When you're having to indent your parenthesis, then it should be obvious that you should rewrite your code instead. Having everything in a single huge expression is not a good thing. \$\endgroup\$ – svick Jun 15 '13 at 0:03
  • \$\begingroup\$ Hehehe, you are right. It's kind of a nested if-else, but without the if-else. \$\endgroup\$ – Guillermo Gutiérrez Jun 15 '13 at 6:21
  • \$\begingroup\$ I would not override equality at all for entities. I'd define an IEqualityComparer<T> that compares by Id, but I'd avoid making it the default. \$\endgroup\$ – CodesInChaos Jun 16 '13 at 16:24
  • \$\begingroup\$ “Any instance of a derived class is considered equal to a instance of the base class if they have the same key.” Does that mean that new Foo { FooId = 42 }.Equals(new Bar { BarId = 42 }) should return true? That would be very weird. \$\endgroup\$ – svick Jun 16 '13 at 19:48
  • \$\begingroup\$ It's because it is a base class for NHibernate entities, and sometimes I work with stub objects and NHibernate proxies at the same time. For example, sometimes I create a new stub, and need to replace the object with the same key in a list of proxies. \$\endgroup\$ – Guillermo Gutiérrez Jun 16 '13 at 22:43
3
\$\begingroup\$
//TKey = Type of the Key
//Key of the object, which determine it's uniqueness

If you're going to use comments like these, it's useful to use XML documentation comments instead, so that you can see them in IntelliSense.

Also, you should try to keep your comments grammatically correct, though I understand that's not always easy, especially if you're not a native speaker.

public Entity()
{
    FullClassName = GetType().FullName + "#";
    KeyIsNullable = typeof(TKey).IsAssignableFrom(typeof(Nullable));
    BaseClassType = typeof(Entity<TKey>);
    KeyIsComplex = !typeof(TKey).IsPrimitive;
}

I think it doesn't make much sense to store these in each instance. If your profiling shows that retrieving these values on each call slows down your code, store at least the last three values in static fields (you could initialize them from a static constructor).

Also, KeyIsNullable will be always false, because Nullable is a static class that's distinct from Nullable<T>. (But it doesn't matter anyway, see below.)

bool result = BaseClassType.IsAssignableFrom(obj.GetType());

According to ReSharper, you could instead use BaseClassType.IsInstanceOfType(obj). (I had no idea such method existed.)

But this check means that for example two different types deriving from Entity<int> with the same key will compare as equal. I don't think that's what you want, you should compare the type against GetType() and you should make sure that the types are exactly equal. This is especially true since in such case, the two objects will compare as equal, but will have different hash codes, which makes your code wrong.

Also, your code will throw an exception when obj is null, you should add a check against that.

result
    = result
        && ( …

This monstrous expression doesn't make much sense to me (and that's not just because it's hard to understand). For nullable value types, Equals() works fine, you don't need all this gymnastics.

if ((KeyIsNullable&& Key == null) || (!KeyIsNullable&& Key .Equals(default(TKey))))
{
    return base.GetHashCode();
}

This code indicates that if the Key has its default value, you want to use reference equality. But there is no indication of that in your Equals(). You have to decide what exactly does equal mean for your type and keep that definition consistent across Equals() and GetHashCode().

Also, again, you don't need special code for nullable value types.

string stringRepresentation = FullClassName + ((KeyIsComplex)? Key.GetHashCode().ToString() : Key.ToString());
return stringRepresentation.GetHashCode();

I haven't seen hash code include the type before. It could make sense if you're using hash-based collections that can contain different types with the same key values, though doing that is not very common, I think.

Though if you want to do that, I would use the hash code of GetType() instead of dealing with type name.

Also, there is no reason to use strings here, simply combining the hash codes (e.g. using XOR) is enough.


With all these changes, your code will look like this:

/// <typeparam name="TKey">Type of the Key</typeparam>
public abstract class Entity<TKey>
{
    /// <summary>
    /// Key of the object, which determines its uniqueness
    /// </summary>
    public abstract TKey Key { get; }

    public override bool Equals(object obj)
    {
        if (obj == null)
            return false;

        if (obj.GetType() != GetType())
            return false;

        bool sameKey = Key.Equals(((Entity<TKey>)obj).Key);

        if (sameKey && Key.Equals(default(TKey)))
            return ReferenceEquals(this, obj);

        return sameKey;
    }

    public override int GetHashCode()
    {
        if (Key.Equals(default(TKey)))
            return base.GetHashCode();

        return GetType().GetHashCode() ^ Key.GetHashCode();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Many thanks @svick. It's a very detailed explanation. I will take all your valuable advices into account. I added more details to the question, regarding to the inheritance and key nullability, sorry because I didn't mentioned them before. PD: You are right, english it's not my native language, I had to translate my code, hehehe... \$\endgroup\$ – Guillermo Gutiérrez Jun 16 '13 at 3:07
  • \$\begingroup\$ One question: why using XOR? I have seen a couple of examples using it to generate a new hash code, but I didn't get why. Thanks in advance. \$\endgroup\$ – Guillermo Gutiérrez Jun 18 '13 at 21:26
  • \$\begingroup\$ @guillegr123 Because it's a simple and reasonable way to combine two unrelated hashcodes. XOR has its issues, so in general it's better to combine them using prime numbers and multiplication. But I think that in this specific case, XOR is okay. \$\endgroup\$ – svick Jun 18 '13 at 22:42
4
\$\begingroup\$

I think you are overcomplicating things you can say simple this:

    public abstract class Entity<TKey> where TKey : struct 
    {
        public abstract TKey Key { get; }

        protected bool Equals(Entity<TKey> other)
        {
            return Key.Equals(other.Key);
        }

        public override bool Equals(object obj)
        {
            if (ReferenceEquals(null, obj)) return false;
            if (ReferenceEquals(this, obj)) return true;
            var asThis = obj as Entity<TKey>;
            return asThis != null && Equals(asThis);
        }

        public override int GetHashCode()
        {
            return Key.GetHashCode();
        }
    }

No need the type magic A key cannot be null and it should be a ValueType An Equals and GetHashCode implementation should be symmetric and fast (your solution is slow and i'm not sure it is symmetric or not)

\$\endgroup\$
  • \$\begingroup\$ Thanks @Peter Kiss, I like the Hash Code based on the Keys HashCode. Also, I haven't considered the symmetric implementation between Equals and GetHashCode, I will review that. There is a caveat, though: the key can be null. \$\endgroup\$ – Guillermo Gutiérrez Jun 16 '13 at 3:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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