4
\$\begingroup\$

I have implemented the following PHP magic method behavior to save my time of creating accessor and mutators for properties of any class that extends this base class.

I am looking for suggesiongs on improving it for my application that assumes the following:

Properties of the form:

  • $_first_name
  • $_user_id

Methods of the form:

  • getUserId()
  • setFirstName("Mohammad")

I have some points that I think need more research:

  • Is it a secure way? What are the insecurities here?

  • Is my way of handling errors by using throw new Exception('my message'); a good practice? What are the alternatives?

  • Is there a best practice or a design pattern that addresses such a solution for reusability?

  • Am I exaggerating the steps? Are there any steps can be combine into one step with the same results?

The __call function of my Base class:

class Base
{
    private $_first_name; // just an example

    public function __call($name, $args)
    {
        // The begining of a method must be in all lower case characters. 
        // E.g. set, get
        $result = preg_match('/^[a-z]*/', $name, $matches);

        if($result !== 1)
        {
            throw new Exception('Action not recognized.');
        }

        // Hold the found action.
        $action = $matches[0];

        // Find the rest of the method name.
        $result = preg_match_all('/[A-Z][a-z]*/', $name, $matches);

        // $matches will hold a multi-dimensional array
        // we need the first 1D array only.
        $matches = $matches[0];

        if($result < 1 || $result === FALSE)
        {
            throw new Exception('Malformed method name.');
        }

        // Construct the property name.
        $property = '_' . strtolower(implode('_', $matches));

        if(! property_exists($this, $property))
        {
            throw new Exception("Property: '{$property}' not found.");
        }

        switch($action)
        {
            // Mutator
            case 'set':
            {
                if(count($args) === 1)
                {
                    $this->$property = $args[0];
                    return;
                }
                else
                {
                    throw new Exception('You must provide 1 argument only.');
                }
            }

            // Accessor
            case 'get':
            {
                return $this->$property;
            }
        }
    }
}
\$\endgroup\$
5
\$\begingroup\$

Exceptions are fine, but consider using less general Exception classes, like BaseException or whatever. This will allow for more flexible error reporting.

First thing that strikes me as odd about your code is that you're using underscore to name your properties, but CamelCase to name your methods. This is quite inconsistent and leads to unnecessary complexity here. Using naming convention that actually helps you is a good idea.

With that in mind, a little change in naming convention could turn into something like this:

function __call($method, $args)
{
    $action = substr($method, 0, 3);
    $prop = strtolower(substr($method, 3));
    $getter = "get".ucfirst($prop);
    $setter = "set".ucfirst($prop);

    switch ($action) {
        case "get" :
            if (isset($this->_fields[$prop])) {
                return $this->_fields[$prop];
            } else throw new EntityException("Unknown property {$prop}");
            break;
        case "set" :
            if (isset($this->_fields[$prop])) {
                $this->_fields[$prop] = $args[0];
                return null;
            } else
                throw new EntityException("Unknown property {$prop}");
            break;
        default : throw new EntityException("Unknown action {$action[0]}");
    }
}

Notice i use a an array $this->_fields to store all my fields. This goes for my second point - Safety and Reusability.

Consider this piece of code:

class User
{
    private $_name;   

    function getName(){
        return $this->_name;
    }
}

We use our getName method everywhere, but later we need to add another field - second name. We can safely insert this second field inside our getName and the code would change without taking any further actions. This is kinda the whole point behind getters and setters - incapsulating logic away.

Now with your code you'll have complications with overriding your setters, and that doesn't make it safe in any way.

Now, reusability. In your base class, you write all your fields explicitly, which doesn't seem very reusable to me. Look this code (warning, large wall of text incoming).

abstract class Entity
{
    private $_fields;
    protected $schema = array();

    function __construct(array $data = array())
    {
        /** Check that our concrete implementation has its Schema */
        if (!$this->schema) {
            throw new EntityException("Class ".get_called_class()." Schema not defined");
        }
        /**
         * mapping fields value to our schema
         *
         * @TODO Move to separate method to do this not only on Construct;
         */

        $this->_fields = array_reduce(
            $this->schema,
            function ($fields, $element) use ($data) {
                $fields[$element] = $data[$element];
                return $fields;
            }
        );
    }

    function __call($method, $args)
    {
         /** Same call implementation here as above */
    }

    /**
     * Inner getter for getter/setter overrides
     */

    protected function getField($name)
    {
        if (!isset($this->_fields[$name])) {
            throw new EntityException ("Property {$name} not found");
        }
        return $this->_fields[$name];
    }

    /**
     * Inner setter for getter/setter overrides
     */

    protected function setField($name, $value)
    {
        if (!isset($this->_fields[$name])) {
            throw new EntityException ("Property {$name} not found");
        }    
        $this->_fields[$name] = $value;
    }
}

/**
 * Class User
 *
 * Concrete implementation of Entity class
 *
 */
class User extends Entity
{
    /**
     * List of our entity fields
     * @var array
     */
    protected $schema = array("id", "name", "email", "phone");

    /**
     * method getName concrete implementation;
     *
     * @return string
     */

    function getName()
    {
        return "My name is ".$this->getField("name");
    }
}


try {
    $userData = array("id" => 1, "name" => "Andrew", "phone" => "000000000", "email" => "test@test.com");

    $user = new User($userData);

    echo $user->getName()."<br />";
    echo $user->getEmail()."<br />";
    $user->setPhone("111111111");
    echo $user->getPhone();

} catch (Exception $e) {
    echo $e->getMessage();
}

Now, if you want to create a News entity, you can simply write:

class News extends Entity
{
   protected $schema = array("id", "title", "text", "time");
}

And use it at your leisure. Later you can add active record methods to Entity, like save, delete, update and so on, and still use them on any entity you might want.

As to why i used array $_fields instead of simply listing fields - array is much more convenient when you have to map values to your fields and build queries from them.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ For reference, the ucfirst to create the getter and setter names aren't necessary. Besides the fact that function/method names in PHP are case-insensitive...you already have the method name, as $method. In fact, if the method already exists and is public, __call will never be called at all, so you don't really have to check for its existence; you already know it doesn't exist, unless private and protected are being used incorrectly. \$\endgroup\$
    – cHao
    Feb 11 '14 at 23:12
  • \$\begingroup\$ First i use strtolower to get desired property name. Then i have to use ucfirst. And case-insensitivity is one of those things that hurt me alot about PHP, so i prefer not to use it. Good point about method_exists check though, thanks, i'll made necessary edits. \$\endgroup\$
    – Eternal1
    Feb 12 '14 at 13:05

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.