0

I have a class heirarchy as follows:

public abstract class Issue
{
    // some base properties
    // these are not included in equality checks for deriving classes
    // also not performing equality checks on this base class (it is abstract anyway)
}

public class IssueTypeA : Issue, IEquatable<IssueTypeA>
{
    // some properties specific to this class
    // these are the two for which equality comparison is performed
    public string requirement { get; set; }
    public string preamble { get; set; }

    public bool Equals(IssueTypeA that)
    {
        // determine based on the values of the properties
        // they must both be the same for equality
        if ((this.requirement.Equals(that.requirement)) &&
           (this.preamble.Equals(that.preamble)))
        {
            return true;
        }

        return false;
    }
}

public class IssueTypeB : Issue, IEquatable<IssueTypeB>
{
    // some properties specific to this class

    public bool Equals(IssueTypeB that)
    {
        // determine based on the values of the properties
    }
}

There is another class, intended to receive objects in the above heirarchy (except the base class, of course), and do some comparison operations with them:

public class Comparer<T> where T : Issue
{
    // various comparison methods
    public IEnumerable<T> getReferenceChangedIssues(IEnumerable<T> spreadsheetIssues, IEnumerable<T> downloadedIssues)
    {
        foreach (T spreadsheetRecord in T spreadsheetIssues)
        {
            foreach (T downloadedIssue in downloadedIssues)
            {
                // this is the point of failure
                // there are cases in which this should be true, but it is not
                if (spreadsheetRecord.Equals(downloadedIssue))
                {
                    // the referenceChanged method works fine by itself
                    // it has been unit tested
                    if (spreadsheetRecord.referenceChanged(downloadedIssue))
                        yield return spreadsheetRecord;
                }
            }
        }
    }
}

Through unit testing and debugging, it's apparent that the custom Equals methods defined above are not being used correctly by Comparer. Why is this? It is also intended to include methods with some set operations in the future, which will need Equals.

Al2110
  • 566
  • 9
  • 25
  • What if to Compare you're given one `IssueTypeA` and one `IssueTypeB`, which `Equals` method would it call? – Yuriy Faktorovich May 05 '20 at 04:43
  • @YuriyFaktorovich I have not tried that. I will do, but in normal circumstances two different subtypes of `Issue` would not be mixed. Perhaps generics are not suitable here. – Al2110 May 05 '20 at 04:49
  • If you implement *IEquatable* you should also override *Object.Equals* and *Object.GetHashCode*. What methods *Comparer* include? Does it implement interface *IComparer*? What problems do you face in the methods of *Comparer*? – Iliar Turdushev May 05 '20 at 04:52
  • @IliarTurdushev I will add an example of a method included in `Comparer`. – Al2110 May 05 '20 at 04:55
  • @Al2110 Now I see that the problem in the definition of the class `Comparer`. When you call `spreadsheetRecord.Equals(downloadedIssue)` a method `Object.Equals` is invoked (instead of `IEquatable.Equals`), because type `T` of `Comparer` does not constrained to implement `IEquatable`. Change the definition of the `Comparer` to the next: `class Comparer where T : Issue, IEquatable`. And also consider overriding `Object.Equals` and `Object.GetHashCode` in your `Issue` classes. – Iliar Turdushev May 05 '20 at 05:08

1 Answers1

0

When you implement IEquatable, you really need to override bool Equals(object) and GetHashCode. If you don't do that, things will break in weird ways. This was the stripped-down-est thing I could come up with. I think it should solve your issue (but I can't really tell without your comparison code). If folks point out issues, I'll fix up the code:

public abstract class Issue : IEquatable<Issue>
{
    // possibly (not not necessarily) some base properties
    // if they are included, make them part of the equality comparison and
    // include them in the HashCode calculations

    public bool Equals(Issue other)
    {
        if (other == null)
        {
            return false;
        }
        if (this.GetType() != other.GetType())
        {
            return false;
        }
        //test local properties and return true/false based on their values
        return true;
    }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        if (obj.GetType() != this.GetType()) return false;
        return Equals((Issue) obj);
    }

}

public class IssueTypeA : Issue, IEquatable<IssueTypeA>
{
    public string AName { get; set; } = "My A Name";
    public int AValue { get; set; } = 42;

    public bool Equals(IssueTypeA other)
    {
        if (!base.Equals(other))
        {
            return false;
        }

        return AName == other.AName && AValue == other.AValue;
    }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        if (obj.GetType() != this.GetType()) return false;
        return Equals((IssueTypeA) obj);
    }


    //hash-codes need to be consistent once they are calculated - jump through some hoops
    private int? _hashCodeValue = default;
    public override int GetHashCode()
    {
        if (!_hashCodeValue.HasValue)
        {
            _hashCodeValue = $"{AName}{AValue}".GetHashCode();
        }

        return _hashCodeValue.Value;
    }
}

public class IssueTypeB : Issue, IEquatable<IssueTypeB>
{
    // some properties specific to this class
    public string BName { get; set; } = "My B Name";
    public int BValue { get; set; } = 84;



    public bool Equals(IssueTypeB other)
    {
        if (!base.Equals(other))
        {
            return false;
        }
        //calculate equality based on local properties and return true/false
        return BName == other.BName && BValue == other.BValue;
    }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        if (obj.GetType() != this.GetType()) return false;
        return Equals((IssueTypeB) obj);
    }

    //hash-codes need to be consistent once they are calculated - jump through some hoops
    private int? _hashCodeValue = default;
    public override int GetHashCode()
    {
        if (!_hashCodeValue.HasValue)
        {
            _hashCodeValue = $"{BName}{BValue}".GetHashCode();
        }

        return _hashCodeValue.Value;
    }
}

This is a better stab at a solution than the first pass. I check for equality based on properties local to each class. I also provide a GetHashCode implementation that is consistent (in the weird way that I believe GetHashCode requires - but in a way that drive Resharper nuts).

Flydog57
  • 6,851
  • 2
  • 17
  • 18
  • I added to the original post, the properties for which the equality comparison for `IssueTypeA` is based. I see you gave an example in your answer for the case that base properties are used in the comparison. In my case, they are not. Do I still need the equals and get hash code methods in the base class? – Al2110 May 05 '20 at 06:34