Base class for implementing IComparable
Clash Royale CLAN TAG#URR8PPP
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#
add a comment |
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#
add a comment |
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#
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#
c#
asked Dec 19 '18 at 14:50
Blackwood
1358
1358
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
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.
Thanks @petertaylor. I suppose I just assumed thatCompareTo(null)
should return a positive value. I'll take your advice and change theCompare
method.
– Blackwood
Dec 19 '18 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 '18 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 theArrayList
users call aSort
overload which takes anIComparer
.
– Peter Taylor
Dec 19 '18 at 20:26
That makes sense (and simplifies my code). Thanks.
– Blackwood
Dec 19 '18 at 20:32
add a comment |
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
.
How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (wherecomp1 Is Nothing
does not cause the error) to C#. I apologize.
– Blackwood
Dec 19 '18 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 '18 at 17:56
add a comment |
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
);
);
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
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.
Thanks @petertaylor. I suppose I just assumed thatCompareTo(null)
should return a positive value. I'll take your advice and change theCompare
method.
– Blackwood
Dec 19 '18 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 '18 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 theArrayList
users call aSort
overload which takes anIComparer
.
– Peter Taylor
Dec 19 '18 at 20:26
That makes sense (and simplifies my code). Thanks.
– Blackwood
Dec 19 '18 at 20:32
add a comment |
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.
Thanks @petertaylor. I suppose I just assumed thatCompareTo(null)
should return a positive value. I'll take your advice and change theCompare
method.
– Blackwood
Dec 19 '18 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 '18 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 theArrayList
users call aSort
overload which takes anIComparer
.
– Peter Taylor
Dec 19 '18 at 20:26
That makes sense (and simplifies my code). Thanks.
– Blackwood
Dec 19 '18 at 20:32
add a comment |
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.
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.
answered Dec 19 '18 at 15:41
Peter Taylor
15.7k2658
15.7k2658
Thanks @petertaylor. I suppose I just assumed thatCompareTo(null)
should return a positive value. I'll take your advice and change theCompare
method.
– Blackwood
Dec 19 '18 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 '18 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 theArrayList
users call aSort
overload which takes anIComparer
.
– Peter Taylor
Dec 19 '18 at 20:26
That makes sense (and simplifies my code). Thanks.
– Blackwood
Dec 19 '18 at 20:32
add a comment |
Thanks @petertaylor. I suppose I just assumed thatCompareTo(null)
should return a positive value. I'll take your advice and change theCompare
method.
– Blackwood
Dec 19 '18 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 '18 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 theArrayList
users call aSort
overload which takes anIComparer
.
– Peter Taylor
Dec 19 '18 at 20:26
That makes sense (and simplifies my code). Thanks.
– Blackwood
Dec 19 '18 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 '18 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 '18 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 '18 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 '18 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 '18 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 '18 at 20:26
That makes sense (and simplifies my code). Thanks.
– Blackwood
Dec 19 '18 at 20:32
That makes sense (and simplifies my code). Thanks.
– Blackwood
Dec 19 '18 at 20:32
add a comment |
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
.
How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (wherecomp1 Is Nothing
does not cause the error) to C#. I apologize.
– Blackwood
Dec 19 '18 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 '18 at 17:56
add a comment |
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
.
How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (wherecomp1 Is Nothing
does not cause the error) to C#. I apologize.
– Blackwood
Dec 19 '18 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 '18 at 17:56
add a comment |
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
.
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
.
answered Dec 19 '18 at 17:09
t3chb0t
34.1k746114
34.1k746114
How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (wherecomp1 Is Nothing
does not cause the error) to C#. I apologize.
– Blackwood
Dec 19 '18 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 '18 at 17:56
add a comment |
How embarrassing. I had not tested the equality operator since I converted the code from VB.Net (wherecomp1 Is Nothing
does not cause the error) to C#. I apologize.
– Blackwood
Dec 19 '18 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 '18 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 '18 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 '18 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 '18 at 17:56
@Blackwood yeah, VB.NET is full of surprises by doing things differently than the rest of the world ;-)
– t3chb0t
Dec 19 '18 at 17:56
add a comment |
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.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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