1
\$\begingroup\$

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 ?

\$\endgroup\$
  • \$\begingroup\$ can you please provide more information about how the Person is created? Who is calling: new Person()? it looks like the API could have been better structured: any time you have conditionals which also set behaviour: that's a no-no for OOP die-hards. Secondly why are setFirstName/setLastName not instance methods within the person class? \$\endgroup\$ – BKSpurgeon Apr 13 '17 at 9:30
  • \$\begingroup\$ This code belongs to a PersonFactory which creates Persons depending on the inputs of the method createPerson. I don't understand the expression: "that's a no-no for OOP die-hards" sorry. I don't understand either what you mean by "why are setFirstName/setLastName not instance methods within the person class" . Do you mean private/protected methods ? Otherwise, as these methods appears in the Person class and are called in this createPerson method, I don't understand why you say they aren't instance methods. \$\endgroup\$ – Leogout Apr 13 '17 at 9:48
  • \$\begingroup\$ please post the PersonFactory and where the factory is being called from. it's very hard to comment without seeing overall where it's going. \$\endgroup\$ – BKSpurgeon Apr 13 '17 at 9:52
  • \$\begingroup\$ I added some context but I don't think it will give you any clues. There is still a createPerson() method which instanciates a person based on nullable parameters... Could you explain the terms I didn't understood in your first comment please ? \$\endgroup\$ – Leogout Apr 13 '17 at 10:03
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Yes you are right, this factory isn't very useful. The only use case i can find is if i need to override it to provide extended Persons, but it's a very rare occasion. \$\endgroup\$ – Leogout Apr 14 '17 at 14:12
0
\$\begingroup\$

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.
  1. 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
\$\endgroup\$
  • \$\begingroup\$ Thanks for your time, that's crazy how much effort you put in this answer! When i have enough reputation i'll give you a huge +1! The Person example was just to make it more readable. The question wasn't about the Factory design pattern neither about best OOP practices but really about which solution would be more clear, sustainable or efficient. That's why i thought context wasn't useful here. The truth is this Factory actually returns different instances of different classes based on given parameters, but when I had to make these parameters nullable I wondered which solution would be better. \$\endgroup\$ – Leogout Apr 13 '17 at 13:01
  • \$\begingroup\$ as a general rule you don't want to return different instances of different classes - unless they are subclasses of the same type. \$\endgroup\$ – BKSpurgeon Apr 13 '17 at 13:13
  • \$\begingroup\$ btw updated the above answer because i made a very bad typo. perhaps review it in a couple of weeks \$\endgroup\$ – BKSpurgeon Apr 13 '17 at 13:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.