Base class for implementing IComparable












6














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);
}
}









share|improve this question







New contributor




Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.

























    6














    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);
    }
    }









    share|improve this question







    New contributor




    Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
    Check out our Code of Conduct.























      6












      6








      6


      1





      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);
      }
      }









      share|improve this question







      New contributor




      Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.











      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);
      }
      }






      c#






      share|improve this question







      New contributor




      Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.











      share|improve this question







      New contributor




      Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.









      share|improve this question




      share|improve this question






      New contributor




      Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.









      asked Dec 19 at 14:50









      Blackwood

      1357




      1357




      New contributor




      Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.





      New contributor





      Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






      Blackwood is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






















          2 Answers
          2






          active

          oldest

          votes


















          7















          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.






          share|improve this answer





















          • Thanks @petertaylor. I suppose I just assumed that CompareTo(null) should return a positive value. I'll take your advice and change the Compare method.
            – Blackwood
            Dec 19 at 17:23










          • On the subject of implementing IComparable, it is true that this is not suggested in the documentation. I got the recommendation from a Stack Overflow answer (that I now can't find) that argued for implementing IComparable in order to avoid problems when sorting an ArrayList containing objects of the class. If I don't implement IComparable, I get an InvalidOperationException when sorting an ArrayList containing objects of my class. I wouldn't use an ArrayList myself, but I suppose I should allow for people who would. Do you still recommend against implementing IComparable?
            – Blackwood
            Dec 19 at 17:34










          • @Blackwood, I see it as a tradeoff between a little pain for people who chose runtime checks over compile-time checks by not upgrading to Net Framework 2.0 vs a little pain for everyone else. As far as I'm concerned, let the ArrayList users call a Sort overload which takes an IComparer.
            – Peter Taylor
            Dec 19 at 20:26










          • That makes sense (and simplifies my code). Thanks.
            – Blackwood
            Dec 19 at 20:32



















          7














          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.






          share|improve this answer





















          • How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (where comp1 Is Nothing does not cause the error) to C#. I apologize.
            – Blackwood
            Dec 19 at 17:54






          • 1




            @Blackwood yeah, VB.NET is full of surprises by doing things differently than the rest of the world ;-)
            – t3chb0t
            Dec 19 at 17:56











          Your Answer





          StackExchange.ifUsing("editor", function () {
          return StackExchange.using("mathjaxEditing", function () {
          StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
          StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
          });
          });
          }, "mathjax-editing");

          StackExchange.ifUsing("editor", function () {
          StackExchange.using("externalEditor", function () {
          StackExchange.using("snippets", function () {
          StackExchange.snippets.init();
          });
          });
          }, "code-snippets");

          StackExchange.ready(function() {
          var channelOptions = {
          tags: "".split(" "),
          id: "196"
          };
          initTagRenderer("".split(" "), "".split(" "), channelOptions);

          StackExchange.using("externalEditor", function() {
          // Have to fire editor after snippets, if snippets enabled
          if (StackExchange.settings.snippets.snippetsEnabled) {
          StackExchange.using("snippets", function() {
          createEditor();
          });
          }
          else {
          createEditor();
          }
          });

          function createEditor() {
          StackExchange.prepareEditor({
          heartbeatType: 'answer',
          autoActivateHeartbeat: false,
          convertImagesToLinks: false,
          noModals: true,
          showLowRepImageUploadWarning: true,
          reputationToPostImages: null,
          bindNavPrevention: true,
          postfix: "",
          imageUploader: {
          brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
          contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
          allowUrls: true
          },
          onDemand: true,
          discardSelector: ".discard-answer"
          ,immediatelyShowMarkdownHelp:true
          });


          }
          });






          Blackwood is a new contributor. Be nice, and check out our Code of Conduct.










          draft saved

          draft discarded


















          StackExchange.ready(
          function () {
          StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209976%2fbase-class-for-implementing-icomparablet%23new-answer', 'question_page');
          }
          );

          Post as a guest















          Required, but never shown

























          2 Answers
          2






          active

          oldest

          votes








          2 Answers
          2






          active

          oldest

          votes









          active

          oldest

          votes






          active

          oldest

          votes









          7















          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.






          share|improve this answer





















          • Thanks @petertaylor. I suppose I just assumed that CompareTo(null) should return a positive value. I'll take your advice and change the Compare method.
            – Blackwood
            Dec 19 at 17:23










          • On the subject of implementing IComparable, it is true that this is not suggested in the documentation. I got the recommendation from a Stack Overflow answer (that I now can't find) that argued for implementing IComparable in order to avoid problems when sorting an ArrayList containing objects of the class. If I don't implement IComparable, I get an InvalidOperationException when sorting an ArrayList containing objects of my class. I wouldn't use an ArrayList myself, but I suppose I should allow for people who would. Do you still recommend against implementing IComparable?
            – Blackwood
            Dec 19 at 17:34










          • @Blackwood, I see it as a tradeoff between a little pain for people who chose runtime checks over compile-time checks by not upgrading to Net Framework 2.0 vs a little pain for everyone else. As far as I'm concerned, let the ArrayList users call a Sort overload which takes an IComparer.
            – Peter Taylor
            Dec 19 at 20:26










          • That makes sense (and simplifies my code). Thanks.
            – Blackwood
            Dec 19 at 20:32
















          7















          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.






          share|improve this answer





















          • Thanks @petertaylor. I suppose I just assumed that CompareTo(null) should return a positive value. I'll take your advice and change the Compare method.
            – Blackwood
            Dec 19 at 17:23










          • On the subject of implementing IComparable, it is true that this is not suggested in the documentation. I got the recommendation from a Stack Overflow answer (that I now can't find) that argued for implementing IComparable in order to avoid problems when sorting an ArrayList containing objects of the class. If I don't implement IComparable, I get an InvalidOperationException when sorting an ArrayList containing objects of my class. I wouldn't use an ArrayList myself, but I suppose I should allow for people who would. Do you still recommend against implementing IComparable?
            – Blackwood
            Dec 19 at 17:34










          • @Blackwood, I see it as a tradeoff between a little pain for people who chose runtime checks over compile-time checks by not upgrading to Net Framework 2.0 vs a little pain for everyone else. As far as I'm concerned, let the ArrayList users call a Sort overload which takes an IComparer.
            – Peter Taylor
            Dec 19 at 20:26










          • That makes sense (and simplifies my code). Thanks.
            – Blackwood
            Dec 19 at 20:32














          7












          7








          7







          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.






          share|improve this answer













          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.







          share|improve this answer












          share|improve this answer



          share|improve this answer










          answered Dec 19 at 15:41









          Peter Taylor

          15.7k2658




          15.7k2658












          • Thanks @petertaylor. I suppose I just assumed that CompareTo(null) should return a positive value. I'll take your advice and change the Compare method.
            – Blackwood
            Dec 19 at 17:23










          • On the subject of implementing IComparable, it is true that this is not suggested in the documentation. I got the recommendation from a Stack Overflow answer (that I now can't find) that argued for implementing IComparable in order to avoid problems when sorting an ArrayList containing objects of the class. If I don't implement IComparable, I get an InvalidOperationException when sorting an ArrayList containing objects of my class. I wouldn't use an ArrayList myself, but I suppose I should allow for people who would. Do you still recommend against implementing IComparable?
            – Blackwood
            Dec 19 at 17:34










          • @Blackwood, I see it as a tradeoff between a little pain for people who chose runtime checks over compile-time checks by not upgrading to Net Framework 2.0 vs a little pain for everyone else. As far as I'm concerned, let the ArrayList users call a Sort overload which takes an IComparer.
            – Peter Taylor
            Dec 19 at 20:26










          • That makes sense (and simplifies my code). Thanks.
            – Blackwood
            Dec 19 at 20:32


















          • Thanks @petertaylor. I suppose I just assumed that CompareTo(null) should return a positive value. I'll take your advice and change the Compare method.
            – Blackwood
            Dec 19 at 17:23










          • On the subject of implementing IComparable, it is true that this is not suggested in the documentation. I got the recommendation from a Stack Overflow answer (that I now can't find) that argued for implementing IComparable in order to avoid problems when sorting an ArrayList containing objects of the class. If I don't implement IComparable, I get an InvalidOperationException when sorting an ArrayList containing objects of my class. I wouldn't use an ArrayList myself, but I suppose I should allow for people who would. Do you still recommend against implementing IComparable?
            – Blackwood
            Dec 19 at 17:34










          • @Blackwood, I see it as a tradeoff between a little pain for people who chose runtime checks over compile-time checks by not upgrading to Net Framework 2.0 vs a little pain for everyone else. As far as I'm concerned, let the ArrayList users call a Sort overload which takes an IComparer.
            – Peter Taylor
            Dec 19 at 20:26










          • That makes sense (and simplifies my code). Thanks.
            – Blackwood
            Dec 19 at 20:32
















          Thanks @petertaylor. I suppose I just assumed that CompareTo(null) should return a positive value. I'll take your advice and change the Compare method.
          – Blackwood
          Dec 19 at 17:23




          Thanks @petertaylor. I suppose I just assumed that CompareTo(null) should return a positive value. I'll take your advice and change the Compare method.
          – Blackwood
          Dec 19 at 17:23












          On the subject of implementing IComparable, it is true that this is not suggested in the documentation. I got the recommendation from a Stack Overflow answer (that I now can't find) that argued for implementing IComparable in order to avoid problems when sorting an ArrayList containing objects of the class. If I don't implement IComparable, I get an InvalidOperationException when sorting an ArrayList containing objects of my class. I wouldn't use an ArrayList myself, but I suppose I should allow for people who would. Do you still recommend against implementing IComparable?
          – Blackwood
          Dec 19 at 17:34




          On the subject of implementing IComparable, it is true that this is not suggested in the documentation. I got the recommendation from a Stack Overflow answer (that I now can't find) that argued for implementing IComparable in order to avoid problems when sorting an ArrayList containing objects of the class. If I don't implement IComparable, I get an InvalidOperationException when sorting an ArrayList containing objects of my class. I wouldn't use an ArrayList myself, but I suppose I should allow for people who would. Do you still recommend against implementing IComparable?
          – Blackwood
          Dec 19 at 17:34












          @Blackwood, I see it as a tradeoff between a little pain for people who chose runtime checks over compile-time checks by not upgrading to Net Framework 2.0 vs a little pain for everyone else. As far as I'm concerned, let the ArrayList users call a Sort overload which takes an IComparer.
          – Peter Taylor
          Dec 19 at 20:26




          @Blackwood, I see it as a tradeoff between a little pain for people who chose runtime checks over compile-time checks by not upgrading to Net Framework 2.0 vs a little pain for everyone else. As far as I'm concerned, let the ArrayList users call a Sort overload which takes an IComparer.
          – Peter Taylor
          Dec 19 at 20:26












          That makes sense (and simplifies my code). Thanks.
          – Blackwood
          Dec 19 at 20:32




          That makes sense (and simplifies my code). Thanks.
          – Blackwood
          Dec 19 at 20:32













          7














          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.






          share|improve this answer





















          • How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (where comp1 Is Nothing does not cause the error) to C#. I apologize.
            – Blackwood
            Dec 19 at 17:54






          • 1




            @Blackwood yeah, VB.NET is full of surprises by doing things differently than the rest of the world ;-)
            – t3chb0t
            Dec 19 at 17:56
















          7














          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.






          share|improve this answer





















          • How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (where comp1 Is Nothing does not cause the error) to C#. I apologize.
            – Blackwood
            Dec 19 at 17:54






          • 1




            @Blackwood yeah, VB.NET is full of surprises by doing things differently than the rest of the world ;-)
            – t3chb0t
            Dec 19 at 17:56














          7












          7








          7






          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.






          share|improve this answer












          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.







          share|improve this answer












          share|improve this answer



          share|improve this answer










          answered Dec 19 at 17:09









          t3chb0t

          34k746112




          34k746112












          • How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (where comp1 Is Nothing does not cause the error) to C#. I apologize.
            – Blackwood
            Dec 19 at 17:54






          • 1




            @Blackwood yeah, VB.NET is full of surprises by doing things differently than the rest of the world ;-)
            – t3chb0t
            Dec 19 at 17:56


















          • How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (where comp1 Is Nothing does not cause the error) to C#. I apologize.
            – Blackwood
            Dec 19 at 17:54






          • 1




            @Blackwood yeah, VB.NET is full of surprises by doing things differently than the rest of the world ;-)
            – t3chb0t
            Dec 19 at 17:56
















          How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (where comp1 Is Nothing does not cause the error) to C#. I apologize.
          – Blackwood
          Dec 19 at 17:54




          How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (where comp1 Is Nothing does not cause the error) to C#. I apologize.
          – Blackwood
          Dec 19 at 17:54




          1




          1




          @Blackwood yeah, VB.NET is full of surprises by doing things differently than the rest of the world ;-)
          – t3chb0t
          Dec 19 at 17:56




          @Blackwood yeah, VB.NET is full of surprises by doing things differently than the rest of the world ;-)
          – t3chb0t
          Dec 19 at 17:56










          Blackwood is a new contributor. Be nice, and check out our Code of Conduct.










          draft saved

          draft discarded


















          Blackwood is a new contributor. Be nice, and check out our Code of Conduct.













          Blackwood is a new contributor. Be nice, and check out our Code of Conduct.












          Blackwood is a new contributor. Be nice, and check out our Code of Conduct.
















          Thanks for contributing an answer to Code Review Stack Exchange!


          • Please be sure to answer the question. Provide details and share your research!

          But avoid



          • Asking for help, clarification, or responding to other answers.

          • Making statements based on opinion; back them up with references or personal experience.


          Use MathJax to format equations. MathJax reference.


          To learn more, see our tips on writing great answers.





          Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


          Please pay close attention to the following guidance:


          • Please be sure to answer the question. Provide details and share your research!

          But avoid



          • Asking for help, clarification, or responding to other answers.

          • Making statements based on opinion; back them up with references or personal experience.


          To learn more, see our tips on writing great answers.




          draft saved


          draft discarded














          StackExchange.ready(
          function () {
          StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209976%2fbase-class-for-implementing-icomparablet%23new-answer', 'question_page');
          }
          );

          Post as a guest















          Required, but never shown





















































          Required, but never shown














          Required, but never shown












          Required, but never shown







          Required, but never shown

































          Required, but never shown














          Required, but never shown












          Required, but never shown







          Required, but never shown







          Popular posts from this blog

          Сан-Квентин

          Алькесар

          Josef Freinademetz