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?
2 Answers 2
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.
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 wrotesetProperties(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 agetProperty('bar');
call, and it returnsnull
you may find debugging is a bit of a nightmare, the more complex your code becomes.
Compare that tosetBat('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()
orproperty_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('[email protected]')
->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.
-
\$\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– Dmitri Zaitsev2013年09月15日 22:11:33 +00:00Commented Sep 15, 2013 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 callsfilter_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 thegetId
andsetId
methods for type-hinting can be useful)... \$\endgroup\$Elias Van Ootegem– Elias Van Ootegem2013年09月16日 06:59:57 +00:00Commented Sep 16, 2013 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– Dmitri Zaitsev2013年09月16日 16:04:39 +00:00Commented Sep 16, 2013 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– Elias Van Ootegem2013年09月17日 06:23:38 +00:00Commented Sep 17, 2013 at 6:23 -
\$\begingroup\$ Thanks, that confirms my thoughts. I was confused by your function
__set($name, $value)
returningnull
- 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– Dmitri Zaitsev2013年09月17日 12:31:17 +00:00Commented Sep 17, 2013 at 12:31
$this
has a method called'set' . ucfirst($key)
(sosetAction
if the key isaction
), you can usemethod_exists()
for this, and if it does have it, invoke that instead of setting the property manually. \$\endgroup\$