views:

410

answers:

5

So I have a class with a single string parameter in its constructor:

public MyClass(string name)
{
    this.Name = name;
}

but I never want to worry about some one initializing an instance of MyClass with name that is NULL or a zero length string. What is the best was to validate the parameter? With a property or method you can always just do nothing, return null, or not set anything if an invalid argument is provided. But once a constructor is called the instance of the object is already created regardless of whether it was passed valid arguments.

Is throwing an exception as shown below the best way? Or is there some other method that's more preferred?

public MyClass(string name)
{
    if (name == null | name == "") throw new ArgumentException("Name can not be null or blank", "name");

    this.Name = name;
}

Of course I could always set the name to some default like "Unnamed MyClass" if the argument is invalid, but in this case I'd rather just prevent an invalid name then trying to assume some alternate behavior.

+1  A: 

You can either set the name to some default or throw an exception. Anything else is just plain wrong since it would create an object with a non-valid state.

Tal Pressman
+5  A: 
  1. Throw an ArgumentNullException
  2. Document that the ctor throws ArgumentNullException if name is null
  3. If you are using Code Contracts, add a Contract.EndContractBlock() line after your parameter validation.

Edit: Instead of this:

if (name == null || name == "")

Use this:

if (string.IsNullOrEmpty(name))
280Z28
Be very careful when throwing exceptions in constructors. You might end up with leaked resources if you've started grabbing resources/allocating memory etc.
Matt H
Either way, wouldn't it have been || (Logical OR) and not | (Bitwise OR) ? Just wondering.
Jorge Israel Peña
@Blaenk, yes, I used copy and paste. Amusingly my usage was not factually affected by the OP's bug. :)
280Z28
Point 3, if using Code Contracts, would it not be far better to use Contract.Requires(!string.IsNullOrEmpty(name));? ... As I understand it, Constract.EndContractBlock(); is really only meant to be added to legacy code when migrating to Code Contracts (or possibly conditions that cannot be effectively checked with contracts).
jerryjvl
@jerryjvl: If the code is publicly exposed, you should always validate the parameters. `Contract.Requires` has the `ConditionalAttribute` applied. If you are performing closed-world static analysis, then you can use `Contract.Requires` by itself.
280Z28
+1  A: 

If passing an empty or null name represents an error, throwing an exception is the reasonable thing to do. Ignoring the null name and doing some arbitrary recovery is just masking the error and becomes likely to lead to invalid state.

If not passing a name is a valid way to construct the object, offer a parameterless constructor.

kyoryu
+2  A: 

The preferred solution here is to throw an exception. Fail early, fail often. Document what the valid parameters are for your constructor and that it throws an ArgumentNullException or ArgumentOutOfRangeException on invalid parameters.

In my opinion, the salient point is that you don't want to silently absorb failures. Suppose the user typed in a name incorrectly (accidentally left it null, for example). Better to have the operation fail and return notification to the user than to swallow the failure (with, say, a default) and have the user unaware they mistyped their entry.

I asked a similar question awhile back to settle an argument with some colleagues.

"But once a constructor is called the instance of the object is already created regardless of whether it was passed valid arguments."

The object is created (i.e., non-null) only if the constructor returns normally.

Jason
+1  A: 

There is a Guard utility class that you might find useful for validating arguments passed to any method.

The class is avaialable here using System; using System.Globalization;

    namespace Microsoft.Practices.Mobile.CompositeUI.Utility
    {
        /// <summary>
        /// Common guard clauses.
        /// </summary>
        public static class Guard
        {


 /// <summary>
     /// Checks a string argument to ensure it isn't null or empty.
     /// </summary>
     /// <param name="argumentValue">The argument value to check.</param>
     /// <param name="argumentName">The name of the argument.</param>
     public static void ArgumentNotNullOrEmptyString(string argumentValue, string argumentName)
     {
      ArgumentNotNull(argumentValue, argumentName);

      if (argumentValue.Trim().Length == 0)
       throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Properties.Resources.StringCannotBeEmpty, argumentName));
     }

     /// <summary>
     /// Checks an argument to ensure it isn't null.
     /// </summary>
     /// <param name="argumentValue">The argument value to check.</param>
     /// <param name="argumentName">The name of the argument.</param>
     public static void ArgumentNotNull(object argumentValue, string argumentName)
     {
      if (argumentValue == null)
       throw new ArgumentNullException(argumentName);
     }

     /// <summary>
     /// Checks an Enum argument to ensure that its value is defined by the specified Enum type.
     /// </summary>
     /// <param name="enumType">The Enum type the value should correspond to.</param>
     /// <param name="value">The value to check for.</param>
     /// <param name="argumentName">The name of the argument holding the value.</param>
     public static void EnumValueIsDefined(Type enumType, object value, string argumentName)
     {
      if (Enum.IsDefined(enumType, value) == false)
       throw new ArgumentException(String.Format(CultureInfo.CurrentCulture,
        Properties.Resources.InvalidEnumValue,
        argumentName, enumType.ToString()));
     }

     /// <summary>
     /// Verifies that an argument type is assignable from the provided type (meaning
     /// interfaces are implemented, or classes exist in the base class hierarchy).
     /// </summary>
     /// <param name="assignee">The argument type.</param>
     /// <param name="providedType">The type it must be assignable from.</param>
     /// <param name="argumentName">The argument name.</param>
     public static void TypeIsAssignableFromType(Type assignee, Type providedType, string argumentName)
     {
      if (!providedType.IsAssignableFrom(assignee))
       throw new ArgumentException(string.Format(CultureInfo.CurrentCulture,
        Properties.Resources.TypeNotCompatible, assignee, providedType), argumentName);
     }
    }
}
chikak