Should I use setters with null or no setter when null value?

Question

I would like to know what are the drawbacks of setting values this way:

class PersonFactory
{
    public function createPerson($firstName = null, $lastName = null, $separator = null)
    {
        $person = new Person();
        $person
            ->setFirstName($firstName)
            ->setLastName($lastName)
            ->setSeparator($separator);

        return $person;
    }
}

And if this way would be more clear, sustainable or efficient

class PersonFactory
{
    public function createPerson($firstName = null, $lastName = null, $separator = null)
    {
        $person = new Person();
        if (!null === $firstName) {
            $person->setFirstName($firstName);
        }

        if (!null === $lastName) {
            $person->setLastName($lastName);
        }

        if (!null === $separator) {
            $person->setSeparator($separator);
        }

        return $person;
    }
}

Example call:

class PersonController extends Controller
{
    public function indexAction()
    {
        // get the PersonFactory instance
        $person = $this->get('person.factory')
            ->createPerson('Bob', 'Whatever', '-');

        return $this->render('some/file.html.twig', [
            'person' => $person
        ]);
    }
}

It feels like they are the same as null is already the value of an undefined variable, at least in PHP (looking at you Javascript).

So, would the only difference would be to call the setters even if it's not useful ?

Answer 1

Your factory class has little value here if your underlying Person class is fully mutable. That is, since your person class provides setters, there is not reason you couldn't just do this:

$person = new Person();
$person->setFirstName(...);
$person->setLastName(...);
// etc.

Or, if you wanted to pass the values to constructor:

$person = new Person($firstName, $lastName, ...);

All your "factory" is doing is acting like a constructor for Person.

Factories can be very useful for helping you to collect all dependencies needed for the class(es) it will instantiate in order to ensure the returned object is set up in a proper state. Here you are not doing that at all, in that you are allowing pretty much any value (or no value at all) to be passed to the factory and doing no validation whatsoever against the passed data.

What if this factory gets passed an array, object, numeric primitive, empty string, etc. for one of your parameters? Do you truly want to allow the Person object to be instantiated into this state?

If you are truly OK with the values on person being set to arbitrary values (i.e. this is simple app and you are not worried about class re-use), and you are OK with the use case where not all parameters are set, then I would say just go for your first form, as there should be no problem in setting a null value to an already null property. I think the second form might imply some level of validation or state control that simply doesn't exist.

If this is your preferred approach, I would just ditch the factory class and make the Person constructor behave like this factory method is doing now.

Answer 2

First off i'm a little confused by what you are trying to do, and why you are doing this that way. I don't really grasp the full context of what and why you are doing it that way.

1. IMO since you are returning the same type of person, you don't need a person factory (update). You would need a person factory class if you were returning different types of People. Have a look at the method of polymorphism employed here: https://gist.github.com/BKSpurgeon/db4f8062569181d871f1565c89103e7e notice how we have different types of cities: LondonCity, SanFranCity - they all supply slightly different behaviour, but they are all cities.
2. setLastName not instance methods within the person class? Please have a look at the code below for what i mean. You probably should make these method private if you don't intend to call them.
3. "that's a no-no for OOP diehards"

this is what i mean.

# as OOP programmers we don't want to be doing things like this:

if x == 2 THEN do this
If x == 3 THEN do this
if x  == 4 THEN do this.
# As OOP programmers this is all we want to do:
#
cww = ClassWhichDoesWork.new(x)
cww.DoWork()
#
# and then we use a Factory method and polymorphism to supply the correct behaviour.
# we have no conditionals which say if x == 2 THEN do this. all of that is taken care of by polymorphism.
# if you do it that way you code will be very easy to maintain and to make changes, and to test as well.

4. Here is a gist which is probably how I would structure the above code given my very limited understanding of what you are trying to do