CODEWITHSUNDEEP
c#oopmethods
terjetyl
terjetyl

Reputation: 9565

OO style parameters vs type parameters

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.

Upvotes: 3

Views: 774

Answers (12)

Juliet
Juliet

Reputation: 81516

Definitely better to pass a Person object around, rather than a bunch of primitive types as parameters. Compare the following two methods


public static void Withdrawal(Account account, decimal amount)
{
    DB.UpdateBalance(account.AccountNumber, amount);
}

public static void Withdraw(int accountNumber, decimal amount)
{
    DB.UpdateBalance(accountNumber, amount);
}

The two methods look nearly identical, but the second one is unsafe. An int can come from anywhere, so you're screwed if you write this:

private void CloseTransaction(Transaction tran)
{
    BankAccounts.Withdrawal(tran.Account.RoutingNumber, tran.Amount);
        // logic error: meant to pass Account.AccountNumber instead of Account.RoutingNumber
}

This is the worst kind of error because it won't throw a compilation error or a runtime exception. You might catch this error in your automated tests if you write them well enough, but this bug is easy to miss and might be able to hide for months without being discovered.

I worked a company which wrote bank software, and we really did run across a bug of this type in production. It only occurred during a specific kind of vault transfer, and it was only discovered when one of our banks noticed their GL balances off by a few $100 each time they ran their end-of-month process. The bank suspected employee theft for months, but it was only through careful code review that someone traced the problem to a bug in our software.

Upvotes: 1

duffymo
duffymo

Reputation: 308743

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.

Upvotes: 0

Johannes Schaub - litb
Johannes Schaub - litb

Reputation: 506897

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. .

Upvotes: 0

Brian Rasmussen
Brian Rasmussen

Reputation: 116401

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.

Upvotes: 0

user39603
user39603

Reputation: 2235

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);
}

Upvotes: 1

JoshBerke
JoshBerke

Reputation: 67068

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.

Upvotes: 2

Dmitri Nesteruk
Dmitri Nesteruk

Reputation: 23789

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

Upvotes: 0

Robert Venables
Robert Venables

Reputation: 5971

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.

Upvotes: 0

Kb.
Kb.

Reputation: 7400

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

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

Upvotes: 0

Greg Beech
Greg Beech

Reputation: 136587

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.

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1500055

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.

Upvotes: 10

Otávio Décio
Otávio Décio

Reputation: 74250

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.

Upvotes: 2

Related Questions