Page 1 of 1

How to organize models / controllers

Posted: Fri Aug 14, 2009 2:35 am
by shiznatix
So, here is my situation. I am using Zend Framework. RIght now I extend all applicable models from an abstract class that holds 2 very basic queries, a simple "select 1 row where..." and "select multiple rows where..." but they only select from 1 table and are just very basic but commonly used types of queries. This works really well right now.

But of course I have much more complex queries going on. In the past, if the query involves joining 1 or more table I have created a separate function for it. The problem is that I now have a whole lot of these functions. They are cluttering my models and don't even do anything object-oriented so they kinda seam like a waste of space to have there.

I am now trying to minimize these, if not remove them completly. I realize that I could just write all these queries in the Controllers and use the model to format the data for how the view needs it but this seams to just be moving the problem from one to the other, not actually solving the problem. Its quite difficult to read through the code when there are these really big queries everywhere.

So that brings us to my question, how do I handle these really large queries? I want to do something like I am doing for the simple queries but just have no ideas. This is how I would do a simple query to pull, for example, all the news items:

Code: Select all

$this->view->news = Singleton::getObject('News')
->setCacheTitle('newsLoggedInFrontPage'.Singleton::getObject('Languages')->fetchLanguageCode())
->setCacheTags(array('news'))
->fetchEntries(
    array(
        'headline',
        'url_headline',
        'short_story',
        'posted_time'
    ),
    array(
        'is_front_page' => '1',
        'active' => '1',
        'fk_language_code' => Singleton::getObject('Languages')->fetchLanguageCode()
    ),
    4, 'id DESC'
)
->formatForUser()->fetchData();
Which makes it small and easy to just get the data, format it, then return it. It also keeps me from having to repeat code a lot. With the long queries, I have to repeat it a lot.

One idea I had was to extend my main models with another "queries" model, so I would have like Models_News and then Models_Queries_News and the Models_Queries_News would be just a class that was a function container that housed the large queries, the same way as now but quite a bit more out of the way, a place I would hardly have to go to and debug and whatnot. But I am not sure if that is really the right way to go about it.

What do you guys do in these cases? How have you handled it? What roles do your contollers and models play and how do you extend them to add functionality?

Re: How to organize models / controllers

Posted: Mon Aug 17, 2009 3:44 am
by shiznatix
*bump*

any ideas / suggestions?

Re: How to organize models / controllers

Posted: Mon Aug 17, 2009 4:08 am
by Eran
you asked a very broad question which is actually several questions rolled into one.

- The model is the appropriate place to handle all the queries. That is part of its responsibility - to abstract the data source. I'm not sure what you consider to be complex queries, but many queries can be abstracted using a select class such as Zend_Db_Select
Bear in mind that controllers are generally not reusable. Models can be reused within the application, so if you abstract data access correctly, you will be able to use the same methods in several situations.
- Not sure how to reference your example code. Does your use of the singleton pattern here has any real purpose? why aren't you instancing objects directly?
Consider giving that singleton class a more meaningful name than 'Singleton'. That is the name of a generic pattern and tells nothing about what the role of this class (I'm assuming Registry?)

Re: How to organize models / controllers

Posted: Mon Aug 17, 2009 5:05 am
by shiznatix
Yes, I am aware its a very broad question and for that, sorry, but I have more "how to use OOP correctly" questions than specific problems.

Basically, what I have been doing with models often is just making a method for each query that joins in 1 or more table. The kinda just seams silly since it creates a whole lot of random methods that are difficult to keep track of when it comes to where is this used, do i use it in more than 1 place, and whenever I don't need it anymore, can I just delete it or will that break something. I don't want to put these queries in my controllers, it will just make the controllers cluttered but I am not sure really what to do with this all. I do use zend_db_table_abstract to make my queries which works well but still, the code gets big there.

Really, I have been leaning to just moving all queries, for lets say, the news to a class called "Queries_News" or something like that which would just be the holder for them, keep them out of sight and everything.

As for the Singleton stuff, yes, it is just a registry. I use it mostly because it keeps me from having to start the classes every time. It just saves some lines and I would imagine memory since that way I don't have to initiate class "News" every time I want to get the news, I just pull it from the registry. When it comes to things like the news, all I am doing is pulling out the news and formating it properly. This makes more sense to me but maybe its not a good idea? Why not do it this way and what benefits does making a new instance every time give?

Edit: another question I have is, should I just have 1 model per page? Like, sometimes I have complex stuff to get and I have in the past just made a separate model that really isn't part of that page but brings together other models to get the final data. Should I, instead, move these to separate classes outside of the Model area and have the model fetch these outside classes to do that work? Would that make more sense?

Re: How to organize models / controllers

Posted: Mon Aug 17, 2009 6:17 am
by Eran
I can't really comment on your models "getting big" without seeing some concrete code. Maybe if you showed something specific that troubles you, it would help
should I just have 1 model per page? Like, sometimes I have complex stuff to get and I have in the past just made a separate model that really isn't part of that page but brings together other models to get the final data
A (Domain) Model is supposed to model entities in the application domain. Entities such as users, products, bugs etc. Not specific pages - the model should not know (too much) about how its data is presented in a specific format (such as an HTML page). The more loosely coupled the models are from other layers (controllers, views) the more reusable it is inside the application.

Model public method names define the interface to that entity, and you should try to make them as descriptive as possible. For example, if you want to get all the bugs assigned to a specific user, you could write a method called getAssignedTo($userId) for the Users model. This serves two main purpose - one is to abstract the inner workings of the data source (often, the database query) from other layers, and the second is it creates a sort of domain language that is easy to relate to.

Code: Select all

$users = new Users();
$bugs = $users -> getAssignedTo($userId);
This is pretty readable, certainly a lot more than having the query inline. Abstracting complex logic into smaller chunks as methods with descriptive names makes you code more readable and maintainable. One of the big advantageous of OO is the added semantics of coupling those methods to the data they access / modify - creating an entity that is easier to relate to and understand.
I use it mostly because it keeps me from having to start the classes every time. It just saves some lines and I would imagine memory since that way I don't have to initiate class "News" every time I want to get the news, I just pull it from the registry. When it comes to things like the news, all I am doing is pulling out the news and formatting it properly. This makes more sense to me but maybe its not a good idea? Why not do it this way and what benefits does making a new instance every time give?
Using a singleton in this fashion is very similar to using global variables. By instancing the objects directly you can control their scope. Controlling scope is very important for maintainability as an application grows in complexity - if those objects can be access and modified from anywhere (remember, object are passed around by reference by default), you will start developing hard to track bugs (since you can't tell where or when something modifies the object).

Another issue which relates to productivity is that you can't use your IDE auto-completion feature. This is very important to some of us, as it gives high productivity and comfort boost.

Focusing on such micro optimizations for memory / performance sake is irrelevant. In all likelihood this will not be the bottleneck in your application (much more likely is database / IO operations), and you should always design your application for maintainability and flexibility first - making it much easier to optimize when necessary.

Re: How to organize models / controllers

Posted: Tue Aug 18, 2009 6:04 am
by shiznatix
Ok I understand I have some ideas about how to separate most everything into its own containers and have the model do its thing.

What I am gathering is that I should minimize my controllers by moving most stuff to my models, so my controllers are just dumb, they don't know what all they are really getting from the models, they just call for the info and receive it. Is that correct?

If so, then having my controller with the indexAction method like this would be wrong:

Code: Select all

public function indexAction()
{
    $this->view->layout()->title = USERS_ARTICLES_TITLE;
    
    $this->view->articles = Singleton::getObject('Articles')
    ->fetchEntries(
        array(
            'title',
            'headline',
            'url_title',
            'short_article',
        ),
        array(
            'active = "1"',
            'fk_language_code' => Singleton::getObject('Languages')->fetchLanguageCode(),
        ),
        '', 'id DESC'
    )
    ->formatForUser()->fetchData();
}
but instead I should move that code that tells what to query on to a method in the model yes? I should instead have for that controller method something like this:

Code: Select all

public function indexAction()
{
    $this->view->layout()->title = USERS_ARTICLES_TITLE;
    
    $articles = new Articles;
    $this->view->articles = $articles->fetchArticlesIndexEntries();
}
correct? That would be better practice?

The only problem I see with this is that I am just adding 1 function for each query that I use. It just seams to add in a whole lot of separate methods in my models that just hold a query but the queries are often very similar so I end up having a lot of similar methods running around. Seams a bit icky to me, its basically what I am doing right now which was one of the reasons I thought it might be a good time to do something different.

Re: How to organize models / controllers

Posted: Tue Aug 18, 2009 7:10 am
by Eran
Controllers are not necessarily dumb, but you are right that the preference is for thin controllers. Think of controllers as the part of your application the issues commands to the model. The model is aware of its inner workings but lacks context, and the controller provides context for it.

The point is that controllers are not reusable as they are very concrete implementations. On the other hand, the model is still somewhat abstract (lacks context) which makes it more reusable within the application.
It just seams to add in a whole lot of separate methods in my models that just hold a query but the queries are often very similar so I end up having a lot of similar methods running around
This is exactly where the model can help you, since it is a reusable component. You can reduce duplication by refactoring several similar methods into one method and reusing it in different contexts (controllers/actions). If you put those queries in the controllers you have almost no chance of reuse.

Re: How to organize models / controllers

Posted: Tue Aug 18, 2009 8:01 am
by shiznatix
Righty-ho. Ok so here, I think I have a good idea going on now. This is my basic articles/ page. It will either show a brief bit of all articles or all of 1 article. We shall start with the controller:

Code: Select all

<?php
 
class ArticlesController extends Zend_Controller_Action
{
    protected $pageId = 2;
    
    public function init()
    {
        Translations::loadFile(array('user/articles/view'));
        
        $this->view->layout()->pageId = $this->pageId;
        
        $this->articles = new Models_Users_Articles;
    }
    
    public function indexAction()
    {
        $this->view->layout()->title = USERS_ARTICLES_TITLE;
        
        $this->view->articles = $this->articles->fetchActiclesIndexArticles();
        
        //this has to be here incase a non-existant
        //article title is tried in the articleAction method
        $this->render('index');
    }
    
    public function articleAction()
    {
        $this->view->article = $this->articles
        ->set('title', $this->_getParam('title'))
        ->set('languageCode', Singleton::getObject('Languages')->fetchLanguageCode())
        ->fetchArticle();
        
        if (!$this->view->article && Singleton::getObject('Languages')->fetchDefaultLanuageCode() != Singleton::getObject('Languages')->fetchLanguageCode())
        {
            $this->view->article = $this->articles
            ->set('title', $this->_getParam('title'))
            ->set('languageCode', Singleton::getObject('Languages')->fetchDefaultLanuageCode())
            ->fetchArticle();
        }
        
        if (!$this->view->article)
        {
            return $this->indexAction();
        }
        
        $this->view->layout()->title = $this->view->article['title'];
    }
}
then Models/Articles.php

Code: Select all

<?php
 
class Models_Articles extends Models_Queries_Articles
{
    protected $modelName = 'Articles';
    protected $tableName = 'rb_articles';
    
    protected $defaultCacheTags = array('articles');
    
    public function set($var, $val)
    {
        $this->$var = $val;
        
        return $this;
    }
}
then Models/Queries/Articles.php

Code: Select all

<?php
 
class Models_Queries_Articles extends Abstracts_Table
{
    /*
     Used in: Users/ArticlesController.php - indexAction()
    */
    public function fetchActiclesIndexArticles()
    {
        return $this->fetchEntries(
            array(
                'title',
                'headline',
                'url_title',
                'short_article',
            ),
            array(
                'active = "1"',
                'fk_language_code' => Singleton::getObject('Languages')->fetchLanguageCode(),
            ),
            '', 'id DESC'
        )
        ->formatForUser()
        ->fetchData();
    }
    
    /*
     Used in: Users/ArticlesController.php - articleAction()
    */
    public function fetchArticle()
    {
        return $this->fetchEntry(
            array(
                'title',
                'headline',
                'full_article',
            ),
            array(
                'url_title' => $this->title,
                'active = "1"',
                'fk_language_code' => $this->languageCode,
            )
        )
        ->fetchData();
    }
}
and last Models/Users/Articles.php

Code: Select all

<?php
 
class Models_Users_Articles extends Models_Articles
{
    public function formatForUser()
    {
        foreach ($this->data as &$article)
        {
            if (isset($article['shortArticle']))
            {
                $article['shortArticle'] = str_replace('<p>', '<p style="display: inline;">', $article['shortArticle']);
            }
        }
        
        return $this;
    }
}
My reasoning for having a Models/Users/Articles.php and not just put everything in Models/Articles.php is because I have a whole admin section that I link in as well (Models/Admin/Articles.php) which, although rarely, sometimes does use the same queries that I use in the users section of the website so I can put all queries that deal with the articles in the Queries/Articles and so I can use them between the admin and the users.

How is this setup? In the right direction, heading towards the right direction, or just crashed into a train?

Re: How to organize models / controllers

Posted: Tue Aug 18, 2009 8:55 am
by Eran
You're moving along, but it can still be much improved. You have this method:

Code: Select all

public function fetchActiclesIndexArticles()
    {
       ...
    }
Which does something very specific. The query process inside looks pretty inflexible as well. How about:

Code: Select all

public function fetchActiclesIndexArticles() {
        $filters = array(
             'active = "1"',
             'fk_language_code' => Singleton::getObject('Languages')->fetchLanguageCode(),
        );
                
        $columns = array(
            'title',
            'headline',
            'url_title',
            'short_article',
        );
        
        return $this -> fetchArticles('id DESC',$filters,$columns); 
    }
    public function fetchArticles($order = 'id DESC',$filters = array(),$columns = array('*')) {
        return $this->fetchEntries(
            $columns,
            $filters,
            '', 
            $order
        )
        ->formatForUser()
        ->fetchData();
    }
 
As an example of a more generic/reusable method. This is just an example, you can take it to where you wish.

Re: How to organize models / controllers

Posted: Tue Aug 18, 2009 9:34 am
by shiznatix
I already am doing something of the sort in the Abstracts_Table class:

Code: Select all

<?php
 
abstract class Abstracts_Table
{
    protected $data;
    
    private $_lastSQL;
    private $_cacheTags;
    
    protected $dbFields = array();
    
    public function setData($data)
    {
        $this->data = $data;
    }
    
    public function fetchData()
    {
        return $this->data;
    }
    
    public function clearCache()
    {
        $this->_cacheTags = null;
    }
    
    public function setCacheTags($tags)
    {
        $this->_cacheTags = $tags;
        
        return $this;
    }
    
    public function cache()
    {
        return Singleton::get('cache')->save($this->fetchData(), $this->fetchCacheTitle(), $this->fetchCacheTags());
    }
    
    public function loadCache()
    {
        return Singleton::get('cache')->load($this->fetchCacheTitle());
    }
    
    public function fetchCacheTitle()
    {
        return md5($this->_lastSQL);
    }
    
    public function fetchCacheTags()
    {
        return (is_array($this->_cacheTags) ? $this->_cacheTags : $this->defaultCacheTags);
    }
    
    public function setDbField($var, $val)
    {
        $this->dbFields[$var] = $val;
        
        return $this;
    }
    
    public function clearFbFields()
    {
        $this->dbFields = array();
        
        return $this;
    }
    
    public function fetchEntries($fields = '*', $where = array(), $limit = '', $order = '', $group = '')
    {
        $select = Singleton::getObject($this->modelName, 'table')->select()
        ->from($this->tableName, $fields)
        ->order($order)
        ->limit($limit);
        
        if ($group)
        {
            $select->group($group);
        }
    
        if (count($where))
        {
            foreach ($where as $field => $val)
            {
                if (is_int($field))
                {
                    $select->where($val);
                }
                else if (is_array($val))
                {
                    switch ($val[0])
                    {
                        case 'LIKE':
                            $select->where($field.' LIKE ?', new Zend_Db_Expr('"'.$val[1].'"'));
                            break;
                    }
                }
                else
                {
                    $select->where($field.' = ?', $val);
                }
            }
        }
        
        $this->_lastSQL = $select->__toString();
        
        if ($cache = $this->loadCache())
        {
            $this->setData($cache);
        }
        else
        {
            $this->doSelect($select);
            $this->cache();
        }
        
        $this->clearCache();
        
        return $this;
    }
    
    public function fetchEntry($fields = '*', $where = array(), $order = '')
    {
        $select = Singleton::getObject($this->modelName, 'table')->select()
        ->from($this->tableName, $fields)
        ->order($order)
        ->limit(1);
        
        if (count($where))
        {
            foreach ($where as $field => $val)
            {
                if (is_int($field))
                {
                    $select->where($val);
                }
                else
                {
                    $select->where($field.' = ?', $val);
                }
            }
        }
        
        $this->_lastSQL = $select->__toString();
        
        if ($cache = $this->loadCache())
        {
            $this->setData($cache);
        }
        else
        {
            $this->doSelect($select, false);
            $this->cache();
        }
        
        $this->clearCache();
        
        return $this;
    }
    
    public function doSelect($select, $multipleRows = true)
    {
        if ($multipleRows)
        {
            try
            {
                $data = Singleton::getObject($this->modelName, 'table')->fetchAll($select)->toArray();
            }
            catch (Zend_Exception $e)
            {
                //trigger_error('Exception caught: '.print_r($e, true), E_USER_ERROR);
                return false;
            }
            
            $this->setData(array_map('arrayKeysToCamelCase', $data));
        }
        else
        {
            try
            {
                $data = Singleton::getObject($this->modelName, 'table')->fetchRow($select);
            }
            catch (Zend_Exception $e)
            {
                //trigger_error('Exception caught: '.print_r($e, true), E_USER_ERROR);
                return false;
            }
 
            if ($data)
            {
                $data = $data->toArray();
                $this->setData(arrayKeysToCamelCase($data));
            }
        }
    }
}
Plus there is a difference between fetchEntries() and fetchEntry() in how the data is returned.

It seams that if I did the way you suggest it would just be adding a second layer where its not really needed. What if I did something like this:

Code: Select all

public function setActiclesIndex()
    {
        $this->fetchEntries(
            array(
                'title',
                'headline',
                'url_title',
                'short_article',
            ),
            array(
                'active = "1"',
                'fk_language_code' => $this->dbFields['languageCode'],
            ),
            '', 'id DESC'
        );
        
        return $this;
    }
and then in the controller:

Code: Select all

public function indexAction()
    {
        $this->view->layout()->title = USERS_ARTICLES_TITLE;
        
        $this->view->articles = $this->articles
        ->setDbField('languageCode', Singleton::getObject('Languages')->fetchLanguageCode())
        ->setActiclesIndex()
        ->formatForUser()
        ->fetchData();
        
        //this has to be here incase a non-existant
        //article title is tried in the articleAction method
        $this->render('index');
    }
that way the controller tells how it wants the data and when to get it. The Articles model just gets the data and holds it, gets told to format it in a certain way and does, then returns it when its finally ready. The controller does all the instructing on what it wants while the model just has the means to do it. Yes the queries methods are not super re-usable (only if you use the exact same query in 2 different places) but thats to be expected yes?