Is it ok to have a GUID private property in a class in order to use it in GetHashCode override?

Question

Is it OK to have a GUID private property in a class in order to use it in GetHashCode override?

Something like:

public class Voucher : IComparable<Voucher>, IComparable, IEquatable<Voucher>
{
    private Guid? _guid;


    private Guid Guid
    {
        get
        {
            return _guid ?? (_guid = Guid.NewGuid()).GetValueOrDefault();
        }
    }
    public int Id { get; private set; }
    public string Number { get; private set; }
    public DateTime Date { get; private set; }



    public Voucher(string number, DateTime date)
    {
        Number = number;
        Date = date;
    }

    public Voucher(int id, string number, DateTime date)
        : this(number, date)
    {
        Id = id;
    }



    public override bool Equals(object obj)
    {
        return Equals(obj as Voucher);
    }

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

    public override string ToString()
    {
        return String.Format("[{0}] - [{1:dd/MM/yyyy}]", Number, Date);
    }



    #region IComparable<Voucher> Members

    public int CompareTo(Voucher other)
    {
        if (other == null)
            return -1;

        if (Date != other.Date)
            return Date.CompareTo(other.Date);
        else
            return Number.CompareTo(other.Number);
    }

    #endregion

    #region IComparable Members

    public int CompareTo(object obj)
    {
        return CompareTo(obj as Voucher);
    }

    #endregion

    #region IEquatable<Voucher> Members

    public bool Equals(Voucher other)
    {
        if (other != null)
            return (Number == other.Number) && (Date == other.Date);

        return false;
    }

    #endregion
}

Yesterday I found out that in order to override GetHashCode we have to use only immutable members/fields of the class.

For many of my cases that is only the Id that is produced by identity of the Sql Server and for new instances that is 0.

So for many new objects (not persisted to database thus Id is 0) object hash code is the same. Correct?

Would it be a solution to use GUID like the example above? Thanks.

EDIT Class after comments

So after your comments I've changed it to:

public class Voucher : IComparable<Voucher>, IComparable, IEquatable<Voucher>
    {
        public int Id { get; private set; }
        public string Number { get; private set; }
        public DateTime Date { get; private set; }



        public Voucher(string number, DateTime date)
        {
            Number = number;
            Date = date;
        }

        public Voucher(int id, string number, DateTime date)
            : this(number, date)
        {
            Id = id;
        }



        public override bool Equals(object obj)
        {
            return Equals(obj as Voucher);
        }

        public override int GetHashCode()
        {
            return Number.GetHashCode() ^ Date.GetHashCode();
        }

        public override string ToString()
        {
            return String.Format("[{0}] - [{1:dd/MM/yyyy}]", Number, Date);
        }



        #region IComparable<Voucher> Members

        public int CompareTo(Voucher other)
        {
            if (other == null)
                return -1;

            if (Date != other.Date)
                return Date.CompareTo(other.Date);
            else
                return Number.CompareTo(other.Number);
        }

        #endregion

        #region IComparable Members

        public int CompareTo(object obj)
        {
            return CompareTo(obj as Voucher);
        }

        #endregion

        #region IEquatable<Voucher> Members

        public bool Equals(Voucher other)
        {
            if (other != null)
                return (Number == other.Number) && (Date == other.Date);

            return false;
        }

        #endregion
    }

I guess that this is OK since Voucher is immutable.

But if members Number and Date were not immutable and could be accessed - altered outside the class? Then what is the solution? Is it enough just to document the class something like "Cannot be used in HashCode depended Lists"?

Answer 1

No, it is not okay to use a GUID in this way as it breaks what GetHashCode() is meant to do, which is calculate a hash of the contents of the object where if two objects have the same content, they will have the same hash.

You should rather implement GetHashCode() like in this question : SO - What is the best algorithm for GetHashCode? You should take the entire contents of the object into account for the hash.

The relevant code from the above link is:

public override int GetHashCode()
{
    unchecked // Overflow is fine, just wrap
    {
        int hash = 17;
        // Suitable nullity checks etc, of course :)
        hash = hash * 23 + field1.GetHashCode();
        hash = hash * 23 + field2.GetHashCode();
        hash = hash * 23 + field3.GetHashCode();
        return hash;
    }
}

Answer 2

Using a Guid is unnecessary as others have mentioned. But I think I understand the struggle in terms of comparing unpersisted objects. We use three levels when comparing objects:

AreSame() = represented by being the same space in memory. We don't really use a method here because 'x == y' does this nicely.

AreEqual() = equality, for us, is defined by having the same Id, including 0. If the id is default(int) then we refer to it as 'empty'. So much of the time we're testing for new objects with a method 'IsNullOrEmpty()' which nicely describes an object that either doesn't exist, or an object that is fresh and hasn't yet been persisted.

//querying distinct persisted vouchers
var vouchers = vouchers.Where(w=>!w.IsNullOrEmpty()).Distinct();

AreEquivalent() - This is based on the individual properties of an object (e.g. a composite key) and is very subjective to the object. For instance, if your number/date represented a distinct voucher, then that would be used for equivalency. You can use an anonymous object or something here to keep it clear.

//(warning: handle nulls appropriately, ideally by creating a better equalitycomparer here.).
    public override bool AreEquivalent(Voucher voucher){
    var propsAsAnonymous = v=>new{v.Number,v.Date};

    return propsAsAnonymous(this).Equals(propsAsAnonymous(voucher));
    }

Answer 3

No.

The contract specifies that two equal objects should have the same hashCode.

In your case, if you create two objects with the same contents, your hashcode will be different for both instances. This breaks the contract and potentially the behaviour of components that rely on hashCode.

Also, when you override GetHashCode(), you must also override Equals(object).

Answer 4

When overriding Equals and GetHashCode the rule that you should follow is that if two instances are equal then they should have the same hash code. You are violating that rule by creating unique hash codes for instances that are equal. This will cause issues with collections like Dictionary and HashSet that rely on GetHashCode returning the same value for equal items.

Answer 5

Even though everyone else sais it's not OK, I'd go for 'Well, that depends'.

One thing where GUID's are useful are for distributed systems. After all, they're globally unique identifiers - so if you are checking for equality across the boundaries of multiple processes / instances / persistency / etc and passing objects across those boundaries, I'd say, you're doing the right thing.

I regularly use (sequential) GUID's as ID's for databases as well. While most DBA's don't like this for performance reasons, it has the benefit that you don't need to check before doing an insert, thereby saving a network roundtrip. Personally, I believe this is the future 'best practice' for database keys. Still, I acknowledge that that's very debatable, and currently it's not considered a good practice I suppose.

That said, you're probably not after these kinds of things. :-)

If you just want to check for equality in the instance of your program, you should consider what you want to achieve. If you want to group instances by database ID (f.ex. to check for conflicts), you want to create equality using the key members (ID is probably sufficient in this case since it seems to be a database record on 1 database instance).

If you want unique objects in your application, you can implement equality by yourself (note: the default implementation of Object already works like this). The way to do that is to use RuntimeHelpers.GetHashCode(this) and Object.ReferenceEquals(this, o);. This basically uses the pointers for comparison.

To summarize: What you're after depends on the implementation. Usually, you want equality because you're f.ex. filling a Dictionary or a HashSet. This also requires you override both Equals and GetHashCode. The implementation you should use is the one that makes most sense in that context.