tags:

views:

507

answers:

6

Hi,

I working on a method that does something given a string parameter. A valid value for the string parameter is anything other than null or string.Empty. So my code looks like this.

private void SomeMethod(string someArgument)
{
    if(string.IsNullOrEmpty(someArgument))
        throw new ArgumentNullException("someArgument");

    // do some work
}

Nothing too exciting there. My question is, is it okay to throw an ArgumentNullException even if the string is equal to string.Empty? Because technically it isn't null. If you believe it should not throw ArgumentNullException what exception should be thrown?

Thanks, Keith.

+2  A: 

InvalidArgumentException for the String.Empty case. This would indicate an issue other than it being null. Ideally, if possible, trim your user input and the String.IsNullOrEmpty will usually suffice, otherwise you get into the situation where you can't trim it since it may potentially be null.

private void SomeMethod(string someArgument)
{
    if(string.IsNullOrEmpty(someArgument))
        throw new ArgumentNullException("someArgument");

    if (someArgument.Trim() == String.Empty)
        throw new ArgumentException("Input cannot be empty", "someArgument");

    // do some work
}

EDIT: changed ArgumentException usage for clarity based on Joe's comment.

Ahmad Mageed
I'm using C# 2.0, the only reference to InvalidArgumentException is in the Microsoft.SqlServer.Management.Common namespace, is what you recommend or are you suggesting that I create my own InvalidArgumentException class?
Keith Moore
@Keith: you're correct, my mistake. You could write your own if you're so inclined or use the ArgumentException provided by the framework. I'll edit to reflect the proper name.
Ahmad Mageed
The ArgumentException constructor doesn't take a "paramName" parameter like ArgumentNullException. So 'throw new ArgumentException("paramName")' is potentially confusing, as it doesn't give any indication what's wrong with the argument. You should be providing a "message" argument something like ("someArgument may not be an empty string"). And in an international app this message would need to be localized. Hence I would only go to all this trouble if there is really a need to distinguish the null case from the empty string case.
Joe
@Joe: in this case perhaps the constructor that takes 2 string parameters would be more appropriate, the 1st being the error message and the 2nd being the parameter that caused the exception (http://msdn.microsoft.com/en-us/library/sxykka64.aspx). This would result in: throw new ArgumentException("Input cannot be empty", "someArgument");
Ahmad Mageed
" ... perhaps the constructor that takes 2 string parameters ..." - yes absolutely, either constructor that takes a message parameter would do, but if you're using it in more than one place the constructor with a paramName parameter is better.
Joe
Joes point is valid for any exception. As a user one of the most annoying things is an exception message that doesn't [really] give you any information. _All_ exceptions should have a message with as much information as is pertinent, preferably the message should offer possible solutions. My pet hate is the "File not found" message, what file isn't found?
Keith Moore
You could also try: if ( String.IsNullOrEmpty( someArgument ?? string.Empty ) ) {} but that would get quite cluttered.But I would agree that you should write one for each case.
Dominic Zukiewicz
+3  A: 

You should throw an ArgumentException if an empty string is not an accepted input for your method. It may be very confusing to clients if you throw an ArgumentNullException while they didn't provide a null argument.

It is simply another use case. You may also have methods that do not accept null input values but that do accept empty strings. It's important to be consistent across your entire application.

Ronald Wildenberg
I was thinking along the lines of ArgumentOutOfRangeException but maybe that is used for array index range exceptions.
Keith Moore
For array index exceptions, IndexOutOfRangeException is used. You should use ArgumentOutOfRangeException only if your method is documented to accept only a specific collection of strings (e.g.: "abc", "def", "ghi" are the only accepted inputs).
Ronald Wildenberg
Thanks for clarifying.
Keith Moore
+1  A: 

ArgumentNullException is sometimes used in the .NET Framework for the String.IsNullOrEmpty case - an example is System.Windows.Forms.Clipboard.SetText.

So I think it's reasonable to do the same in your code, unless there is some real value in distinguishing the two cases.

Note that this and other exceptions derived from ArgumentException generally indicate a programming error, and therefore need to provide information needed to help a developer diagnose the problem. Personally I think it's unlikely that a developer would find it confusing if you use ArgumentNullException for an empty string argument, especially if you document this behavior as in the example below.

/// <summary>
/// ... description of method ...
/// </summary>
/// <param name="someArgument">... description ...</param>
/// <exception cref="ArgumentNullException">someArgument is a null reference or Empty.</exception>
public void SomeMethod(string someArgument)
{
   ...
}
Joe
+2  A: 

Taking all the things into account that have been said (Joe / Ahmad Mageed), I would create an exception for that case then.

class ArgumentNullOrEmptyException : ArgumentNullException
Marcel J.
A: 

This depends on circumstance really.

The question comes down to, is it really an error? By that I mean do you always expect a value? If you do, then probably your best bet here is creating your own Exception, perhaps like so:

class StringEmptyOrNullException : Exception
{
}

Where you can also add your own constructors and added information etc.

If it however is not an "exceptional" happening in your program, if would probably be a better idea to return null from the method and handle it from there. Just remember, Exception's are for exceptional conditions.

Hope this helps,

Kyle

Kyle Rozendo
If you *do* create your own exception, it should derive from ArgumentException rather than Exception.
Joe
Depending on context really, in this context yes. If you want to use it in other places, you make it more generic. For instance if you have a method that generates a string that shouldn't be empty, ArgumentException won't be the best choice. So, depending on the amount of use you have for it, and the context in which it is used, you will then decide on what to inherit from.
Kyle Rozendo
A: 

Why don't use this code?

private void SomeMethod(string someArgument)
{
//chek only NULL
if(ReferenceEquals(someArgument,null))
    throw new ArgumentNullException("someArgument");

// and after trim and check
if (someArgument.Trim() == String.Empty)
    throw new ArgumentException("Input cannot be empty", "someArgument");

// do some work
}
1111