8
\$\begingroup\$

I'm working on a PHP based web application.

While building the UserStorage class, (which acts as a storage source for user data, and implements the UserStorageInterface), I noticed something. The get method fetches the user data from the storage source, and returns it:

public function get($identifier = null, $alias = null, $email = null) {
    //Fetch the data from MySQL (PDO)
}

Here, identifier, alias and email all are unique to each user. The reason I made null their default value, is that the method requires only one of them to execute.

I thought, why not put every needed parameter in a object? Then I can limit/extend the functionality of the method as desired, through the provided parameters. Thus I decided on a class named Schema (which means a plan of action or something like that, I believe). Please correct me if the term I used is the wrong one.

The Schema class:

namespace Whelp\Commons\Schema;

use Whelp\Commons\DataTypes\Int;
use Whelp\Commons\DataTypes\String;

class Schema {

    private $source = [];
    private $params = [];
    private $conditions = [];

    private $readyForUse = [];

    public function __construct(array $source = [], array $params = []) {
        $this->source = $source;
        $this->params = $params;
        $this->validate();
    }

    /**
     * @throws \InvalidArgumentException if a parameter which is not optional, and MUST be provided, is missing
     */

    private function validate() {

        if(array_key_exists('conditions', $this->source)) {
            $this->conditions = $this->source['conditions'];
            unset($this->source['conditions']);
        }

        foreach ($this->source as $param => $prop) {

            //If the schema parameter is not optional and the optional key has not been omitted, the provided parameters must contain the schema parameter
            if( array_key_exists('optional', $prop) && !$prop['optional'] && !array_key_exists($param, $this->params)) {
                throw new \InvalidArgumentException(sprintf("Compulsory parameter %s missing", $param));
            }

            //If the schema parameter is specified as optional, and is not present in the provided parameters, simply skip it.
            if( array_key_exists('optional', $prop) && $prop['optional'] && !array_key_exists($param, $this->params) ) {
                continue;
            }

            //If the type of the schema parameter is not provided, the schema is of no use, honestly. So throw an exception.
            if( !array_key_exists('type', $prop) || !$prop['type'] ) {
                throw new \InvalidArgumentException(sprintf("Type property of parameter %s missing from Schema source", $param));
            }

            if(!($this->readyForUse[$param] = $this->convertType($prop['type'], $this->params[$param]))){
                throw new \InvalidArgumentException;
            }

        }

        $this->resolveConditions();

    }

    /**
     * @param string $type
     * @param mixed $value
     * @return mixed
     */

    private function convertType($type, $value) {

        switch ($type) {

            case 'string':

                if($value instanceof String) {
                    return $value->val();
                }

                return strval($value);

                break;

            case 'int':

                if($value instanceof Int) {
                    return $value->val();
                }

                return (int) $value;

                break;

            default:

                return false;

        }

    }

    /**
     * Helper function
     * Used to check if the schema imposes a specific condition
     * @param $condition
     * @return bool
     */
    private function hasCondition($condition) {
        return array_key_exists($condition, $this->conditions);
    }

    /**
     * Helper function
     * Used to get the value of a condition exposed by the schema
     * @param $condition
     * @return null
     */
    private function getCondition($condition) {
        return $this->hasCondition($condition) ? $this->conditions[$condition] : null;
    }

    private function exception($condition, $message) {
        throw new \InvalidArgumentException(sprintf("Condition %s not met by provided parameters : %s", $condition, $message));
    }

    /**
     * Resolve the conditions imposed by the schema
     * @throws \InvalidArgumentException if a condition is not followed by the provided parameters
     * return void
     */
    private function resolveConditions() {

        if($this->hasCondition('optional_atleast')) {

            $condition = $this->getCondition('optional_atleast');

            if(!$this->atleast($condition)) {
                $this->exception('optional_atleast', sprintf("atleast %d optional parameters are required", $condition));
            }

        }

    }

    /**
     * Get the ready to use (type converted) value of a parameter
     * @param string $param
     * @return mixed
     */

    public function get($param) {
        return array_key_exists($param, $this->readyForUse) ?
            $this->readyForUse[$param] :
            null;
    }

    /**
     * Check if an optional param was provided with the schema
     * @param $param
     * @return bool
     */

    public function has($param) {
        return array_key_exists($param, $this->readyForUse);
    }

    /**
     * Check if atleast $num optional parameters are provided with the schema
     * @param $num
     * @return bool
     */

    public function atleast($num) {

        $count = 0;

        foreach ($this->source as $param => $prop)
            if(array_key_exists('optional', $prop) && $prop['optional'] && array_key_exists($param, $this->params)) {
                if($count>=$num) { return true; }
                $count++;
            }

        return $count >= $num;

    }

    /**
     * Calls a closure for every parameter in the schema
     * @param callable $callable
     * @return void
     */

    public function forEachParam(\Closure $callable) {
        $abort = false;
        foreach ($this->readyForUse as $param => $val) {
            call_user_func($callable, $param, $val, function () use (&$abort) {
                $abort = true;
            });
            if($abort) break;
        }
    }

}

The main idea behind this was, that there will be a defined plan of action for each method. When a call is made to a method along with a Schema, then the Schema contains every information the method needs, along with optional information that can be helpful in some way.

To define a Schema, I extend the Schema class, and override its constructor:

namespace Whelp\Commons\Schema\Schemas\User;


use Whelp\Commons\Schema\Schema;

class IdentificationSchema extends Schema {

    public function __construct($param) {

        parent::__construct([

            'identifier' => [

                'optional' => true,

                'type' => 'int'

            ],

            'alias' => [

                'optional' => true,

                'type' => 'string'

            ],

            'email' => [

                'optional' => true,

                'type' => 'string'

            ],

            'conditions' => [

                'optional_atleast' => 1

            ]

        ], $param);

    }

}

What the Schema class does:

  1. It ensures that every parameter which is not optional, is present in the parameters.

  2. It automatically type converts the parameters to the type expected by the method (still working on it).

So now, my get method of UserStorage is:

public function get(IdentificationSchema $schema) {

    $identifierKey = null;
    $identifierValue = null;

    //Here I am determining which identifier has been provided in the schema

    $schema->forEachParam(function ($param, $value, $abort) use(&$identifierKey, &$identifierValue) {
        $identifierKey = $param;
        $identifierValue = $value;
        call_user_func($abort);
    });

    //Again, it doesn't make sense why one won't provide me a identifier. Here I throw an exception again if the identifier is not provided.

    if(is_null($identifierKey) || is_null($identifierValue)) throw new \InvalidArgumentException("Atleast 1 identifier is required to fetch user data");

    //And here's a common step, preparing the query. Since ATTR_EMULATE_PREPARES if disabled, all exceptions are thrown here (I think).
    //I'm just leaving the exceptions thrown here, to be caught by the process which instantiated me.

    $query = $this->handle->prepare(
        sprintf("SELECT * FROM %s WHERE %s=:%s", 'users', $identifierKey, $identifierKey)
    );

    //And here I execute the query

    $query->execute([$identifierKey => $identifierValue]);

    //If no row is returned, someone has tried to loot us (probably). Hehehe. Not funny, so I'll throw an exception here as well.

    if(!$query->rowCount()) {
        throw new UserDoesNotExist(sprintf("User with %s %s does not exist", $identifierKey, $identifierValue));
    }

    //Finally, my task here is done. I'll return the user schema now. Bye bye~~~!!

    return new UserSchema(
        $query->fetch(\PDO::FETCH_ASSOC)
    );

}

Although this use case doesn't really make much use of the Schema, here's another one:

namespace Whelp\Commons\Schema\Schemas\User;


use Whelp\Commons\Schema\Schema;

/**
 * Represents the schema for creating a new user
 * Class NewUserSchema
 * @package Whelp\Commons\Schema\Schemas\User
 */
class NewUserSchema extends Schema {

    public function __construct($params) {

        parent::__construct([

            'alias' => [
                'optional' => false,
                'type' => 'string'
            ],

            'email' => [
                'optional' => false,
                'type' => 'string'
            ],

            'name' => [
                'optional' => false,
                'type' => 'string'
            ]

        ], $params);

    }

}

This Schema is provided to the create method of UserStorage.

public function create(NewUserSchema $schema) {

    $query = $this->handle->prepare(
        sprintf("INSERT INTO %s ('alias', 'name', 'email') VALUES (:alias, :name, :email)", 'users')
    );

    $query->execute([
        'alias' => $schema->get('alias'),
        'name' => $schema->get('name'),
        'email' => $schema->get('email')
    ]);

    return new Identifier($this->handle->lastInsertId());

}

The main point here is that the create method will always know, that if the execution flow has reached upto this point, the Schema contains all the necessary information. If it wouldn't have, then it would have thrown an InvalidArgumentException already.

So, when I will implement a method for updating the user data,

What it could've been :

public function ($identifier = null, $oldEmail = null, $oldAlias = null, $dataToUpdate = []) {

}

What it will be now :

public function (IdentificationSchema $identification, UpdateUserSchema $update){

}

Where UpdateUserSchema contains all the data which needs to be updated :

namespace Whelp\Commons\Schema\Schemas\User;


use Whelp\Commons\Schema\Schema;

/**
 * Represents the schema for updating user data
 * Class UpdateUserSchema
 * @package Whelp\Commons\Schema\Schemas\User
 */
class UpdateUserSchema extends Schema {

    public function __construct($params) {

        parent::__construct([

            'alias' => [
                'optional' => true,
                'type' => 'string'
            ],

            'email' => [
                'optional' => true,
                'type' => 'string'
            ],

            'name' => [
                'optional' => true,
                'type' => 'string'
            ]

        ], $params);

    }

}

Usually, I'm an all-for-readability guy. I follow the fact that the processing power of computers will keep on increasing, but the developers after me will still have to read the same code. That's why I don't mind some decrease in performance.

But, if this poses a drastic decrease in performance, then it can't be overlooked.

I'd like to know your thoughts about this approach. And by thoughts, I mean:

  • Your thoughts on its affect on the readability, scalability, and efficiency of the code, whether positive or negative.
  • Your thoughts on the decrease in performance caused.
  • Your thoughts on whether it's a step worth taking, in trade off for performance.
\$\endgroup\$
  • \$\begingroup\$ For any "performance" question, I like to say: Develop what you're comfortable with and profile and optimize when you have to. This link applies to all frameworks and applications (including yours) when it involves the issues you mentioned. You shouldn't have to worry about performance until you benchmark and profile your code. Then you can find the bottleneck of your application and resolve the problem from where the problem really originates. Not where the problem "might" be. \$\endgroup\$ – Tek Jan 22 '15 at 20:15
  • \$\begingroup\$ @Tek Yes of course, the benchmarks can be saved for later. But the main purpose of this question, was whether this approach simplifies the understanding and readability of the code, or makes it more complex. And if there are going to be drastic changes in performance, then I'd rather optimize the framework now, than after building the application upon it. \$\endgroup\$ – Abhishek Goyal Jan 22 '15 at 21:44
  • \$\begingroup\$ Technically all code is complex, its abstraction of those layers that makes complex code simple. From the top level I dont see anything that would be performance constraining. Plus such a small piece of code sample is not enough to judge to answer your points and the impact on your framework. Like I said, write first optimize second. You won't know what you need to optimize before writing the program. If you try to optimize before writing the code it's pretty much micro-optimization \$\endgroup\$ – Tek Jan 22 '15 at 22:14
  • 1
    \$\begingroup\$ I'll try to update the question with some better use cases of the approach. I see now, what you meant. You're right, I can't optimize anything if i don't have anything to test. \$\endgroup\$ – Abhishek Goyal Jan 22 '15 at 22:18
2
\$\begingroup\$

You appear to have three ways to uniquely identify a row in the database representing a user. The UserStorage class appears to be an implementation of the Repository Pattern. The architectural problem here lies with the Schema classes. You are creating several new classes to reduce the numbers of arguments to your UserStorage methods and to hold the data used in a database operation. It would be better to create a Domain Model for a user, and have the UserStorage class just deal with the User Domain Model.

The UserStorageInterface with strongly defined methods for finding users:

interface UserStorageInterface
{
    public function findById($id);
    public function findByAlias($alias);
    public function findByEmail($email);
    public function update($user);
    public function create($user);
    public function remove($user);
}

The UserStorage class implementing the interface:

class UserStorage implements UserStorageInterface
{
    public function findById($id) {
        $data = // ... execute query and find by id

        return $this->mapToUser($data);
    }

    public function findByAlias($alias) {
        $data = // ... execute query and find by alias

        return $this->mapToUser($data);
    }

    public function findByEmail($email) {
        $data = // ... execute query and find by email

        return $this->mapToUser($data);
    }

    private function mapToUser($data) {
        $user = new User($data['id']);
        $user->setAlias($data['alias']);
        $user->setEmail($data['email']);

        return $user;
    }

    public function create($user) {
        // INSERT
        $user->setId(/* new Id */);
    }

    public function update($user) {
        // UPDATE
    }

    public function remove($user) {
        // DELETE
    }
}

Lastly, the User Domain Model:

class User
{
    private $id;
    private $alias;
    private $oldAlias;
    private $oldEmail;
    private $email;

    public __construct($id = null) {
        if (isset($id)) {
            $this->setId($id);
        }
    }

    public function getId() {
        return $this->id;
    }

    public function setId($id) {
        $this->id = $id;
    }

    public function getAlias() {
        return $this->alias;
    }

    public function setAlias($alias) {
        $this->oldAlias = $this->alias;
        $this->alias = $alias;
    }

    public function getOldAlias() {
        return $this->oldAlias;
    }

    public function getEmail() {
        return $this->email;
    }

    public function setEmail($email) {
        $this->oldEmail = $this->email;
        $this->email = $email;
    }

    public function getOldEmail() {
        return $this->oldEmail;
    }
}

You don't need the Schema classes at all. Breaking your application into these layers has several advantages:

  1. Fewer classes than your current design (User and UserStorage)
  2. Each class does one thing, and does it well
    • User: The Domain Model representing rows in a database. Additional methods can be added to this class to encapsulate business logic, such as changing email address, etc.
    • UserStorage: Implements the CRUD operations of the database, dealing strictly with the User domain model
  3. Each identifying field for the User domain model has its own UserStorage method. Strongly defined interfaces in the data access layer will be easier to maintain than the fuzzy interface of "search by id, alias or email but not more than one"
  4. Removes an unnecessary external dependency to Whelp\Commons\Schema\Schema from your application
\$\endgroup\$

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.