Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Background

My project performs software application management; it covers in-app purchasing, product provisioning, account management, etc. I've recently been tasked to add a cost reporting component.

Code

On the code level, I've spread out my model into a few classes that represent categories or interest areas, each requiring specific dependencies. Here's a rough outline:

models/base.class.php

namespace Models;

class Base
{
    protected $db; // PDO

    public function __construct(PDO $db)
    {
    }

    // helper functions such as data mappers for one or multiple rows
}

models/ordermodel.class.php

namespace Models;

class OrderModel extends Base
{
    protected $gateway; // payment gateway to handle purchases

    public function __construct(PDO $db, Gateway $gateway)
    {
        parent::__construct($db);
        $this->gateway = $gateway;
    }

    // methods for purchase, refund, subscription cancellation, etc.
    public function updateInstallDetails(...);

    public function refundPayment(...);
}

models/provisionmodel.class.php

namespace Models;

class ProvisionModel extends Base
{
    protected $account; // user account to "anchor" queries against

    public function __construct(PDO $db, Entities\Account $account)
    {
        parent::__construct($db);
        $this->account = $account;
    }

    // this method gets called when a software update took place
    // it's called (indirectly) from a controller class
    private function updateInstallDetails(...);
}

The individual classes are accessed via a factory:

class Model
{
    private static $repository = array();

    /**
     * Generic instance loader
     * 
     * @param $name string
     * @param $fn callback generates the object instance
     */
    private static function getModelInstance($name, $fn)
    {
        if (!isset(self::$repository[$name])) {
            self::$repository[$name] = $fn();
        }
        return self::$repository[$name];
    }

    public static function getOrderModel($gateway = null)
    {
        // TODO pass payment gateway object
        return self::getModelInstance(__FUNCTION__, function() use ($gateway) {
            if ($gateway) {
                $gateway = GatewayFactory::load($gateway);
            }
            return new Models\OrderModel(SiteConfig::getDefaultDatabase(), $gateway);
        });
    }
}

Problem

The cost reporting component cross-cuts two main areas of the model, i.e. purchases, refunds and provisioning (shown above). Purchases and refunds of any product are recorded for royalty payments and provisioning keeps track of software updates (between versions there may be added or removed royalties).

My possible solutions

So I have a few options:

  1. Make both my classes aware of the reporting unit; this means extending the constructor and what not.

  2. Apply Decorator pattern to "intercept" method calls.

  3. Apply ObserverMediator pattern; each class will emit "interesting" events and the reporting unit will listen for them and make the appropriate changes to the database tables.

Questions

Does it make sense to spread out my model into separate classes in the first place? Would it make more sense to make one big class that encompasses all my application logic?

ObserverMediator pattern has its pros and cons; the advantage is that you can easily decouple units from each other, but the downside is that it's indeed decoupled. Are there other cons?

Is there another pattern I could apply to crack this? Is there some other major issue with my code?

share|improve this question
add comment

2 Answers 2

Model classes

Model classes should not have any hard dependency in their constructors especially not a PDO or similar stuff. Model can not have any business rule they are containing only data and that's all. Ofcourse if you have some mapping between model classes (Author <--> Books) then some of the relation informations have to be in the model (Mediator!) classes but that's all.

Model classes should not have any knowledge about their storage type. It can be anything not just az SQL database handled with PDO. They can not have self managed CRUD methods (if they does then they become Active Record(s) which is a really bad thing (~God object), antipattern) (maybe factory methods are okay but these methods also can not have anything heavy (query database through a horrible static global accessible thing) inside).

CRUD

Mapped entities should have some kind of top level storage (like in .NET an ObjectContext) which is dealing the storage problem; handling SQL connections and providing transaction capabilities with an Unit of Work (Commit, Rollback) solution. The top level storage can have for example mapped SQL tables for querying from the underlaying storage, inserting into it, deleting from it. Other thing is that this top level storage is the responsible to track the changes in the entities. Of course we have to put some stuff in every model in the system (Mediator #2!) to handle this session but the main logic still in a state manager not in the models.

Repository

Repositories are tricky. With powerful ORM systems we can forget them if the business logic doesn't require another application level. What should a repository contain? Add, Delete, Query methods but no Save() or anything else (not even named query methods like: GetUserByEmailAddress($email)). You can use DAO classes for named queries which are operating one or more repository or on the entire object tree. Do not forget: not every entity has a repository! Only aggregate roots can have repository, for example: an Order model can have a repo but an OrderDetail can not becouse it belongs to an Order model and can not exists whithout it.

Static

In your example your are using in static context object which are containing state (your repository stuff) information which is bad. You will introduce a global state whith them which will bring more harm to your code then good (global hyperspace is inreliable, anyone can modify it).

share|improve this answer
    
"Model can not have any business rule they are containing only data and that's all." This is so wrong. –  netcoder Feb 8 '13 at 14:18
    
The models can contain business rule like stuff, for example a SocialSecurityNumber can have some validation stuff but nothing heavy code. –  Peter Kiss Feb 9 '13 at 6:16
add comment

First, use interfaces whenever possible and make your Factories return them. If later you decide to change implementation or "hot plug" one at run-time, it will be feasible.

I think that looking at frameworks like yii or ZF2 may helps a lot, hunt for design patterns in them !

Like in zend, you may pass a "ServiceManager" and let your class grab dependencies from it.

share|improve this answer
    
ServiceManager is a Service Locator implementation which is not as good as anyone can think. Service locator is about of "i know an all knowing stuff in the global hyperspace and i'm going to ask it to get me an instance of Blah". –  Peter Kiss Feb 8 '13 at 13:49
    
If dependecy injection is used properly, it can do miracles. Angularjs seems to be a very good example of it. ServiceManager is an alternative to it. Instead of "pushing" dependecies, you permit to your objects to ask them. It permit to say "I want to store that thing so give me something to do it" and you don't care about the actual implementation. –  programaths Feb 10 '13 at 8:58
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.