views:

97

answers:

9

In the following example we have two different exceptions we want to communicate.

//constructor
public Main(string arg){
   if(arg==null)
      throw new ArgumentNullException("arg");

  Thing foo=GetFoo(arg);

  if(foo==null)
     throw new NullReferenceException("foo is null");    
}

Is this the proper approach for both exception types?

+1  A: 

The ArgumentNullException is obviously correct and for the second one it depends on your business context.

Petar Minchev
How can it be correct? There isn't a null argument.
Jon Skeet
@Jon - arg is surely an argument and it is checked for being null:)
Petar Minchev
@Petar: Ah... misread it. Sorry, I thought you were suggesting ArgumentNullException for the *second* one. Doh.
Jon Skeet
@Jon, No problem:)
Petar Minchev
+1  A: 

ArgumentNullException is an obvious choice for the first check.

Since it appears that a Thing is derived from an input parameter I would throw an ArgumentException to indicate that a Thing cannot be constructed from the specified input. Afterall, it is (presumably anyway) a problem with the input and not the algorithm used to construct the Thing.

Brian Gideon
Since `Thing` is not an argument, `ArgumentException` should not be used.
John Saunders
The assumption in my answer was that a Thing can be parsed from the input parameter. If the input is invalid you would almost certainly throw an ArgumentException would you not? Of course, if that assumption is not correct then my whole answer is wrong.
Brian Gideon
+1  A: 

ArgumentNullException:

The exception that is thrown when a null reference [...] is passed to a method that does not accept it as a valid argument.

NullReferenceException:

The exception that is thrown when there is an attempt to dereference a null object reference.

"foo is null" is a poor error message, since foo is a local variable. A better error message would be "GetFoo returned null for the input " + arg. Also, if an Exception will be thrown whenever GetFoo returns null, make it throw the appropriate exception.

Kobi
Yep, and the `NullReferenceException` will be thrown automatically by the CLI. Throwing it explicitly has the advantage that one can provide a custom `Exception.Message`.
stakx
@stakx: Wrong. `ArgumentNullException` can also have a custom `Message`. (Just pass two parameters) You should never explicitly throw a `NullReferenceException`.
SLaks
@SLaks: That's not what I meant. What I meant was that you have no influence over the `Message` property if *the CLI* throws the `NullReferenceException`, which is why you might want to throw it *yourself* instead.
stakx
@stakx: You should _never_ throw it yourself. http://msdn.microsoft.com/en-us/library/ms182338.aspx
SLaks
@SLaks: I wouldn't, myself. IMHO `NullReferenceException` is one that you should never actually have to catch, because *usually* it indicates a logical error in the source code that needs to be fixed at compile time, not while the program is executing. This is why I mildly disagree with the MSDN article you linked to: The fact that you shouldn't have to catch this exception doesn't mean that you should never throw it. (See my comment above for why you might want to do so, e.g. during debugging.) I'll agree however that throwing `NullReferenceException` definitely isn't good error handling...
stakx
A: 

I think that these are both correct. For me, choosing an exception type is really just about whether or not it will clearly portray the error that has occured. I always think about it from the perspective of another dev that hasn't seen my class before. Will this other dev be provided with enough information to quickly and accurately locate the problem?

Robin
+3  A: 

Never throw NullReferenceException. That doesn't mean that a null was passed. It means that there was an attempt to use the null.

John Saunders
+5  A: 

You should never explicitly throw a NullReferenceException.

If null was passed as a parameter, you should throw an ArgumentNullException with the name of the parameter.
If some other thing is null, you should probably throw an InvalidOperationException with a descriptive message.

SLaks
+6  A: 

The first exception is definitely correct. It's the second one which is tricky.

There are two possibilities here:

  • GetFoo() isn't meant to return null, ever. In that case we've basically proven a bug in GetFoo(). I'm not sure of the best exception here, leaving ContractException (from Code Contracts) aside. Basically you want something like ContractException - an exception which means "The world has gone crazy: this isn't just an externally unexpected result, there's a bug here."
  • GetFoo() can legitimately return null, due to arg's value. In this case I would suggest that ArgumentException (but not ArgumentNullException) may be appropriate. On the other hand, it's odd to throw ArgumentException after using the argument.

InvalidOperationException isn't quite appropriate here, but I might be tempted to use it as the closest thing to a contract failure...

EDIT: You should also consider creating your own exception, as per Aaronaught's answer.

Jon Skeet
I have to disagree with you for once; `ArgumentException` really does not impart enough information to clearly understand what went wrong, and also makes the exception difficult to catch. `ArgumentException` generally implies a bug, an incorrect usage of the method, whereas it's not clear here that the problem was really with the *argument*. Maybe the `GetThing` method failed for an unexpected reason and papered over that failure with a null return, or maybe there was no real bug at all but some critical file or database record was deleted. `ArgumentException` actually obfuscates the issue.
Aaronaught
+1  A: 

For the first case, I'll pile on and say that ArgumentNullException is correct.

For the second case, I'm really very surprised that nobody else has said this: You should be making your own Exception class for that. None of the built-in system exceptions are really appropriate:

  • ArgumentException implies that the argument itself was invalid in some way; that's not really the case here. The argument was fine, it's just that something unexpected happened later.

  • InvalidOperationException is almost correct, but that exception is generally interpreted to mean that an operation was invoked at the wrong time, such as trying to execute a command on a connection that hasn't been opened yet. In other words, it indicates a mismatch between the current state of the object and the specific operation you tried to perform; that's really not applicable here either.

  • NullReferenceException is right out. That's a reserved exception that means something completely different (that the program actually tried to deference the null reference).

None of these are right. What you really need to be doing is communicated specifically what went wrong, and in order to do that, you should create a MissingThingException. That exception can include the ID of the thing (presumably arg) in its message/detail. This is the best for callers, because it allows them to catch the specific exception if they know how to handle it, and also the best for end users, because it allows you to leave a meaningful error message.

Summary: Create a custom exception class for this.

Aaronaught
IMO you *shouldn't* be catching exceptions like this except in *very* rare situations where there's a known bug you can't fix, and you're simply working around it. To be honest, I don't think we have enough information about what GetThing is meant to be doing to make a particularly good judgement.
Jon Skeet
@Jon: Yes, perhaps they shouldn't be caught, that's up to the caller. The more important aspect of this is giving exceptions a semantic meaning. A `MissingThingException` tells you exactly which assumption was incorrect. On the other hand, if the null return value is *never* expected, then there shouldn't be any `throw` statement at all, just an assertion or code contract. Let the `NullReferenceException` bubble through if you really have no clue what to do about it; otherwise, tell the caller what actually happened.
Aaronaught
@Aaronaught: Letting the NullReferenceException bubble up *at some point* means we don't get to see it as soon as we realise something is wrong. I agree that a code contract would be good if it indicates a definite bug. I'm still unsure about the rest though - as I say, a lot of it is contextual, and we just don't have that context. I think in some situations an ArgumentException would be appropriate; in others a MissingThingException would be better, as you suggest.
Jon Skeet
@Jon: That's why I mentioned an assert; if you did get a `NullReferenceException`, you'd be able to see exactly where the *real* problem was in debug mode. But, you're right of course, missing context and all that.
Aaronaught
getFoo returns a linq to sql entity from the database. If foo is not found for the arg a business rule is violated. I agree if we were using .net 4.0 that a contractexception would work, and also an Assert would work better.
Kenoyer130
@user200295: If it indicates a business rule violation, then it would make sense to have an exception to represent that general concept. You could then use that exception in other, similar places.
Jon Skeet
+1  A: 

Per the Code Analysis blog, Exception usage should be as follows:

  • ArgumentOutOfRangeException if the input value you are testing is not within an expected set of values. (e.g. if the parameter represents a command name, and the provided command name is invalid)

  • ArgumentNullException if the input value cannot be null in order to execute the function.

  • ArgumentException if the input value is invalid (e.g. if you pass in an empty string, but empty strings are not allowed)

Warren