Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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);
        }
    }
}
share|improve this question
10  
"acceptable to throw an exception?" What's the alternative? –  S.Lott Feb 23 '11 at 19:16
7  
@S.Lott: Do nothing. Set a default value. Print a mesasge to the console/logfile. Ring a bell. Blink a light. –  FrustratedWithFormsDesigner Feb 23 '11 at 19:27
3  
@FrustratedWithFormsDesigner: "Do nothing?" How will the "text not empty" constraint be satisfied? "Set a default value"? How will the text argument value be used? The other ideas are fine, but those two would lead to an object in an state that violates the constraints on this class. –  S.Lott Feb 23 '11 at 19:28
1  
@MPelletier: I will repeat myself. "open" is not the issue. You've said it twice. I'm sure you're open. What alternative forced you to ask this question? Clearly, someone suggested something to you. What specific suggestion caused you to ask this question? Please explain what code you saw or what conversation you had that lead you to see an alternative to an exception. –  S.Lott Feb 23 '11 at 19:57
2  
@MPelletier, validating the parameters ahead of time will end up violating DRY. (Don't Repeat Yourself) As you'll need to copy that pre-validation to any code that tries to instantiate this class. –  CaffGeek Feb 23 '11 at 20:23

10 Answers 10

up vote 16 down vote accepted

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", "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", "text");
share|improve this answer
    
I'm not sure his "specific case" is specific enough to give such specific advice. We have no idea if it makes sense in this context to throw an exception or simply allow the object to exist anyway. –  FrustratedWithFormsDesigner Feb 23 '11 at 19:15
    
@FrustratedWithFormsDesigner - I assume you down-voted me for this? Of course you are right that I am extrapolating but clearly this theoretical object must be invalid if the input text is empty. Would you really want to have to call Init to receive some sort of error code when an exception will work just as well and force your object to always maintain a valid state? –  ChaosPandion Feb 23 '11 at 19:23
1  
I tend to split that first case out into two separate exceptions: one that tests for nullity and throws an ArgumentNullException and another which tests for empty and throws an ArgumentException. Allows for the caller to be more picky if they want to in its exception handling. –  Jesse C. Slicer Feb 23 '11 at 19:26
1  
@Jesse - I've used that technique but I'm still on the fence about its overall usefulness. –  ChaosPandion Feb 23 '11 at 19:37
2  
+1 for immutable objects! I also use them wherever possible (I personally find almost always). –  user8685 Feb 23 '11 at 20:45

So, what happens when your messagebox tries to show in my web app, on the server with no one around to click OK and resume execution? I know I'd much rather deal with an exception . . .

share|improve this answer
4  
not to mention just the ickyness of coupling gui code with every single class that validates its input –  jk. Feb 25 '11 at 9:36
2  
Obviously, you purchase third party software to automatically click the dialog :) –  Wayne M Aug 12 '11 at 20:20
1  
This is upvoted? Am I the only one that think this is completely irrelevent to the question being asked, and sees "MessageBox.Show" as simply a placeholder to simplify the question being asked? -1 –  bunglestink Aug 13 '11 at 3:46
1  
Why make presumptions? If the author had intended that, he very well could have indicated it. One can only answer the question as presented. –  Wyatt Barnett Aug 13 '11 at 13:54

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.

share|improve this answer
    
You are so gonna ruffle some feathers with you C++ comment. :) –  ChaosPandion Feb 23 '11 at 20:13
3  
Ehh, C++ is a great language, but one of my pet peeves is people who learn one language well and then write like that all the time. C# isn't C++. –  Ant Feb 23 '11 at 20:21
5  
It can still be dangerous to throw exceptions from a ctor in C# because the ctor might have allocated unmanaged resources that will then never be disposed. And the finalizer needs to be written to be defensive against the scenario where an incompletely-constructed object is finalized. But these scenarios are relatively rare. An upcoming article on the C# Dev Center will be covering these scenarios and how to defensively code around them, so watch that space for details. –  Eric Lippert Feb 24 '11 at 17:25
5  
eh? throwing an exception from the ctor is the only thing you can do in c++ if you can't construct the object, and there is no reason this has to cause a memory leak (though obviously its up to ). –  jk. Feb 25 '11 at 9:53
    
@jk: Hmm, you're right! Though it seems an alternative is to flag bad objects as "zombies", which the STL apparently does in lieu of throwing exceptions: yosefk.com/c++fqa/exceptions.html#fqa-17.2 Objective-C's solution is much tidier. –  Ant Feb 25 '11 at 10:34

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.

share|improve this answer
    
+1 for the confession alone :) –  MPelletier Feb 23 '11 at 19:14

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.

share|improve this answer
    
I don't see the advantage. Why is testing the result of a factory for null preferable to having a try-catch somewhere that can catch the exception from the constructor? Your approach seems to be "disregard exceptions, go back to explicitly checking the return value from methods for errors". We long ago accepted that exceptions are generally superior to having to explicitly test every method return-value for errors, so your advice seems suspect. I'd want to see some justification for why you think the general rule does not apply here. –  D.W. Aug 12 '11 at 22:19
    
we never did accept that exceptions were superior to return codes, they can be a humungous performance hit if thrown too often. However, throwing an exception from a constructor will leak memory, even in .NET, if you've allocated something that's unmanaged. You have to do a lot of work in your finaliser to make up for this case. Anyway, the factory is a good idea here, and TBH it should throw an exception instead of returning a null. Assuming you create only few 'bad' classes, then making the factory throw is preferable. –  gbjbaanb Dec 14 '11 at 23:48

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?

share|improve this answer
1  
I think the question implies that the class actually requires the string to be not empty. That is not the case in your StringBuilder example. –  Sergio Acosta Feb 23 '11 at 19:12
    
Then the answer must be yes - it is acceptable to throw a exception. To avoid having to try/catch this error, then you could use a factory pattern to handle the object creation for you, which would include an empty string case. –  WeekendWarrior Feb 23 '11 at 19:19

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.

share|improve this answer

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
    }
}
share|improve this answer
  • 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.
share|improve this answer

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.

share|improve this answer
2  
@Dunk, that's just wrong. –  CaffGeek Feb 23 '11 at 20:27
4  
@Chad, that is your opinion, but my opinion is not wrong. It is just different from yours. I find try-catch code far more difficult to read and I've seen far more errors created when people use try-catch for trivial error handling than more traditional methods. That has been my experience and it isn't wrong. It is what it is. –  Dunk Feb 23 '11 at 20:42
5  
The POINT is that after I call a constructor, if it doesn't create because an input is invalid, that is an EXCEPTION. The app's data is now in an inconsistent/invalid state if I simply create the object and set a flag saying isValid = false. Having to test after a class is created if it's valid is horrible, very error prone design. And, you said, have a constructor, then call initialize... What if I don't call initialize? What then? And what if Initialize gets bad data, can I throw an exception now? Your class shouldn't be difficult to use. –  CaffGeek Feb 23 '11 at 21:01
5  
@Dunk, if you find exceptions hard to use, you're using them wrong. Simply put, bad input was provided to a constructor. That's an exception. The app should not continue to run unless it's fixed. Period. If it does, you end up with a worse problem, bad data. If you don't know how to handle the exception, you don't, you let it bubble up the call stack until it can be handled, even if that means simply logging, and stopping execution. Simply put, you don't need to handle exceptions, or test for them. They simply work. –  CaffGeek Feb 23 '11 at 22:26
3  
Sorry, this is just too much of an anti-pattern to consider. –  MPelletier Feb 25 '11 at 2:31

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.