Passing an object instead of parameters

Question

Please see the code below:

public class Person
{
 public DateTime DateOfBirth;
 public List<Offer> Offers = new List<Offer>();
 public Person(DateTime DateOfBirth)
 {
 int age = DateTime.Now.Year-candidate.Year;
 if (DateTime.Now < candidate.AddYears(age)) age--;
 if (age < 18 || age > 99)
 throw new ArgumentException("Age must be greater than 18.", "DateOfBirth");
 _dateOfBirth=dateOfBirth;
 }
 public void GetOffers(IOfferCalculator offerCalculator)
 {
 return offerCalculator.GetOffers(this);
 }
 public void AssignOffers(List<Offer> offers)
 {
 //Assign offers to Person.Offers here.
 }
}

Notice that I pass the entire Person object to Person.GetOffers(). The reason I do this is because the person has to be over 21 for all of the offers and this is checked (validated) in the Person constructor. Is this a good reason to pass the Enquiry as an object to: offerCalculator.GetOffers? The reason I ask is because t3chb0t described this as a code smell in my question here: https://codereview.stackexchange.com/questions/186124/testing-the-process-of-assigning-offers-to-a-customer

Alternatively I could just pass the DateOfBirth like this: return offerCalculator.GetOffers(DateOfBirth). However, this would mean that I would have to validate the DateOfBirth in every offer.

I am trying to follow the principle of least astonishment these days and find myself overthinking a lot. My two questions are:

1) Is it a good appraoch to pass the Person object to offerCalculator.GetOffers as Person already validated the date of birth?

2) Should I be checking the date of birth against every Offer, even though every Offer requires the person to be over 21? I don't think this will evert change i.e. the person will always have to be over 21.

• Laiv: The AssignOffers is confusing me. Well, the whole class is confusing me. what's is supposed to do GetOffersand AddOffers if offers is public?

Answer 1

Like anything this ultimately comes down to the use case and team's coding standards. However in general I would support passing in objects instead of basic data types.

You've already given one reason, to allow you to validate in one place rather than multiple but let me add another few reasons.

If you're only passing in a date, it's possible that someone could pass in an incorrect date - for example the date the person was created in the system. This would not be caught as a compilation error, however if they accidentally passed in a User instead of a Person then the compilation would fail and the bug would be prevented.

Secondly, if the requirements ever changed so GetOffers( ) required an additional property (let's say it was based on their location as well as their DOB) this could easily be refactored to use other properties on the Person object instead of needing to change signature.

However, I'm not a great fan of your code layout. I suspect one of the reasons this is getting a little knotted is that you've not got a clearly defined business logic layer. Wouldn't it make more sense to have your UI (or whatever calling code you're using) call an IOfferCalculator which takes a person? That in my mind makes far more sense than having a UI create a person and pass in an IOfferCalculator which then in turn takes the person itself?

My suggestion would be:

public void MyUIMethod(int personId)
{
 // _personDataAccess and _offerCalculator will have been passed in via DI
 var person = _personDataAccess.GetPerson(personId);
 var offers = _offerCalculator.GetOffers(person);
 // do something with the offers
}

TLDR Personally I like passing in object however ultimately it comes down to design and the teams' coding standards. However, if I was you I'd take a look at your BLL design - that, I suspect is a cause of some of your headaches.

• Pieter B: How do you think about passing the object but through an interface declaration?
• Liath: @PieterB do you mean where IPerson is an ICanHaveOffers? Great approach (as long as it's not engineered for the problem at hand).
• w0051977: @Laith, I don't think it is advisable to have Domain Objects that implement interfaces. +1 for the code in the question though i.e. bypassing Enquiry for getting offers.
• Liath: @DavidArno you're right of course - they should be resolved when the class is created and passed in via DI not resolved as required. I don't think it impacts the answer but I'll clean it up in case anyone out there is C&Ping!
• w0051977: @Laith, I cannot see a comment from David Arno. What did he say?