OO style parameters vs type parameters

Question

Say you have these two methods:

Number 1:

void AddPerson(Person person)
{
  // Validate person
  if(person.Name != null && IsValidDate(person.BirthDate)
    DB.AddPersonToDatabase(person);
}

Number 2:

void AddPerson(string name, DateTime birthDate)
{
  Person p = new Person(name, birthDate);
  DB.AddPersonToDatabase(person);
}

Which of the two methods is the best one? I know the first one is more correct OO-wise, but I feel the second is more readable, and you don't have to make sure the object is valid as the parameters make sure of this. I just don't like to having to validate the objects anywhere I pass them as parameters. Are there other approaches?

EDIT: Thx for all the answers. To clarify, validating in the constructor and a IsValid method is of course a good approach, but in my code the valid state of the person is often dependant of the context and could vary from method to method. This could of course be a sign of bad design.

The code is just a example to describe the problem.

Answer 1

The first one has the advantage of allowing you to change the Person definition without breaking existing code, only recompilation is needed. You may think the second one is more readable, but the first one is more maintainable, your choice.

Answer 2

The first shouldn't have to validate person.Name and person.BirthDate - they should be validated automatically by the Person constructor. In other words, if you're passed a Person, you should know that it's valid.

On the other hand, you'd have to check that person isn't a null reference.

It's sometimes worth having convenience methods like the second version to avoid having to explicitly call the constructor quite as often though. It should usually just call the Person constructor and then delegate the work to the first form.

Answer 3

I prefer the former (passing an object) because it reduces coupling of the API to the object. If you change the Person object, e.g. add a new property such as Nickname which wasn't previously needed, then in the first case you don't need to change the public API, whereas in the second one you need to either change the method or add a new overload.

Answer 4

I would most of the time go for the first one.

The second one would break the signature for every change in Person

Answer 5

It depends on the context.

If all of your calling methods deal with Person objects, then the first is the correct solution.

However, if some of your calling methods deal with name and birthdates, and some deal with Person objects, then the second example is the correct solution.

Answer 6

I would just create overloads for both. Especially considering that they can be created automatically.

Answer 7

Another option would be:

void AddPerson(Person person)
{  // Validate person  
   if(person.IsValid)
   {
     DB.AddPersonToDatabase(person);
   }
}

Assuming that Person doesn't validate itself when it is constructed. Which in some cases is viable if the object can be an invalid state while it is transient.

Answer 8

I agree that it depends entirely on the context, there are no absolute rules for this. In my opinion, it would be nonsense to have methods like these:

person.SetBirthDate(Person person)
person.ResetPassword(Person person)

But In this case I do prefer the former, because, as Greg Beech said, the method doesn't (have to) know anything about the domain object.

By the way, consider overloading (DRY - Don't Repeat Yourself):

void AddPerson(Person person)
{
  if(person.Name != null && IsValidDate(person.BirthDate)
    DB.AddPersonToDatabase(person);
}

void AddPerson(string name, DateTime birthDate)
{
  Person p = new Person(name, birthDate);
  this.AddPerson(p);
}

Answer 9

If you want to overload and offer both, I would have the second call the first:

void AddPerson(string name, DateTime birthDate)
{
    AddPerson(new Person(name, birthDate);
}

I would also use Josh's idea of adding an IsValid() function, or Jon Skeet's idea of validating in the constructor.

Answer 10

I'm assuming that the methods are not part of the Person type. With that in mind I feel that both of them have a little too much knowledge about another type (Person) imo. The first version shouldn't have to validate the creation of a valid person instance. If this is the responsibility of the caller, then every caller must do this. That's brittle.

The second version has a strong dependency to another Type due to the fact, that it creates an instance of this type.

I would definitely prefer the first version, but I would move the validation part from that piece of the code.

Answer 11

I think Jon pretty much nailed it. Person should be responsibility to ensure a valid Person is created it its constructor.

About the resonsibility who creates or not creates the Person object (whether the AddPerson method or its caller), read

http://en.wikipedia.org/wiki/GRASP_%28Object_Oriented_Design%29#Creator

It's about questions of responsibility in OOP. In your specific case if the AddPerson wrapping the call to a DB interface, I'm not quite sure about it. It depends what that Person object is used for outside that context. If it's solely for the purpose of containing the data to be added to the Database, creating it in the AddPerson method possibly is a good idea, because it decouples the user of your class from having to know about the Person class. .

Answer 12

Better encapsulation if you use objects; that's what they're for. I think people get into trouble when they use primitives too much.

It should not be possible to create an invalid Person. The constructor should check for valid parameters and throw IllegalArgumentException or fail an assert if they're invalid. This is what Programming By Contract is all about.