Correct way to deal with this constructor situations

Question

I have a constructor like so:

public function __construct($data)
{
    $this->_data = new SimpleXMLElement($data);
}

Which is fine. However i would like to give the opportunity to pass the simplexmlelement or the string.

Something like:

public function __construct($data)
{
    if (is_string($data)) {
        $data = new SimpleXMLElement($data);
    }
    $this->_data = $data;
}

Is this bad practice?

My thoughts were that the client code can then decide how much work it wants to do up front. But does it cause any problems?

see [this question](http://stackoverflow.com/questions/1699796/best-way-to-do-multiple-constructors-in-php)'s accepted answer — ajiang, Aug 28 '13 at 19:45
Possibly of interest: one of the PSR coding guidelines discourages underscore prefixes for protected and private variables. This was popular back when all class properties were public, but now we have real protected and private access levels, the underscore is not required. — halfer, Aug 28 '13 at 20:53
Simple question, complex answer. Sorry for that, I'd say this can not be easily answered and it's a bit between programming question and code-review. But it's good you ask. Reminds to explain simple things which most often is most complex, so it's very effective to ask. — hakre, Aug 28 '13 at 21:56

score 6 · Accepted Answer · edited May 23 '17 at 12:15

Well, one way to deal with that is to move the problem out of there, it's actually pretty simple then:

public function __construct(SimpleXMLElement $data)
{
    $this->_data = $data;
}

You then just pass a SimpleXMLElement in there and can decide when you construct the object if it is from a string or another SimpleXMLElement or what not.

As you can see, this makes the code even more flexible. Two simple rules followed:

Prevent to make use of the new keyword inside a constructor.
Reduce the usage of if.

If you now wonder where you can put the code to create the object (e.g. if that is some logic you would use in multiple places and you fear to duplicate it), you can consider to either add some static helper methods:

public static function createFromXmlString($string) {
    return static::createFromSimpleXmlElement(
        simplexml_load_string($string)
    );
}

public static function createFromSimpleXmlElement(SimpleXMLElement $element) {
    return new static($element);
}

public function __construct(SimpleXMLElement $data)
{
    ...

Usage:

$foo = foo::createFromXmlString('<root/>');

Or to create a factory object that does the job (move the object creation code away from the object).

The static global class methods are often easier to introduce, so you normally can write them on the fly, but most often if there are more than two start to think about how to remove those functions again, e.g. moving code into a factory.

Is this [constructor dealing with multiple input types] bad practice?

It sort of is, because you use the new keyword inside the constructor. It destroys testing and the classname you use after new is a hidden dependency (you don't want those; see as well Why not “instantiate a new object inside object constructor”? [actually not such a good Q&A, just look for code-smell discussions about this and also inversion of control / dependency injection -- Update: more the material of this kind is in my mind: Flaw: Constructor does Real Work (ca. Nov 2008; by Miško Hevery) (whatch also this guys video on Youtube, he has a nice accent and good style to explain things)])

The other part of bad practice is that you tend to add more and more cases to deal with in the same constructor. So it changes over time while actually the object did not really change.

I'd say this is a common mistake (and shortcut as well), as there is not so much right and wrong in life, just try to find out for yourself, what is a decision point for you. Already asking here is a good sign I'd say, you start to sharpen your senses.

My thoughts were that the client code can then decide how much work it wants to do up front. But does it cause any problems?

First of all it is a very good understanding to differ between the "client" code and the object code. You clearly show what you think is a benefit.

On the other hand, one object should do one thing at a time and also creating a new object is a critical phase. If you start to put much code and logic into the constructor, things tend to become complicated. It's just a place too important to introduce complicated code to (e.g. anything more than three lines is complicated). The constructor should only initialize the new object and that's it.

Regardless of the suggestions I've been given in the first part of my answer, more OOP style would probably be the following:

abstract class Foo {...}

class FooString extends Foo {
    public function __construct($string) {
        ...
    }
}

class FooSimpleXML extends Foo {
    public function __construct(SimpleXMLElement $element) {
        ...
    }
}

You have one type/object, which is Foo. As your codebase grows, your client code has different requirements to get a Foo from, however it always needs a Foo. By creating subtypes of the Foo basetype, old code must not be changed and new code can be added for the changed requirements.

This is perhaps the most clean approach, however, you need to know that Foo is distinct type and not some kind of helper object here.

so dont offer the choice, and make the client code responsible for creating the object always? — Marty Wallace, Aug 28 '13 at 19:44
...and maybe offer an alternative constructor if really necessary: `Foo::fromString($data)`... — deceze, Aug 28 '13 at 19:45
@deceze: One can tend to do so, if (I can not always suggest that), then I would use `class::createFromType()`, start all those static global class methods with `create` verb. — hakre, Aug 28 '13 at 19:46
@Marty Wallace: I now also added an additional part that discusses a bit more your concrete feedback questions. — hakre, Aug 28 '13 at 20:04
@MartyWallace: Thanks for the accept, but I was not confident with the link to question about complexity and side-effects in the constructor, here is a much better link (also updated the answer): [Flaw: Constructor does Real Work (ca. Nov 2008; by Miško Hevery)](http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/) - This guy is pretty good at explaining stuff, so worth to read even it's long. You take something for developer life. — hakre, Aug 28 '13 at 22:10

Correct way to deal with this constructor situations

1 Answers1