OOP Design Question - Validating properties

Question

I have the habit of always validating property setters against bad data, even if there's no where in my program that would reasonably input bad data. My QA person doesn't want me throwing exceptions unless I can explain where they would occur. Should I be validating all properties? Is there a standard on this I could point to?

Example:

public void setName(String newName){
   if (newName == null){
      throw new IllegalArgumentException("Name cannot be null");
   }
   name = newName;
}

...

//Only call to setName(String)
t.setName("Jim");

Answer 1

You're enforcing your method's preconditions, which are an important part of its contract. There's nothing wrong with doing that, and it also serves as self-documenting code (if I read your method's code, I immediately see what I shouldn't pass to it), though asserts may be preferable for that.

Answer 2

Personally I prefer using Asserts in these wildly improbable cases just to avoid difficult to read code but to make it clear that assumptions are being made in the function's algorithms.

But, of course, this is very much a judgement call that has to be made on a case-by-case basis. You can see it (and I have seen it) get completely out of hand - to the point where a simple function is turned into a tangle of if statements that pretty much never evaluate to true.

Answer 3

Well, it's been awhile since this question was posted but I'd like to give a different point of view on this topic.

Using the specific example you posted, IMHO you should doing validation, but in a different way.

The key to archieving validation lies in the question itself. Think about it: you're dealing with names, not strings. A string is a name when it's not null. We can also think of additional characteristics that make a string a name: is cannot be empty nor contain spaces.

Suppose you need to add those validation rules: if you stick with your approach you'll end up cluttering your setter as @SingleShot said.

Also, what would you do if more than one domain object has a setName setter? Even if you use helper classes as @dave does, code will still be duplicated: calls to the helper instances.

Now, think for a moment: what if all the arguments you could ever receive in the setName method were valid? No validation would be needed for sure. I might sound too optimistic, but it can be done.

Remember you're dealing with names, so why don't model the concept of a name? Here's a vanilla, dummy implementation to show the idea:

public class Name

  public static Name From(String value) {
    if (string.IsNullOrEmpty(value)) throw new ...
    if (value.contains(' ')) throw new ...  

    return new Name(value);
  }

  private Name(string value) {
    this.value = value;
  }

  // other Name stuff goes here...  
}

Because validation is happening at the moment of creation, you can only get valid Name instances. There's no way to create a Name instance from an "invalid" string. Not only the validation code has been centralized, but also exceptions are thrown in a context that have meaning to them (the creation of a Name instance).

You can read about great design principles in Hernan Wilkinson's "Design Principles Behind Patagonia" (the name example is taken from it). Be sure to check the ESUG 2010 Video and the presentation slides

Finally, I think you might find Jim Shore's "Fail Fast" article interesting.

Answer 4

You are doing ok ! Whether it's a setter or a function - always validate and throw meaningfull exception. you never know when you'll need it, and you will...

Answer 5

In general I don't favor this practice. It's not that performing validation is bad, but rather, on things like simple setters it tends to create more clutter than its worth in protecting from bugs. I prefer using unit tests to insure there are no bugs.

Answer 6

It's a tradeoff. It's more code to write, review and maintain, but you'll probably find problems quicker if somehow a null name gets through.

I lean towards having it because eventually you find you do need it.

I used to have utility classes to keep the code to a minimum. So instead of

if (name == null) { throw new ...

you could have

Util.assertNotNull(name)

Then Java added asserts to the language and you could do it more directly. And turn it off if you wanted.

Answer 7

It's well done in my opinion. For null values throw IllegalArgumentException. For other kind of validations you should consider using a customized exceptions hierarchy related to your domain objects.

Answer 8

I'm not aware of any documented standard that says 'validate all user input' but it's a very good idea. In the current version of the program it may not be possible to access this particular property with invalid data but that's not going to prevent it from happening in the future. All sorts of interesting things happen during maintenance. And hey, you never know when someone will reuse the class in another application that doesn't validate data before passing it in.