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?

Answer 1

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.