Constructor parameter validation in C# - Best practices

Question

What is the best practice for constructor parameter validation?

Suppose a simple bit of C#:

public class MyClass
{
    public MyClass(string text)
    {
        if (String.IsNullOrEmpty(text))
            throw new ArgumentException("Text cannot be empty");

        // continue with normal construction
    }
}

Would it be acceptable to throw an exception?

The alternative I encountered was pre-validation, before instantiating:

public class CallingClass
{
    public MyClass MakeMyClass(string text)
    {
        if (String.IsNullOrEmpty(text))
        {
            MessageBox.Show("Text cannot be empty");
            return null;
        }
        else
        {
            return new MyClass(text);
        }
    }
}

Answer 1

I tend to perform all of my validation in the constructor. This is a must because I almost always create immutable objects. For your specific case I think this is acceptable.

if (string.IsNullOrEmpty(text))
    throw new ArgumentException("message", nameof(text));

If you are using .NET 4 you can do this. Of course this depends on whether you consider a string that contains only white space to be invalid.

if (string.IsNullOrWhiteSpace(text))
    throw new ArgumentException("message", nameof(text));

Answer 2

A lot of people state that constructors shouldn't throw exceptions. KyleG on this page, for example, does just that. Honestly, I can't think of a reason why not.

In C++, throwing an exception from a constructor is a bad idea, because it leaves you with allocated memory containing an uninitialised object that you have no reference to (ie. it's a classic memory leak). That's probably where the stigma comes from - a bunch of old-school C++ developers half-arsed their C# learning and just applied what they knew from C++ to it. In contrast, in Objective-C Apple separated the allocation step from the initialisation step, so constructors in that language can throw exceptions.

C# can't leak memory from an unsuccessful call to a constructor. Even some classes in the .NET framework will throw exceptions in their constructors.

Answer 3

Throw an exception IFF the class cannot be put into a consistent state with regard to its semantic use. Otherwise do not. NEVER allow an object to exist in an inconsistent state. This includes not providing complete constructors (like having an empty constructor + initialize() before your object is actually completely built)...JUST SAY NO!

In a pinch, everyone does it. I did it the other day for a very narrowly used object within a tight scope. Some day down the road, I or someone else will probably pay the price for that slip in practice.

I should note that by "constructor" I mean the thing the client calls to build the object. That could just as easily be something other than an actual construct that goes by the name "Constructor". For example, something like this in C++ wouldn't violate the principle IMNSHO:

struct funky_object
{
  ...
private:
  funky_object();
  bool initialize(std::string);

  friend boost::optional<funky_object> build_funky(std::string);
};
boost::optional<funky_object> build_funky(std::string str)
{
  funky_object fo;
  if (fo.initialize(str)) return fo;
  return boost::optional<funky_object>();
}

Since the only way to create a funky_object is by calling build_funky the principle of never allowing an invalid object to exist remains intact even though the actual "Constructor" doesn't finish the job.

That's a lot of extra work though for questionable gain (maybe even loss). I'd still prefer the exception route.

Answer 4

In this case, I would use the factory method. Basically, set your class have only private constructors and have a factory method which returns an instance of your object. If the initial parameters are invalid, just return null and have the calling code decide what to do.

public class MyClass
{
    private MyClass(string text)
    {
        //normal construction
    }

    public static MyClass MakeMyClass(string text)
    {
        if (String.IsNullOrEmpty(text))
            return null;
        else
            return new MyClass(text);
    }
}
public class CallingClass
{
    public MyClass MakeMyClass(string text)
    {
        var cls = MyClass.MakeMyClass(text);
        if(cls == null)
             //show messagebox or throw exception
        return cls;
    }
}

Never throw exceptions unless the conditions are exceptional. I'm thinking that in this case, an empty value can be passed easily. If that is the case, using this pattern would avoid exceptions and the performance penalties they incur while keeping the MyClass state valid.

Answer 5

• A constructor shouldn't have any side-effects.
  • Anything more than private field initialization should be viewed as a side-effect.
  • A constructor with side-effects breaks the Single-Responsibilty-Principle (SRP) and runs contrary to the spirit of object-oriented-programming (OOP).
• A constructor should be light and should never fail.
  • For instance, I always shudder when I see a try-catch block inside a constructor. A constructor should not throw exceptions or log errors.

One could reasonably question these guidelines and say, "But I don't follow these rules, and my code works fine!" To that, I would reply, "Well that may be true, until it isn't."

• Exceptions and errors inside of a constructor are very unexpected. Unless they are told to do so, future programmers will not be inclined to surround these constructor calls with defensive code.
• If anything fails in production, the generated stack trace may be difficult to parse. The top of the stack trace may point to the constructor call, but many things happen in the constructor, and it may not point to the actual LOC that failed.
  • I've parsed many .NET stack traces where this was the case.

Answer 6

It depends what MyClass is doing. If MyClass was actually a data repository class, and parameter text was a connection string, then best practice would be to throw an ArgumentException. However, if MyClass is the StringBuilder class (for instance), then you could just leave it blank.

So it depends on how essential parameter text is to the method - does the object make sense with a null or blank value?

Answer 7

My preference is to set a default, but I know Java has the "Apache Commons" library that does something like this, and I think that's a fairly good idea as well. I don't see an issue with throwing an exception if the invalid value would put the object in an unusable state; a string isn't a good example but what if it was for poor man's DI? You couldn't operate if a value of null was passed in in place of, let's say, a ICustomerRepository interface. In a situation like this, throwing an exception is the correct way of handling things, I would say.

Answer 8

When I used to work in c++, I didn't use to throw exception in constructor because of memory leak problem it can lead to, I had learned it the hard way. Anyone working with c++ knows how difficult and problematic memory leak can be.

But if you are in c#/Java, then you don't have this problem, because garbage collector will collect the memory. If you are using C#, I think it is preferable to use property for nice and consistent way to make sure that data constraints are guaranteed.

public class MyClass
{
    private string _text;
    public string Text 
    {
        get
        {
            return _text;
        } 
        set
        {
            if (string.IsNullOrWhiteSpace(value))
                throw new ArgumentException("Text cannot be empty");
            _text = value;
        } 
    }

    public MyClass(string text)
    {
        Text = text;
        // continue with normal construction
    }
}

Answer 9

Throwing an exception will be a real pain for the users of your class. If validation is an issue then I generally make creating the object a 2 step process.

1 - Create the instance

2 - Call an Initialize method with a return result.

OR

Call a method/function that creates the class for you that can do the validation.

OR

Have an isValid flag, which I don't particularly like but some people do.