needs overload operator< and null check

Question

I´m overloading the lessthan-operator in c# and I`m wondering whether this needs to check for null. Below you can find an Example:

public static bool operator <(MyClass x, MyClass y)
{
  if (x == null && y == null)
  {
    return false;
  }
  if (x == null)
  {
    return true; //false?
  }
  if (y == null)
  {
    return false; //true?
  }
  return x.Value < y.Value;
}

Or is this correct:

public static bool operator <(MyClass x, MyClass y)
{
  return x.Value < y.Value;
}

I didn´t find any instruction on this. But maybe I missed something.

score 13 · Accepted Answer · edited Oct 27 '14 at 18:58

13

The answer depends on your intended usage pattern. If you plan to have nulls in the mix, and you would like to consider null values to be less than non-null values, then your implementation is correct; if you would like to consider null values to be greater than non-null objects, then the commented out return values (false and true) should be used instead. If you do not plan to allow nulls in the mix, throwing an ArgumentNullException or allowing NullReferenceException would be the right choice.

edited Oct 27 '14 at 18:58

Chris Nielsen

14,731
7
48
54

answered Mar 08 '12 at 13:41

Sergey Kalinichenko

714,442
84
1,110
1,523

2

Throw `ArgumentNullException` kids, never throw an intentional `NullReferenceException`. – Dagrooms May 04 '17 at 20:31
1

It's the difference between "I meant to do this" and "oops." – Dagrooms May 04 '17 at 20:31

score 5 · Answer 2 · answered Mar 08 '12 at 13:41

Both approaches are correct (for different values of correct).

If x or y are likely to be null and that has a valid meaning in your case then go with the first approach.

If x and y are highly unlikely to be null then go with the second and let any exceptions propagate to the calling code for handling.

score 3 · Answer 3 · answered Mar 08 '12 at 13:40

3

Personally I would throw a ArgumentNullException if either x or y are null, which should be an exceptional circumstance.

answered Mar 08 '12 at 13:40

mdm

12,480
5
34
53

6

`NullReferenceException` shouldn't be thrown from user code, it means there is a programming bug. Use `ArgumentNullException` instead. – asawyer Mar 08 '12 at 13:41
@asawyer `ArgumentNullException` also indicates a programming bug. I’m not sure having both was such a smart decision of the framework. – Konrad Rudolph Mar 08 '12 at 13:42
@Konrad: The docs say 'The exception that is thrown when a null reference (Nothing in Visual Basic) is passed to a method that does not accept it as a valid argument.' for `ArgumentNullException`, but 'The exception that is thrown when there is an attempt to dereference a null object reference.' for `NullReferenceException`, so I think the former is more appropriate. – mdm Mar 08 '12 at 13:44
If a null is passed into a reference type comparison, there _is_ a programming bug - in the calling code. It often (always?) won't make sense to compare a null reference type for magnitude - how else would you respond to that error? – Tom W Mar 08 '12 at 13:45
@mdm The distinction is artificial and arbitrary. Logically, passing a null object to a method which doesn’t expect it amounts to dereferencing a null pointer. The only *meaningful* difference occurs if the method really doesn’t dereference (but I’m hard-pressed to think of such an example except when storing the value). I believe the .NET framework designers shouldn’t have introduced two separate exceptions for that. – Konrad Rudolph Mar 08 '12 at 13:49
@KonradRudolph If you let the framework throw a null reference somewhere in the body of a method, it's not nearly as easy to debug then an upfront "Hey, you called me badly with this parameter" exception. I think Eric Lippert wrote a blog post on this, but I'm not seeing it. – asawyer Mar 08 '12 at 13:55
@KonradRudolph: I respectfully disagree :) If you call a method and it throws an NRE, it might be confusing to debug (e.g. if you are also passing in arguments that can be null) - especially if you are calling that method in an assembly that has been compiled without debugging symbols. I believe there _should_ be a distinction between 'You called me and I'm throwing an NRE, good luck finding out why' and 'You called me but this argument named X shouldn't be null'. The ANE inherits from ArgumentException alongside 8 or 9 other different exceptions for dealing with invalid arguments in this way. – mdm Mar 08 '12 at 13:58
@asawyer You are totally right. But it’s irksome that for this fact alone you need to check all parameters. But even so, this still doesn’t justify the need for two separate exception classes. – Konrad Rudolph Mar 08 '12 at 14:00
@mdm I think you are wrong. In both cases you get the information that an argument was null that shouldn’t be. At that point, hit the documentation to find out which it is. I fail to see what confusion could possibly arise. – Konrad Rudolph Mar 08 '12 at 14:02
@KonradRudolph I normally use two separate constructs when I have two separate semantics. Why should this be different? – asawyer Mar 08 '12 at 14:02
@asawyer I claim that the semantics are *not* different. In both cases you access a `null` object which shouldn’t be. Whether the access happens by calling a method *on* it or passing it *to* a method should make no difference, it’s only syntax. Semantically, the same happens: you want to operate on a non-existing referenced object. – Konrad Rudolph Mar 08 '12 at 14:04
@KonradRudolph No, in one case you deference a null object, and in the other you fail a parameter validation check. This is not the same thing. – asawyer Mar 08 '12 at 14:08
@asawyer A “parameter validation check” is an artificial construct used to express the notion of “I don’t allow `null` here since I want to deference.” It is just a *mechanism*. But in both cases you *logically intend to* dereference the object. – Konrad Rudolph Mar 08 '12 at 14:11
I agree with Konrad on the logical thing. But I still think it makes sense to have both exception types because their semantics are different. One of them refers to an *argument* and actually identifies the specific parameter, which is more helpful for debugging the code than a `NullReferenceException` from in the middle of the code. Remember that if lots of methods are missing the null check, you might end up with a `NullReferenceException` from much deeper in the call tree than where the actual problem is. – Timwi Apr 25 '12 at 23:20

score 1 · Answer 4 · answered Mar 08 '12 at 13:40

1

A custom operator is little more than a static method. Moreover, operators in generals shouldn't normally throw exceptions. Which means you need those null-checks if MyClass is a reference-type.

By the way, it's conventional for nulls to be less than non-nulls, which makes your proposed implementation idiomatic.

answered Mar 08 '12 at 13:40

Ani

111,048
26
262
307

how can the first parameter be null? – vulkanino Mar 08 '12 at 13:41
Or you just let the system throw, no harm in that (except that the exception is slightly less appropriate). – Konrad Rudolph Mar 08 '12 at 13:42
@vulkanino: The first parameter can be null since the operator is implemented as a static method. – Ani Mar 08 '12 at 13:44
@vulkanino MyClass a = null; MyClass b = null; return a < b; – Servy Mar 08 '12 at 13:58

score -1 · Answer 5 · edited Nov 12 '12 at 14:43

-1

It's a bad idea to overload operators on classes. It's ok for structs though.

If you do decide to overload an operator on a class, you will either have to:
a. Include null-check into your logic
b. Throw exceptions when null is passed in
c. Don't null check and allow for NullReferenceExceptions (bad)

Basically, it's a bad idea to overload an operator on a class. I'd either turn your class into a struct, or just implement an interface such as IComparable<T> / IEquatable<T> which has guidelines when null values are used in comparisons.

edited Nov 12 '12 at 14:43

zastrowm

8,017
3
43
63

answered Mar 08 '12 at 13:43

myermian

31,823
24
123
215

I don’t agree with that – it often makes sense to have value-like classes, in particular since the relevant (useful) guideline in .NET says “if in doubt, write a class”. – Konrad Rudolph Mar 08 '12 at 13:50
It's a worse idea to overload operators for a class. A struct is a value, which can be compared to other values. A class is a reference and should not be treated like a struct. I have yet to see any need to overload an operator for a class (as opposed to a struct) ever. – myermian Mar 08 '12 at 14:01
1

standard example: Complex class. Wouldn't you overload the operators? – vulkanino Mar 08 '12 at 14:03
I don´t agree either, see System.Version. It is a class and I think <, >, ... is usefull and makes sense. – Lukas Mar 08 '12 at 14:04
4

@m-y Whether you overload operators has *logical* reasons: you overload them when it makes sense for a business entity. On the other hand, whether to use a class or a struct are *not* business decisions but rather technical decisions based on technical trade-offs. The two decisions are (somewhat) orthogonal. For instance, why wouldn’t you overload comparison operators for a `String` class? – Konrad Rudolph Mar 08 '12 at 14:09

needs overload operator< and null check

5 Answers5

Linked