4
\$\begingroup\$

Instead of using multiple setter functions within a class like

public function setAction()
public function setMethod()

etc

I've been using a method from a base class that all other classes have access to.

public function set_properties($properties) {

    foreach ($properties as $key => $value) {
    $this->$key = $value;
    }
}

So I can just use it like so

$Class->set_properties(array('action' => 'page.php', 'method' => 'get'));

Is this a bad practice that I'm getting into or is this an okay way of setting properties?

\$\endgroup\$
  • 2
    \$\begingroup\$ You should still keep the possibility that a property will need a special setter (that does more than just setting the property). In your loop, you should check whether $this has a method called 'set' . ucfirst($key) (so setAction if the key is action), you can use method_exists() for this, and if it does have it, invoke that instead of setting the property manually. \$\endgroup\$ – bažmegakapa Aug 11 '13 at 17:07
  • \$\begingroup\$ You could also generate the setters name and call it passing the property. Then you have the best of both worlds. \$\endgroup\$ – Bart Aug 11 '13 at 18:49
  • \$\begingroup\$ I would prefer having multiple setters to make things simple and clear. \$\endgroup\$ – Kinjal Sep 27 '13 at 12:21
2
\$\begingroup\$

Having individual setters has a couple of advantages:

A dedicated setter allows you to put dedicated validations for these properties. You could obviously put them in the bulk setter as well, but that will make the bulk setter large and unwieldy very quickly. You want methods to do one dedicated thing instead. That keeps them maintainable.

Dedicated setters clearly communicate which properties can be set. With a bulk setter, it needs documentation or a look at the source code to learn what I can legally set. I usually don't want to look at your class code. I just want to look at the public API and understand how I can use that class.

That's not to say bulk setters are bad per se. If you know folks are likely to set two or more values after another because they naturally go together, you could allow for a setFooAndBar($foo, $bar) method. But as you can see, that's still much more explicit than a generic set(array $data). Also one could argue that if you need to set arguments that naturally go together, you rather want them to be in an object of their own and pass that object then.

On a side note: Your bulk setter will create properties on the fly for those keys that are not properties in the class. While you could consider that a feature, it makes debugging harder because a developer has to know the object's current state at runtime to know what properties it has.

On another side note: a base class is usually a code smell. It tends to amass unrelated functionality and turn in a God object quickly. It also hampers reuse because all your classes will depend on it then.

\$\endgroup\$
1
\$\begingroup\$

implementing a setProperties method is fine, because that's probably what you're doing in the __construct method anyway:

class Foo
{
    private $bar = null;
    private $baz = null;
    public function __construct(array $properties)
    {
        foreach($properties as $k => $value)
        {
            $this->{$k} = $value;
        }
    }
}

Which is, of course, absolutely fine. Personally, though, I'd still implement a setter/getter method for each individual property, though, for several reasons:

  • More readable code. If someone is to use your class, and wants to set a distinct property to a given value, there's no need for him to write setProperties(array('bar' => 'new value'));. He can simply write $instance->setBar('new value'); which is easier to understand, and less error prone if you ask me. (upon reviewing my answer, I actually noted I made a typo, and wrote setProperties(array(); by mistake, missing the second closing )... case in point :-P)
  • A setter means that property was predefined. Read my answer to this SO question to get a better understanding as to why this is important. Bottom line: it's faster to get that property and set it, because the lookup is -in theory- an O(1) operation.
  • Accidental typo's make for O(n) lookups, so they're almost always going to be slower. A typo like setProperties(array('bat' => 'new value')); won't throw up errors, but when, later on, you attemt a getProperty('bar'); call, and it returns null you may find debugging is a bit of a nightmare, the more complex your code becomes.
    Compare that to setBat('new Value');, which will throw an error (method undefined)... you will, I think, agree that the latter scenario is a tad easier to debug.
  • Types. I know PHP isn't all too strict with its types. A string can be converted to an int at runtime, should the need arrive. Though it can sometimes come in handy if you know what type a property, or var holds. Again, take data-models. If a model, representing a database record has a property id, that id is likely to be an integer. The setter, then, could contain a cast ($this->id = (int) $idParameter;). The example class below illustrates some other tricks you might consider using, like passing the $_POST['password'], which will be converted to a hash on the fly.
  • Programming by interface. A number of classes might share one or two methods/properties. It could well be that this is the only bit of data a given method requires to work. Rather than leaving out the type-hints, and chekcing with method_exists() or property_exists(), or even worse: @$instance->property, you could implement an interface with getters and setters for those properties, and use a type-hint for that interface
  • Access modifiers per property: This is might seem too obvious, but is important to keep in mind. When objects inherit from one another, they gain access to their parents/eachother's __get and __set methods. By using individual properties, you can use the access modifiers to hide certain properties, even from any child classes that might extend from your current classes, and with it any other classes that follow suit.

On a personal note:
sometimes, it's inevitable to use PHP's definition of object overloading (dynamic properties), but on the whole, I try to avoid it as much as possible. Not only because it's slower, but also because often, in case of Data models, the properties are mapped to a DB table. If you introduce a typo there, the DB field names don't match, and you might end up inserting incomplete data, or some other class might churn out faulty queries.
That's why I tend to write my data-models like so:

class User
{
    const SALT = 'S0m3_5@lt_6035-@-L0n9=vv@y';
    private $id = null;
    private $name = null;
    private $hash = null;
    private $active = false;
    public function __construct(array $properties)
    {
        $this->setProperties($properties);
    }
    /**
     * Bulk insert method, uses setter
     **/
    public function setProperties(array $properties)
    {
        foreach($properties as $name => $value)
        {
            $this->__set($name, $value);
        }
        return $this;
    }
    /**
     * Magic setter, to allow `$instance->bar = 'foo';`
     * To behave like $instance->setBar('foo');
     **/
    public function __set($name, $value)
    {
        $name = 'set'.ucfirst(strtolower($name));
        if (!method_exists($this, $name))
        {//don't overload
            return null;//or throw error
        }
        return $this->{$name}($value);
    }
    /**
     * Magic getter, to allow $instance->bar;
     * To behave like $instance->getBar();
     **/
    public function __get($name)
    {
        $name = 'get'.ucfirst(strtolower($name));
        if (!method_exists($this, $name))
        {
            return null;//or throw error
        }
        return $this->{$name}();
    }
    /**
     * Property-specific setter
     **/
    public function setId($int)
    {
        $this->id = (int) $int;
        return $this;
    }
    /**
     * Property-specific getter
     **/
    public function getId()
    {
        return $this->id;
    }
    public function setHash($pwd, $isHash = false)
    {
        $isHash = !!$isHash;//make sure this is a bool
        if (!$isHash)
        {
            $pwd = sha1(self::SALT.$pwd);
        }
        $this->hash = $pwd;
        return $this;
    }
    public function setPassword($pwd)
    {
         return $this->setHash($pwd);
    }
    public function getHash()
    {
        return $this->hash;
    }
}

As you can see, I've also implemented a setPassword method, even though these isn't a password method. This method serves as an alias for setHash, in case you pass the contents of the entire $_POST array to the constructor (when processing a register or login form, for example). This'll cause the setter to do its job, while at the same time, not creating any issues with overloading the class properties, and it doesn't require you to call setHash($_POST['password']); separatly.

Like I said before: programming by interface:
Methods like these aliases are also useful when you're implementing your own interfaces. For example: clients can have an email property, whereas co-workers might have an internal office-link address or something. Create a Contactable interface, and implement a getEmail and setEmail method in the Coworker class, that servers as an alias of the getOfficeLink and setOfficeLink methods respectively.

interface Contactable
{
    public function getEmail();
    public function setEmail($email);
}
//Client class:
class Client implements Contactable
{
    private $email = null;
    public function getEmail()
    {
        return $this->email;
    }
    public function setEmail($email)
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL))
        {//sanitize email!
            $email = null;
        }
        $this->email = $email;
    }
}
//Coworker class:
class Client implements Contactable
{
    private $officeLink = 1;
    public function setOfficeLink($link)
    {
        $link = (int) $link;
        if ($link > 1000 || $link < 1)
        {
            $link = 1;
        }
        $this->officeLink = 1;
        return $this;
    }
    public function getOfficeLink()
    {
        return $this->link;
    }
    public function getEmail()
    {
        return $this->getOfficeLink();
    }
    public function setEmail($email)
    {
        $this->setOfficeLink((int) $email);
    }
}

As you see here, it would be impossible to move the email getters and setters to an abstract class, because the validation for the values differs too much (clients should provide a valid email, whereas coworkers require a number ranging from 1 to 999). An interface is ideal for the job here, because it ensures the methods exist, but the implementation depends almost entirely on the class which implements that interface. Anyway, you can now use type-hints by interface:

public function notifyPerson(Contactable $entity)
{
    $daemon = $this->getMailer();
    $deamon->setMessage('Your request has been processed')
           ->from('noreply@eager-helpers.org')
           ->to($entity->getEmail())
           ->send();
}

This, of course, assumes that the deamon has been programmed to recognize both emails and officeLink numbers. In any case, it's clean code, easy to read and doesn't require you to create a new instance of Client, and assign it a officeLink in email drag, just for one method call, which would make for messy code.

\$\endgroup\$
  • \$\begingroup\$ I found this inspiring for my answer programmers.stackexchange.com/questions/209962/…. I would appreciate some explanation why return $this->{$name}($value) is used instead of $this->$name = $value for the setter and $this->{$name}() instead of $this->$name for the getter. \$\endgroup\$ – Dmitri Zaitsev Sep 15 '13 at 22:11
  • \$\begingroup\$ @DmitriZaitsev: The reason why I'm using the methods, and not accessing the properties directly is simply because the setters and getters may contain additional logic, validating the value (setEmail, for example calls filter_var($email FILTER_VALIDATE_EMAIL)). The getters are there to make sure I'm not accessing properties that don't have a public getter, so I can make sure I'm not exposing properties that I don't want to expose. It also allows me to use interfaces (an interface that defines the getId and setId methods for type-hinting can be useful)... \$\endgroup\$ – Elias Van Ootegem Sep 16 '13 at 6:59
  • \$\begingroup\$ That makes sense, appreciate it! The only thing makes me worry is returning null when you aim for chaining but you comment about throwing error, which is probably the right thing here. \$\endgroup\$ – Dmitri Zaitsev Sep 16 '13 at 16:04
  • \$\begingroup\$ @DmitriZaitsev: Only getters will return null if the value doesn't exist, like $unsetVar evaluates to null. Chainable getters would be illogical. Throwing an exception in setters is, IMHO, the right thing to do. If some code passes a wrong argument to the setter it should throw, because an invalid call to a setter is a bug, not the getter returning null, that's just the consequence of the invalid setter call. Ah well, bottom line: most of my objects implement some form of bulk/array-setter , but that just calls the setters. PS: would you mind awfully accepting one of the answers? \$\endgroup\$ – Elias Van Ootegem Sep 17 '13 at 6:23
  • \$\begingroup\$ Thanks, that confirms my thoughts. I was confused by your function __set($name, $value) returning null - a misprint? PS. I wouldn't mind at all to accept your answer, if only I was the question asker, all I could do was to upvote :) \$\endgroup\$ – Dmitri Zaitsev Sep 17 '13 at 12:31

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.