Separation of concern
You mention "separation of concern" in your question. It's fantastic that you actually know about it, and are trying to adopt good practices whilst learning about OO. However, allow me to be a pedantic tw*t for a second, and stress that SoC is not the same as SRP (Single Responsibility Principle).
It's the SRP that states that a class can have one, and only one reason to change. That's a fancy way of saying that 1 class == 1 job. SoC operates on a higher level. You may, or may not have, stumbled across buzzwords or terms like MVC. SoC is the main reason for architectural patterns like the MVC pattern: Each component (module) can, and often does, consist of a multitude of classes. Each of these classes have a single job that can be grouped under a specific module (ie: the "V" module (view) consists of a renderer class, a class that manages the cache, a class that manages the output buffers etc...).
Just a bit of a pedantic lecture, as an aside :-P
At first glance:
Well, the first thing that strikes me as, erm..., off is your failing to specify the visibility of your class/interface members. While it's true that PHP will default to public access unless you specify otherwise, and that interface members have to be public by definition, it's best to be specific, therefore change your methods from:
function init();//in interface
To:
public function init();
Another niggle I have is with your coding style. It might seem irrelevant at first, or a matter of personal preference (which it is, to a certain degree), but there are such things as coding standards. They exist for good reason, and should really be adhered to as much as possible. In this case, this means that:
function get_is_author_referrer();
//should become
public function getIsAuthorReferrer();
//or perhaps better still:
public function isAuthorReferrer();
In general, methods that start with get
are getter methods (methods that will return the value of a member/property). set
methods are setters to set that member. If a method should return a boolean, it's quite common to see methods with an is
prefix. Just like it's fairly common to see classes that implement a has
method, to check the value of a given property, take this basic example:
class Foo
{
/**
* @var array|null
*/
protected $someArray = null;
/**
* @param array|null $vals = null
*/
public function __construct(array $vals = null)
{
$this->someArray = $vals;
}
/**
* @return array|null
*/
public function getVals()
{
return $this->someArray;
}
/**
* @param array $vals
* @return $this
*/
public function setVals(array $vals)
{
$this->someArray = $vals;
return $this;
}
/**
* @return bool
*/
public function isArray()
{
return is_array($this->someArray);
}
/**
* @return bool
*/
public function hasVals()
{
return !empty($this->someArray);
}
}
I can check to see if the $someArray
property of a given instance of Foo
is null, or an array (using isArray
). I can also check to see if that property isn't empty. I can get the value of the property, or set it using the getter and setter methods just fine. There's no real point in using additional properties for this, so I'd suggest you do something along the lines of:
protected $authorReferrer = null;
//in the init method:
$this->authorReferrer = isset( $_GET['aq'] ) ? $_GET['aq'] : null;
//add these methods:
public function isAuthorReferrer()
{
return $this->authorReferrer !== null;//if not null
}
public function getAuthorReferrer()
{
return $this->authorReferrer;
}
The upshot is that your class can now be used both to check if certain params were set in the $_GET
superglobal (more on your use of this later), The instance can also be used to retrieve the values, and possibly validate or normalize them.
Try not to support people on EOL versions
Your class, and interface seem to be following the PSR-0 autoloading standard (to some degree). This standard states that the _
(underscores) in names like Single_Post_Referrer_Class
will be replaced with directory separators. It was commonplace to use this in PHP versions prior to PHP 5.3 (which introduced namespace support). However, PSR-0 is now deprecated in favour of the PSR-4 standard, simply because there is no supported PHP version left that does not support namespaces. Heck, even 5.3 has been EOL'd about half a year ago.
What does this mean for you? Well, put simply: Shorter class names:
class Single_Post_Referrer_Class {}
//now becomes:
<?php
namespace MainNS\Single\Post;
class Referrer {}
Where MainNS
is the root namespace for your project. I ditched the Class
because that's a reserved keyword, and a rather dodgy class name anyway.
TL;TR: Embrace namespaces
Specific critiques
Now for some more specific critiques/issues I have with your code:
A constructor mustn't be abused
You're certainly not the only person to abuse a constructor. And to be honest, I've seen a lot worse that your code (honestly, it's not half bad), but let's be honest:
public function __construct()
{//standards: this { goes on the next line
$this->init();
}
Your constructor calls a second method, and yes, all the method seemingly does is to initialize the instance, but look at your class from a user standpoint (the person using your code):
$x = new Referrer();
Does his code accurately reflect what is going on? Of course not! Can you force the user to only create instances of your class when there is a $_GET
variable available? No, you can't. Someone might, for example, choose to write a CLI script using your codebase and (owing to name conflicts or for whatever reason) create an instance of your class, which relies entirely on their being a $_GET
variable available.
I know there are many, many more complete definitions of a class out there, but here's what I think a class essentially is/should be:
"A class is a self-contained unit of code that couples data and functionality together in order to perform a single, task, regardless of the context in which task is to be performed"
What does that mean? Well, take the simplest of classes for example: a data model:
class Person
{
/**
* @var string
*/
protected $name = null;
/**
* @var string
*/
protected $email = null;
/**
* @var int
*/
protected $age = null;
/**
* @param string $name
* @param string $email
* @param int $age
*/
public function __construct($name, $email, $age)
{
$this->setName($name)
->setEmail($email)
->setAge($age);
}
/**
* Basic, but somewhat unsafe setter (Relies on user to pass expected value)
* @param string $name
* @return $this
*/
public function setName($name)
{
$this->name = $name;
return $this;
}
/**
* @return string
*/
public function getName()
{
return $this->name;
}
/**
* Example of validating setter: throws exceptions if value isn't valid
* @param string $name
* @return $this
* @throws \InvalidArgumentException if email is not a valid address
*/
public function setEmail($email)
{
if (!filter_var($email, \FILTER_VALIDATE_EMAIL))
{//<-- allman brackets, not standard, I know... :-P
throw new \InvalidArgumentException(
sprintf(
'%s expects $email to be valid email address, %s is not',
__METHOD__,
$email
)
);
}
$this->email = $email;
return $this;
}
/**
* @return string
*/
public function getEmail()
{
return $this->email;
}
/**
* Example of setter that attempts some basic normalization
* @param int $age
* @return $this
*/
public function setAge($age)
{
$this->age = abs((int) $age);//use abs in case of negative vals, cast to int
return $this;
}
/**
* @return int
*/
public function getAge()
{
return $this->age;
}
}
This class does what it says on the tin: It represents a person, and all of its associated data (a name, an age, and an email address). The setters perform some normalization or validation, and regardless of where this class is used, or where the data comes from: this class will behave perfectly predictable:
$person = new Person('Foo Bar', '[email protected]', 33);
echo $person->getName(), ' is ', $person->getAge(), ' years of age', PHP_EOL,
' and can be reached via: ', $person->getEmail();
try {
$person->setEmail($person->getAge());//try 33 as email address...
} catch (\InvalidArgumentException $e) {
//Person::setEmail expects $email to be valid email address 33 is not
echo 'Rather predictable: ', $e->getMessage();
}
I could (and probably would) change this class a tad, making the constructor optional, or allow the user of the class to set the properties through an associative array, or an object, by simply adding this method:
/**
* @param array|\stdClass|\Traversable $mixed
* @return $this
* @throws \InvalidArgumentException
public function setBuld($mixed)
{
if (!is_array($mixed) && !$mixed instanceof \stdClass && !$mixed instanceof \Traversable)
{
throw new \InvalidArgumentException(
sprintf(
'%s needs either an array, or an instance of \\stdClass or \\Traversable to be passed, instead saw %s',
__METHOD__,
is_object($mixed) ? get_class($mixed) : gettype($mixed)
);
);
}
foreach ($mixed as $name => $value)
{//create setter from $name
$setter = 'set' . ucfirst(strtolower($name));
if ($setter !== 'setBulk' && method_exists($this, $setter))
{//ensure no recursive calls here, and make sure the setter exists:
$this->{$setter}($value);//set value
}
}
return $this;
}
Using this approach, you change your class and interface to be a bit more forgiving, and (more importantly) make it so that your class no longer relies on super globals like $_GET
:
namespace MainNS\Single\Post;
interface ReferrerInterface
{
/**
* @param array $values
* @return $this
*/
public function init(array $values);
/**
* @return bool
*/
public function isAuthorReferrer();
/**
* @return bool
*/
public function isDateReferrer();
/**
* @param mixed $authorReferrer
* @return $this
*/
public function setAuthorReferrer( $authorReferrer );
/**
* @param string|\DateTime $authorReferrer
* @return $this
*/
public function setDateReferrer( $dateReferrer );
}
Here, I simply reworked your interface a bit, to ensure the public function init
allows the user to pass an array of values to the instance upon which the method is called. So in your class, I'd rewrite a thing or two, too:
namespace MainNS\Single\Post;
class Referrer implements ReferrerInterface
{
/**
* @var mixed (string?)
*/
protected $authorReferrer = null;
/**
* @var \DateTime
*/
protected $dateReferrer = null;
/**
* @param array $values = null;
*/
public function __construct(array $values = null)
{
if ($values)
$this->init($values);
}
/**
* {@inheritdoc}
*/
public function init(array $values)
{//or implement the setBulk I've shown earlier
if (isset($values['dateReferrer']))
$this->setDateReferrer($values['dateReferrer']);
if (isset($values['authorReferrer']))
$this->setAuthorReferrer($values['authorReferrer']);
return $this;
}
/**
* @param mixed $authorDate
* @return $this
*/
public function setDateReferrer($dr)
{
if (!$dr instanceof \DateTime)
{
$dr = new \DateTime($dr);
}
$this->dateReferrer = $dr;
return $this;
}
//implement rest of the interface here...
}
The way you implement the interface is, of course, down to you entirely. However, keep in mind that PHP is quite forgiving in terms of breaches of contract (more so than other languages). That doesn't mean that breaking the inherited or implemented contracts is anything else than wrong (I'd be happy to go into this in a bit more detail if you want me to, just let me know in a comment).
It's therefore important to understand, and religiously follow the liskov principles at all times.
Anyway, I've been going on for some time now, and I guess I'll leave it at this for now. Hope this helps you on your way
function get_is_author_referrer() { return isset($_GET['aq']); }
. Moreover you don't need to insert in your interface one method that you call only inside the class itself. – chumkiu Feb 3 '15 at 19:41