Is calling models from other models bad practice?

I know it's possible to call models from other models, but I can't make up my mind whether it's a good thing or not.

Take this example (watered down for simplicity):

PHP Code:

class Product_model extends CI_Model {

    // Get a product
    public function get_product($product_id) {
        $query = $this->db->select('product_name, price, stock_level, created_by')
                      ->from('product')
                      ->where('product_id', $product_id)
                      ->get();

        $product = $query->row_array();

        // Find out which user created this product
        $product['user'] = $this->user_model->get_user($product['created_by']);

        // Get product images
        $product['images'] = $this->image_model->get_images('product', $product_id);

        return $product;
    }
} 


So when fetching a product using the Product model above, it's also calling the User model and Image model to fetch the user and images associated with that product.  Sometimes though I feel my models should not be so dependent on one another.

However, the other option is to move the call to the User and Image models out of the Product model and into the controllers instead.  The issue here is that several controllers might access $this->product_model->get_product() and so you would also have to do $this->user_model->get_user() and $this->image_model->get_images() several times over within every controller that requires product information.  This begins to seriously clutter up the controllers.  It also means if you introduce a new sub-model in the future that's associated with Products you'd potentially have to go through and call it from many different controllers, rather than just adding a call to it from within the Product model.

Is there a best practice approach here as to whether models should call other models, or not?

Thanks,

I routinely call models from within models, particularly when aggregating data from multiple sources.

This keeps the controllers nice and concise.

No, it's not bad practice.

Simpler is always better

OK thanks for the clarification!

And that is why you use a MY_Controller to handle the stuff that every Controller needs.

What did you Try? What did you Get? What did you Expect?

Joined CodeIgniter Community 2009.  ( Skype: insitfx )

And that is why you use a MY_Controller to handle the stuff that every Controller needs.

I understand that, however in many cases two models are tied together (i.e. related) but don't need to be made available to every single controller.

Here's my logic, not necessarily better or worse, it just does different trade offs.

For handling business logic, lets say if successful payment is made, it should trigger internal notification, update DB records etc, from controller I'd call single model method that sets all the internal things up, lets say emails to admin about new order etc, and that method could load any models.

The benefit would be that you can easily move this logic to different or multiple controllers, and you could also easily run unit tests that have nothing to do with any real controllers at all.

When loading models, some time ago my approach was, load everything so it's available, however now I am definitely in favour of only loading data from DB that I actually need - in your example, I'd probably load user details in only if I need them.

Of course, if 99.9% of your product loads would use user details anyway, then sure, save yourself having to type it out in every controller, just load product and user details are already available.

When loading models, some time ago my approach was, load everything so it's available, however now I am definitely in favour of only loading data from DB that I actually need - in your example, I'd probably load user details in only if I need them.

Of course, if 99.9% of your product loads would use user details anyway, then sure, save yourself having to type it out in every controller, just load product and user details are already available.

Thanks for your suggestions.

One thing I have been doing is this kind of thing:

PHP Code:

$args = array('user' => true, 'images' => false);
// Get the product, along with user data, but don't bother with images
$product = $this->product_model->get_product($product_id, $args); 


This is still a model calling other models, but it allows me to pick and choose which models to sub-query and what data to get.  Like you, I feel uncomfortable fetching masses of sub-data if all you want are the basic fields.

another way to think about this - you aren't just getting a 'product' from a database. you are putting together a bunch of different elements in order to display the product on your web page. so another way to do it would be to have a model like Product_page -
and then that calls the different models to get the product - get the images - get the creator - etc .  

This is going to make it way more flexible down the line as you change what is needed. Also always wrap any database interaction in some kind of conditional statement  in case something messes up. This could be to define an alternative or to stop the operation

Code:

 // have an alternative if nothing comes back  
 if( ! $product['user'] = $this->user_model->get_user($product['created_by'])
 {
     $product['user'] = 'Anonymous' ;
 }  
 
// OR for example if the product does not come back from database -- then it should immediately return false to controller  
// because there is no point in the other searches
if( !  $product  = $this->product_model->get_product($id ) )
{
  return false ;
}
else{ ..... }      

PHP Code:

$args = array('user' => true, 'images' => false);
// Get the product, along with user data, but don't bother with images
$product = $this->product_model->get_product($product_id, $args); 


This is still a model calling other models, but it allows me to pick and choose which models to sub-query and what data to get.  Like you, I feel uncomfortable fetching masses of sub-data if all you want are the basic fields.

Yes, very good solution IMHO - because on controller level you will know what data needs to be fetched from DB.

It's easy to get a bit lazy and go in guns blazing and fetch everything, which should be fine for single product, but if you are displaying lets say index page for 100 products, 100x all the possible data that you won't display on index page could add up very quick.

Only time nested models could become an issue is if same data is needed for different models and libraries to do their work.

Lets say, order was completed, now you need update stock for bought products, then email customer with details on same products.

For argument sake, the email function has it's own wrapper library, so it feels natural to just set it up so it takes customer ID and fetches the details to fill in "Hi XXX" or get the email address.

It would be impractical to get customer details to update their details, then get customer details again to send email, so at some point you might have to look at injecting data rather than IDs.

Maybe something like this:

PHP Code:

// email builder library
public function send($customer)
{
    if (is_numeric($customer)) {
        $this->load->model('customer_model');
        $customer = $this->customer_model->get($customer);
    }
}