Gfrktyl

If I create a class that implements IComparable<T>, I must implement CompareTo<T>. It is also recommended that I implement IEquatable<T> and the non-generic IComparable. If I do all that, I am required or encouraged to:

• Override GetHashCode()
  


• Implement CompareTo(Object)
  


• Override Equals(Object)
  


• Implement Operator ==(T, T)


• Implement Operator !=(T, T)


• Implement Operator >(T, T)


• Implement Operator <(T, T)


• Implement Operator >=(T, T)


• Implement Operator <=(T, T)

That's 9 additional methods, most of which depend on the logic that compares two instances of the class. Rather than having to implement all those methods in any class that implements IComparable<T>, I decided to create a base class that implements IComparable<T> and the other recommended interfaces (similar to the way Microsoft provides Comparer as a base class for implementations of IComparer<T>)

It doesn't make sense to compare instances of two different classes that each inherit from the base class. preventing that was the main reason for making the class generic (although it makes coding a derived class a little more complicate).

I would like to ask for a review of the code for the base class. Am I missing something? Can it be simplified? Is this a bad idea?

Here is the base class

public abstract class Comparable<T> : IComparable, IComparable<T>, IEquatable<T> where T: Comparable<T> {

    public abstract override int GetHashCode();



    public abstract int CompareTo(T other);



    public int CompareTo(object obj) {

        T other = obj as T;

        if (other == null && obj != null) {

            throw new ArgumentException($"Objects of type {typeof(T).Name} can only be compared to objects of the same type", nameof(obj));

        }

        return CompareTo(other);

    }



    public override bool Equals(object obj) {

        return CompareTo(obj) == 0;

    }



    new public bool Equals(T other) {

        return CompareTo(other) == 0;

    }



    private static int Compare(Comparable<T> comp1, Comparable<T> comp2) {

        if (comp1 == null) {

            return ((comp2 == null) ? 0 : -1);

        }

        return comp1.CompareTo(comp2);

    }



    public static bool operator == (Comparable<T> comp1, Comparable<T> comp2) {

        return Compare(comp1, comp2) == 0;

    }



    public static bool operator != (Comparable<T> comp1, Comparable<T> comp2) {

        return Compare(comp1, comp2) != 0;

    }



    public static bool operator > (Comparable<T> comp1, Comparable<T> comp2) {

        return Compare(comp1, comp2) > 0;

    }



    public static bool operator < (Comparable<T> comp1, Comparable<T> comp2) {

        return Compare(comp1, comp2) < 0;

    }



    public static bool operator >= (Comparable<T> comp1, Comparable<T> comp2) {

        return Compare(comp1, comp2) >= 0;

    }



    public static bool operator <= (Comparable<T> comp1, Comparable<T> comp2) {

        return Compare(comp1, comp2) <= 0;

    }

}

Below is a minimal implementation of the base class.

public class SeasonCompare : Comparable<SeasonCompare> {

    public int Number {get; set;}



    public override int GetHashCode() {

        return Number;

    }



    public override int CompareTo(SeasonCompare other) {

        if (other == null) {

            return 1;

        }

        return Number.CompareTo(other.Number);

    }

}

It is also recommended that I implement ... the non-generic IComparable.

I don't see that recommendation in the current doc for IComparable. I would recommend against it: having the non-generic method turns compile-time errors into runtime errors, which are more expensive to find and fix.

      private static int Compare(Comparable<T> comp1, Comparable<T> comp2) {

          if (comp1 == null) {

              return ((comp2 == null) ? 0 : -1);

          }

          return comp1.CompareTo(comp2);

      }

For consistency, this requires that subclasses guarantee that CompareTo(null) returns a positive value, but that requirement isn't documented.

Perhaps a better solution would be:

        private static int Compare(Comparable<T> comp1, Comparable<T> comp2) {

            if (comp1 == null) {

                return comp2 == null ? 0 : 0.CompareTo(comp2.CompareTo(comp1));

            }

            return comp1.CompareTo(comp2);

        }

That way the only requirement is that CompareTo(null) be consistent.

There may be a more elegant way of inverting the sense of a comparison, but that's the easiest one I can think of which doesn't fail on corner cases.

Stack Overflow Exception

You did not test your code appropriately because it throws the StackOverflowException for

(x == y)

This is the rare case when this happens and is triggered by these two methods.

public static bool operator ==(Comparable<T> comp1, Comparable<T> comp2)

{

  return Compare(comp1, comp2) == 0;

}

This calls the Compare method which in turn calls comp1 == null which means that Compare is called... and so on...

private static int Compare(Comparable<T> comp1, Comparable<T> comp2)

{

  if (comp1 == null)

  {

      return ((comp2 == null) ? 0 : -1);

  }

  return comp1.CompareTo(comp2);

}

When implementing comparers or equalities you should always use object.ReferenceEquals for checking arguments against null and never == null.

You should also use standard names for parameters like x & y or left & right and not comp1 and comp2.