HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Correctly coding an OOP object

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
objectcorrectlycodingoop

Problem

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 );.

Solution

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 m

Code Snippets

function init();//in interface
public function init();
function get_is_author_referrer();
//should become
public function getIsAuthorReferrer();
//or perhaps better still:
public function isAuthorReferrer();
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);
    }
}
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;
}

Context

StackExchange Code Review Q#79476, answer score: 6

Revisions (0)

No revisions yet.