Builder pattern for Codeigniter ActiveRecord queries

Question

I am using Codeigniter and it's ActiveRecord.

I had this idea for a base class that provided a generic getter/setter for doing simple queries on a database. I have a lot of database objects that could benefit from not having to duplicate the basic query patterns.

Instead of writing functions whose sole purpose were to chain a few active record queries and return the result, I could instead do it with a generic $parameters array and providing the table name via the function call.

<?php

class ORMModel extends CI_Model {

    public function __call($name, $args) {
        $method = strtolower(substr($name, 0, 3));
        $object = substr($name, 3);

        // Parsing arguments
        $params = (isset($args[0])) ? $args[0] : array();
        $single = (isset($args[1])) ? $args[1] : false;
        $count = (isset($args[2])) ? $args[2] : false;
        $page = (isset($args[3])) ? $args[3] : '';
        $per_page = (isset($args[4])) ? $args[4] : '';

        if ($method === 'get') {
            $this->db->from($object);
        } elseif ($method !== 'set') {
            throw new Exception('Unsupported method ' . $name);
        }

        if (method_exists($this, $name)) {
            $this->{$name}($params);
        } elseif ($method === 'get') {
            $this->__getter($params);
        } else {
            $this->__setter($params);
        };

        if ($method === 'set') {
            // Insert on duplicate method, for both insert/updates, this way the SET can act as the WHERE as well
            return $this->db->on_duplicate($object);
        }

        $this->db->limit($per_page, $page);
        $result = $this->db->get();
        if (!$result) {
            throw new Exception('No result from ' . $name);
        }

        if ($count) {
            return $result->num_rows;
        }

        if ($single) {
            return $result->row_array();
        } else {
            return $result->result_array();
        }
    }

    private function __getter($params) {
        foreach ($params as $field => $value) {
            if (method_exists($this, $field)) {
                $this->where{$field}($value);
            }
            $this->db->where_in($field, $value);
        }
    }

    private function __setter($params) {
        foreach ($params as $field => $value) {
            $this->db->set($field, $value);
        }
    }

}

Example usage:

// Returns all orders
$this->ORMModel->getOrder()

// Returns order ID 3
$this->ORMModel->getOrder(array('id' => 3))

// Returns orders before a certain date
function orderWhereDateBefore() {
    $this->db->where('date <=', $value);
}

$this->ORMModel->getOrder(array('dateBefore' => '2012-01-01'))

// Huge search query (from a form)
$params = array(
    'status' => 'Paid',
    'dateBefore' => '2012-01-01',
    'name' => 'Some Guy',
    ...
);
$this->ORMModel->getOrder($params);

// Set an order
$params = array(
    'status' => 'Paid',
    'dateBefore' => '2012-01-01',
    'name' => 'Some Guy',
    ...
);
$this->ORMModel->setOrder($params);

The additional arguments also provide more flexibility for standard display of results, such as pagination and full count of the result set.

However, I am concerned whether its reusability can trump the concerns of readability, usability and debuggability (for lack of a better word)Maintainability, and if so, can it be improved on?

Answer 1

Well, for a kickoff, you're concerned about your code's reusability can trump its undeniable downsides. But, if you look at one or two things more closely, your code isn't as reusable as perhaps you think it is:

// Parsing arguments
$params = (isset($args[0])) ? $args[0] : array();
$single = (isset($args[1])) ? $args[1] : false;
$count = (isset($args[2])) ? $args[2] : false;
$page = (isset($args[3])) ? $args[3] : '';
$per_page = (isset($args[4])) ? $args[4] : '';

This is just a series of hard-coded statements, with a specific goal in mind. This isn't as generic as your object, called ORMModel would have you believe.
You're also being lead astray by CI's rather confusing name ActiveRecord, which actually doesn't implement the ActiveRecord pattern.

Your code shows signs of a nasty habbit, too, in the sense that you're using magic methods and object overloading. Avoid these things as much as possible, it'll slow your app down when you're accessing/setting properties, and even more so if that requires one or more magic method calls.

There's another issue with using __call too often: it can bypass the access modifiers:

class Unsafe
{
    private function mayOnlyBeCalledInternally()
    {
        //do risky things
    }
    public function __call($method, array $args)
    {
        if (method_exists($this, $method))
        {
            return call_usr_func_array(array($this, $method), $args);
            return $this->{$method}($args);//BTW: this is WRONG
        }
    }
}
$expose = new Unsafe;
$expose->mayOnlyBeCalledInternally();//WORKS!!!!

You're, essentially, exposing all private and protected methods through this __call method. Just don't!
As you may have noticed, I'm calling the methods using call_usr_func_array. The __call method receives the arguments as an array, but that doesn't mean the called method accepts an array. It could well be that this method expects three distinct arguments, not 1 array. The way you're calling existing methods:

if (method_exists($this, $name)) {
    $this->{$name}($params);
} 

Will give you grief in such cases, but I'm going off topic slightly...

Basically, you've observed the limitations of CI's ActiveRecord and set about building something a bit more powerful. Now that's not a bad idea, except for one thing: It's all been done before, and (without wanting to suggest you're a bad dev) better than you'll be able to implement it. You're just 1 person, writing your own thing, Doctrine and the like have been around for some time, and have been worked on by many people. You can't compete with them.

If, however, you're not quite ready to switch to an existing sollution, I'd split your __call method into truly defined functions. For example, the call getOrder is being translated into get and Order, where Order is the tbl/object name. Why don't you just create a method like:

public function getFrom($tbl, array $params)
{
    $this->db->from($tbl);
    foreach($params as $field => $value)
    {
        $this->where{$field}($value);
    }
    //... and so on
}

In give time, you might find yourself creating an abstract model for your tables, and passing table objects to this function:

abstract class TableObject
{
    private $name = null;
    protected $fields = array();
    abstract public function __construct($name);
    final protected function _setName($name)
    {
        if ($this->name !== null)
        {
            throw new BadMethodCallExcpetion('Can\'t change tbl name once instance is created');
        }
        $this->name = $name;
        return $this;
    }
    final public function _getName()
    {
        return $this->name;
    }
    //bulk setter
    final public function setByRow(stdClass $record)
    {//or hint for array, or Traversable, or whatever
        foreach($record as $name => $value)
        {
            if (method_exists($this, 'set'.ucfirst($name)))
            {
                $this->{'set'.ucfirst($name)}($value);
            }
        }
    }
    //get object values as array
    final public funtion toArray()
    {
        $array = array();
        foreach($this->fields as $name)
        {
            $array[$name] = $this->{'get'.ucfirst($name)}();
        }
        return array_filter($array);
    }
}
//then
class Order extends TableObject
{
    public function __construct()
    {
        //set $this->fields here, if not in definition
        $this->_setName('Order');
    }
}

In accordance with my linked answers, it goes without saying that I'd strongly suggest your declaring all properties explicitly, and implementing a getter and setter for each property, too.
Anyhow, if you've done that, you can change the getObject method to:

public function getObject(TableObject $model)
{
    $this->db->form($model->_getName());
    $asArray = $model->toArray();//treat as $params in the snippet above
}

Now this is a more generic, and OO way of working. You could also create an TblArguments object, instead of using an array to determine if there is to be a limit, group by, count set/returned... But if I were you, I'd create specifc methods for count and single and per_page, really...

But really, read through a couple of my more verbose answers on this site, and perhaps on SO, too, I've given more detailed code examples of this approach before, and expanded on certain other benefits, too. But to recap: Your code is not terrible, not at all... it didn't make my eyes water or anything. But your __call method is doing more than it should be doing: it's checking if the particular call was meant to do a specific thing.
If you know that get + someString is meant to be a query, why bother relying on __call, why not implement a generic method for just that purpouse? it'll clean up your code, it'll be more maintainable, and even more reusable.