Cleaning up my Application Structure... Domain Objects and Data Mappers

Question

I've been developing an application utilizing ZendFramework 1.1 for the better part of two years now, and as-so it has seen a few different stages of refactoring from me learning or trying something new. At its current state, I feel that my structure is pretty good in that I can get stuff done quickly, but could certainly use some improvements in certain areas- where I feel like there is a lot of bloat and awkward dependencies.

Bear with me here as I lay down some example code from my application. I will use an example of an Order object which has OrderItem instances which also must be saved. I will explain all necessary parts of instantiation and saving.

As far as my understanding goes, what I've got going on here is more-so in line with the ActiveRecord design pattern than with Domain Models, though I think I have practices from both...

class Order extends BaseObject {
    /** @var OrderItem array of items on the order */
    public $items = array();

    public function __construct($data = array()){

        // Define the attributes for this model
        $schema = array(
            "id" => "int", // primary key
            "order_number" => "string", // user defined
            "order_total" => "float", // computed
            // etc...
        );

        // Get datamapper and validator classes
        $mf = MapperFactory::getInstance();
        $mapper = $mf->get("Order");
        $validator = new Order_Validator();
        $table = new Application_DbTable_Order();

        // Construct parent
        parent::__construct($schema, $mapper, $validator, $table);

        // If data was provided then parse it
        if(count($data)){
            $this->parseData($data);
        }

        // return the instance
        return $this;
    }

    // Runs before a new instance is saved, does some checks
    public function addPrehook(){
        $orderNumber = $this->getOrderNumber();
        if($this->mapper->lookupByOrderNumber($orderNumber)){
            // This order number already exists!
            $this->addError("An order with the number $orderNumber already exists!");
            return false;
        }

        // all good!
        return true;
    }

    // Runs after the primary data is saved, saves any other associated objects e.g., items
    public function addPosthook(){
        // save order items
        if($this->commitItems() === false){
            return false;
        }

        // all good!
        return true;
    }

    // saves items on the order
    private function commitItems($editing = false){
        if($editing === true){
            // delete any items that have been removed from the order
            $existingOrder = Order::getById($this->getId());
            $this->deleteRemovedItems($existingOrder);
        }

        // Iterate over items
        foreach($this->items as $idx => $orderItem){
           // Ensure the item's order_id is set!
           $orderItem->setOrderId($this->getId());

           // save the order item
           $saved = $orderItem->save();
           if($saved === false){
               // add errors from the order item to this instance
               $this->addError($orderItem->getErrors());

               // return false
               return false;
           }

           // update the order item on this instance
           $this->items[$idx] = $saved;
        }

        // done saving items!
        return true;
    }

    /** @return Order|boolean The order matching provided ID or FALSE if not found */
    public static function getById($id){
        // Get the Order Datamapper
        $mf = MapperFactory::getInstance();
        $mapper = $mf->get("Order");

        // Look for the primary key in the order table
        if($mapper->lookup($id)){
            return new self($mapper->fetchObjectData($id)->toArray());
        }else{
            // no order exists with this id
            return false;
        }
    }
}

The parsing of data, saving, and pretty much anything else that applies to all models (a more appropriate term may be Entity) exists in the BaseObject, as so:

class BaseObject {
    /** @var array Array of parsed data */
    public $data;

    public $schema; // valid properties names and types
    public $mapper; // datamapper instance
    public $validator; // validator instance
    public $table; // table gateway instance

    public function __construct($schema, $mapper, $validator, $table){
        // raise an error if any of the properties of this method are missing

        $this->schema = $schema;
        $this->mapper = $mapper;
        $this->validator = $validator;
        $this->table = $table;
    }

    // parses and validates $data to the instance
    public function parseData($data){
        foreach($data as $key => $value){

            // If this property isn't in schema then skip it
            if(!array_key_exists($key, $this->schema)){
                continue;
            }

            // Get the data type of this
            switch($this->schema[$key]){
                case "int": $setValue = (int)$value; break;
                case "string": $setValue = (string)$value; break;
                // etc...
                default: throw new InvalidException("Invalid data type provided ...");
            }

            // Does our validator have a handler for this property?
            if($this->validator->hasProperty($key) && !$this->validator->isValid($key, $setValue)){
                $this->addError($this->validator->getErrors());
                return false;
            }

             // Finally, set property on model
            $this->data[$key] = $setValue;    
        }
    }

    /**
     * Save the instance - Inserts or Updates based on presence of ID
     * @return BaseObject|boolean The saved object or FALSE if save fails
     */
    public function save(){
        // Are we editing an existing instance, or adding a new one?
        $action   = ($this->getId()) ? "edit" : "add";
        $prehook  = $action . "Prehook";
        $posthook = $action . "Posthook";

        // Execute prehook if its there
        if(is_callable(array($this, $prehook), true) && $this->$prehook() === FALSE){
            // some failure occured and errors are already on the object
            return false;
        }

        // do the actual save
        try{ 
            // mapper returns a saved instance with ID if creating
            $saved = $this->mapper->save($this);
        }catch(Exception $e){
            // error occured saving
            $this->addError($e->getMessage());
            return false;
        }

        // run the posthook if necessary
        if(is_callable(array($this, $posthook), true) && $this->$posthook() === FALSE){
            // some failure occured and errors are already on the object
            return false;
        }

        // Save complete!
        return $saved;
    }
}

The base DataMapper class has very simple implementations for save, insert and update, which are never overloaded because of the $schema being defined per-object. I feel like this is a bit wonky, but it works I guess? Child classes of BaseMapper essentially just provide domain-specific finder functions e.g., lookupOrderByNumber or findUsersWithLastName and other stuff like that.

class BaseMapper {
    public function save(BaseObject $obj){
        if($obj->getId()){
            return $this->update($obj);
        }else{
            return $this->insert($obj);
        }
    }

    private function insert(BaseObject $obj){
        // Get the table where the object should be saved
        $table = $obj->getTable();

        // Get data to save
        $saveData = $obj->getData();

        // Do the insert
        $table->insert($saveData);

        // Set the object's ID
        $obj->setId($table->getAdapter()->getLastInsertId());

        // Return the object
        return $obj;
    }
}

I feel like what I have isn't necessarily horrible, but I also feel like there are some not-so-great designs in place here. My concerns are primarily:

Models have a very rigid structure which is tightly coupled to the database table schema, making adding/removing properties from the model or database table a total pain in the butt! I feel like giving all of my objects which save to the database a $table and $mapper in the constructor is a bad idea... How can I avoid this? What can I do to avoid defining $schema?

Validation seems a bit quirky as it is tied very tightly to the property names on the model which also correspond to column names in the database. This further complicates making any database or model changes! Is there a more appropriate place for validation?

DataMappers don't really do much besides provide some complicated finder functions. Saving complex objects is handled entirely by the object class itself (e.g., Order class in my example. Also is there an appropriate term for this type of object, other than 'complex object'? I say that my Order object is "complex" because it has OrderItem objects that it must also save. Should a DataMapper handle the saving logic that currently exists in the Order class?

Many thanks for your time and input!

Answer 1

It's a good practice to separate the concerns between objects as much as possible. Have one responsible for Input Validation, other to perform the business logic, DB operations, etc. In order to keep the 2 objects loosely coupled they should not know anything about each other’s implementation only what they can do. This is defined thru an interface.

I recommend reading this article http://www.javaworld.com/article/2072302/core-java/more-on-getters-and-setters.html and other ones from this guy. He's got a book as well worth reading http://www.amazon.com/Holub-Patterns-Learning-Looking-Professionals/dp/159059388X.

I would separate if possible order and items, I don’t know much about your app but if you need to show a list of 20 orders only with their order numbers then those DB calls and processing regarding order items would be a waste if not separated. This is of course not the only way.

So first you need to know what the order attributes are and encapsulate a way to feed those into an order and also have an order expose that data to other objects.

interface OrderImporter {

    public function getId();

    public function getOrderNumber();

    public function getTotal();
}

interface OrderExporter {

    public function setData($id, $number, $total);
}

In order to keep the business logic separate from the database we need to encapsulate that behavior as well like so

interface Mapper {

    public function insert();

    public function update();

    public function delete();
}

Also I would define a specific mapper whose duty is to handle DB operations regarding orders.

interface OrderMapper extends Mapper {

    /**
     * Returns an object that captures data from an order
     * @return OrderExporter
     */
    public function getExporter();

    /**
     * @param string $id
     * @return OrderImporter
     */
    public function findById($id);
}

Finally an order needs to be able to communicate with all those objects through some messages.

interface Order {

    public function __construct(OrderImporter $importer);

    public function export(OrderExporter $exporter);

    public function save(OrderMapper $orderRow);
}

So far we have a way to provide data to the Order, a way to extract data from the order and a way to interact with the db.

Below I've provided a pretty simple example implementation which is far from perfect.

class OrderController extends Zend_Controller_Action {

    public function addAction() {
        $requestData = $this->getRequest()->getParams();
        $orderForm = new OrderForm();

        if ($orderForm->isValid($requestData)) {
            $orderForm->populate($requestData);
            $order = new ConcreteOrder($orderForm);

            $mapper = new ZendOrderMapper(new Zend_Db_Table(array('name' => 'order')));
            $order->save($mapper);
        }
    }

    public function readAction() {
        //if we need to read an order by id
        $mapper = new ZendOrderMapper(new Zend_Db_Table(array('name' => 'order')));
        $order = new ConcreteOrder($mapper->findById($this->getRequest()->getParam('orderId')));
    }

}

/**
 * Order form can be used to perform validation and as a data provider
 */
class OrderForm extends Zend_Form implements OrderImporter {

    public function init() {
        //TODO setup order input validators
    }

    public function getId() {
        return $this->getElement('orderID')->getValue();
    }

    public function getOrderNumber() {
        return $this->getElement('orderNo')->getValue();
    }

    public function getTotal() {
        return $this->getElement('orderTotal')->getValue();
    }

}

/**
 * This mapper also serves as an importer and an exporter
 * but clients don't know that :)
 */
class ZendOrderMapper implements OrderMapper, OrderImporter, OrderExporter {

    /**
     * @var Zend_Db_Table_Abstract
     */
    private $table;
    private $data;

    public function __construct(Zend_Db_Table_Abstract $table) {
        $this->table = $table;
    }

    public function setData($id, $number, $total) {
        $this->data['idColumn'] = $id;
        $this->data['numberColumn'] = $number;
        $this->data['total'] = $total;
    }

    public function delete() {
        return $this->table->delete(array('id' => $this->data['id']));
    }

    public function insert() {
        return $this->table->insert($this->data);
    }

    public function update() {
        return $this->table->update($this->data, array('id' => $this->data['id']));
    }

    public function findById($id) {
        $this->data = $this->table->fetchRow(array('id' => $id));
        return $this;
    }

    public function getId() {
        return $this->data['idColumn'];
    }

    public function getOrderNumber() {
        return $this->data['numberColumn'];
    }

    public function getTotal() {
        return $this->data['total'];
    }

    public function getExporter() {
        return $this;
    }

}

class ConcreteOrder implements Order {

    private $id;
    private $number;
    private $total;

    public function __construct(OrderImporter $importer) {
        //initialize this object
        $this->id = $importer->getId();
        $this->number = $importer->getOrderNumber();
        $this->total = $importer->getTotal();
    }

    public function export(\OrderExporter $exporter) {
        $exporter->setData($this->id, $this->number, $this->total);
    }

    public function save(\OrderMapper $mapper) {
        $this->export($mapper->getExporter());

        if ($this->id === null) {
            $this->id = $mapper->insert();
        } else {
            $mapper->update();
        }
    }

}