3
\$\begingroup\$

I have a couple of functions in WordPress that I'm rewriting to OOP. All these functions are interlinked in some way and also reuse code from other functions.

This is an example of the first class I'm trying. I'm very new to OOP, but I have learned about separation of concern, so basically each concern get its own class.

interface Single_Post_Referrer_Interface {

    function init();

    function get_is_author_referrer();

    function get_is_date_referrer();

    function set_is_author_referrer( $is_author_referrer );

    function set_is_date_referrer( $is_date_referrer );

}

class Single_Post_Referrer_Class implements Single_Post_Referrer_Interface {

    private $is_author_referrer;
    private $is_date_referrer;

    function __construct() {

        $this->init();

    }

    function init() {

        $is_author_referrer = isset( $_GET['aq'] );
        $is_date_referrer   = isset( $_GET['dq'] );

        $this->set_is_author_referrer( $is_author_referrer );
        $this->set_is_date_referrer( $is_date_referrer );

    }

    function get_is_author_referrer() {

        return $this->is_author_referrer;

    }

    function get_is_date_referrer() {

        return $this->is_date_referrer;

    }


    function set_is_author_referrer( $is_author_referrer ) {

        $this->is_author_referrer = $is_author_referrer ;

        return $this;

    }

    function set_is_date_referrer( $is_date_referrer ) {

        $this->is_date_referrer = $is_date_referrer ;

        return $this;

    }


}

$a = new Single_Post_Referrer_Class();

This class basically is a lot of conditional checks (checks if a certain query variable is in the URL and sets the appropriate condition) and return the conditional checks with a boolean value which will be used by other classes.

As this is my first attempt, I would like to know if this is correctly done and if not, where I can improve. Just a note: I correctly get the correct values if I do a var_dump( $a );.

\$\endgroup\$
  • \$\begingroup\$ This is an example or it's a real code? Why you do this instead of simple 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. \$\endgroup\$ – Luca Rainone Feb 3 '15 at 19:41
  • \$\begingroup\$ This is a sample, I'need to add some more methods and properties. Just wanted a general idea if I'm on the right track :-) \$\endgroup\$ – Pieter Goosen Feb 4 '15 at 4:28
6
\$\begingroup\$

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', 'foo@bar.com', 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

\$\endgroup\$
  • \$\begingroup\$ While I do use the CamelCase naming convention myself, it is not stated any that Snake_case is wrong. It is simply a matter of taste. For some snake case is easier to read as spaces are clearly visible and it doesn't require as much effort to read. \$\endgroup\$ – AnotherGuy Feb 4 '15 at 15:31
  • \$\begingroup\$ I greatly appreciate your detailed answer and many thanks for the critique. \$\endgroup\$ – Pieter Goosen Feb 4 '15 at 15:33
  • \$\begingroup\$ @AnotherGuy: PSR-I: "Method names MUST be declared in camelCase" It's an unequivocal MUST, I'm afraid... \$\endgroup\$ – Elias Van Ootegem Feb 4 '15 at 15:34
  • \$\begingroup\$ Just so you know. There are other coding standard out there. In fact PSR is now PSR-FIG where FIG stands for Framework Interop Group. This isn't a framework :D \$\endgroup\$ – AnotherGuy Feb 4 '15 at 15:39
  • \$\begingroup\$ @AnotherGuy: I know, there's the age-old PEAR standards, and then each framework has some specific standards, too. However, given that now PEAR and most major players adopted the PHP-FIG standards. FIG has been part of the name for at least 3 years now, but that doesn't change the fact that it's the most commonly adopted, and the most active standard out there. The more people adopt a single standard, the quicker we can abandon smaller standards, and all agree on one std, that we all can help shape \$\endgroup\$ – Elias Van Ootegem Feb 4 '15 at 15:53
4
\$\begingroup\$

Welcome to the world of OOP. I think its awesome you are trying something new and pushing yourself, keep going! Before you read on keep in mind this may seem harsh. I have no intention of hurting you. In fact the opposite. By mentioning everything I see as an issue I want to help you. So without further ado, lets get to it then!

The main stuff

There are somethings which caught my eye. First of is the constructor. It is only dispatching the logic to the init() method. This seems wrong to me. You may have heard that doing work in the constructor is bad practice, but there is some interpretation along with it. The constructor should initialize properties and make very simple logic which the class relies on. The $is_author_referrer and $is_date_referrer properties should be set in the constructor. The isset check shouldn't mean much in the constructor as you are setting the return value and working with it.

This leads me to the set_is_* methods. As the class name suggests the class is associated with only one post. I my world it means the data shouldn't suddenly change. In my opinion I would remove them entirely. Their purpose in the init() method is only a wrapper around something as simple as setting a value. This will in turn also make your init() method irrelevant so that can be removed as well.

Now you are left with a small/clear class signature. Only methods which should be called can be called.

Now to some other things. You should make it a habit to declare visibility of your methods as well as your properties. When only writing function ...() it is by default declared public. It is good practice to declare all internally used methods private. Another thing is returning values from methods. When you use a setter method such as the set_is_author_referrer() method, you should either return nothing or a boolean value indicating if the value was set successfully. This will help you use a generic way to check for errors. Better even would be to throw an exception if an error occurred and return null (nothing, is returned by default) on success. When returning $this you should document it heavily. Returning $this can be done in getter methods as well. But the return value of getters has a tendency to be used as arguments on other methods and if you return a class instance, trouble may come your way :D

The minor stuff

Now to some minor change that I would make. As said earlier some say it is bad practice to do work in the constructor. Even though I redeemed the isset call harmless you could improve a little more. To make the class more usable you could pass the value of $_GET['aq'] and $_GET['dq'] as arguments in the constructor.

public function __construct($aq, $dq) { ...

Now consider a scenario where the data doesn't come from the HTTP query string, but from some other source. Before you would have a problem as the class could only work with the query. But now your class in unaware of where the data comes from, in fact, it doesn't even care.

But hey! You lost something nice now. You cannot just instantiate the class and work with it anymore. But luckily there are help here. You can use a factory class. Think of a factory class as a class that knows how your classes should be instantiated (very factory like isn't it?). Consider the example below. (I have used your naming convention).

class Query_Referrer_Factory {

    public static function create() {

        $aq = isset($_GET['aq']);
        $dq = isset($_GET['dq']);

        return new Single_Post_Referrer_Class($aq, $dq);

    }

}

Now to get an instance you would do the following:

$referrer = Query_Referrer_Factory::create();

$referrer->get_is_author_referrer();

Because the method create in the factory is static you wont have to instantiate the factory. Now lets say the data comes from a JSON source. Then you would create a factory called Json_Referrer_Factory and build your class. These factories could even implement their own interface to make sure they can be change without any errors.

interface Post_Referrer_Factory_Interface {

    public static function create();

}

Also try to keep the word class out of class names. As functions/methods usually start with lowercase letters and classes, abstract classes, interfaces and traits start with uppercase letters, this should be self evident. The purists might even say that abstract classes, interfaces and traits should have their type appended, because they otherwise would look like a class. An example AwesomeInterface, NiceTrait or CoolAbstract.

The advanced stuff

In the interface you have declared, you also declare the init() method. I do not assume this should have been called multiple times, so why expose it in an interface. A good rule about interfaces is to keep them short. This means keep the method list short. When keeping interfaces short you will find naming them easier as well, a nice side effect. A rule is that interfaces should only expose what can/should be used by other code. If I would be really harsh I would split into two. A readable and writable interface.

interface Single_Post_Referrer_Readable_Interface {

    function get_is_author_referrer();
    function get_is_date_referrer();

}

interface Single_Post_Referrer_Writable_Interface {

    function set_is_author_referrer($author);
    function set_is_date_referrer($date);

}

Now as your class (in my opinion) shouldn't be able to change state (change properties etc.). The writable interface is irrelevant and the class should only implement the readable variant.

Before I continue with some resources you can use when learning, I would like the write a sum up of everything I have mentioned. The class below relies on the factory strategy I described above.

class Single_Post_Referrer implements Single_Post_Referrer_Readable_Interface {

    private $author;
    private $date;

    public function __construct($aq, $dq) {
        $this->author = $aq;
        $this->date   = $dq;
    }

    function get_is_author_referrer() {
        return $this->author;
    }

    function get_is_date_referrer() {
        return $this->date;
    }

}

Remember this class has no error handling or type checking of incoming arguments. I assume the author should be a string and the date should be an integer or a string.

The good to learn stuff

The one thing that first caught my eye was lack of documentation. When you are writing code help other developers and yourself in six months by writing good documentation. Take a look a the PHP Documentor. Im sure there are plenty of tutorials around the web that can explain the tool for you. This also has a nice side effect. If you are using an IDE (NetBeans, PHPStorm), they will read the documentation and use it through auto-completion.

Another very good thing to look at is the SOLID principle. Here is a tutorial specific for PHP from TutsPlus. It is hard to use all principles at the same time and it is VERY rare to see it in reality. But keeping the principles in mind while coding can help you A LOT (it has for me), even though to bend the rules a little to make it fit.

Hope this can guide you, happy coding!

\$\endgroup\$
  • \$\begingroup\$ Many thanks for your constructive tap on the fingers :-) I really do appreciate your feedback. Will work through your answer. Most things makes sense at this stage. :-) \$\endgroup\$ – Pieter Goosen Feb 4 '15 at 13:12
  • \$\begingroup\$ You are welcome. Feel free to ask me if there is anything that doesn't quite make sense. \$\endgroup\$ – AnotherGuy Feb 4 '15 at 13:13
  • \$\begingroup\$ "the class name suggests the class is associated with only one post" <== PHP is, in essence, stateless, so regardless of names or whatever, any instance of any class will always be "associated" with a single request, POST or otherwise \$\endgroup\$ – Elias Van Ootegem Feb 4 '15 at 15:17
  • \$\begingroup\$ @EliasVanOotegem - I agree with you, but as he described he as new to OOP I wouldn't go there. Besides the way I though about the class was that it represented a single post entry like answer on this site. There can be multiple answers, but the data associated with each of them shouldn't change. \$\endgroup\$ – AnotherGuy Feb 4 '15 at 15:27
  • \$\begingroup\$ +1 For removing init. Usually init is used as a Factory which is another responsibility completely. Look up the factory pattern to find out more! \$\endgroup\$ – Tek Feb 4 '15 at 17:35

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.