Codeigniter calling conditional data retrieval model method from controller

Question

I am wondering if there's a better way to structure my code. I want to keep my controllers thin but at the same time want to keep the model methods flexible. As you can see below, it's almost as if I can write active record code directly on my controller itself (Which not be a good thing?)

The line of code in the controller:

$request_forms = $this->request_form_model->where('status', Request_form_model::STATUS_ON)->find_all(NULL, NULL, NULL, ['title', 'id', 'service_category_id'])->result();

The relevant code in MY_Model which the Request_form_model extends from:

function where($field, $value){
    $this->db->where($field, $value);
    return $this;
}

function find_all($limit = NULL, $offset = NULL, $order_by = NULL, $select_fields = '*', $escape_select = NULL){

    if( is_array( $select_fields ) ) {
        $select_fields = implode(',', $select_fields);
    }

    $this->db->select($select_fields, $escape_select);

    if(isset($order_by)) {
        $this->order_by($order_by);
    }

    $this->limit($limit, $offset);

    return $this->db->get_where($this->table_name);
}

Answer 1

Readability and Usability

Your function is indeed quite generic, which does give you a lot of power in your controller. But I would agree with you, that this is not necessarily a good thing.

Just looking at your controller line, I would have no idea what it does, because it's impossible to know what all the NULL values stand for. Apart from it being hard to read, I would also think that this function is hard to use. I would have to look up the order of the parameters each time when I write a query.

Need for Flexibility

The first thing I would check is if your models actually need to be this flexible. Because that way, you do blow up your controller a bit, as it has to make the decision of what to actually extract.

In your example, you don't actually need the limit, offset, or order_by, so if it's not needed elsewhere (and probably also not needed in the future), think about removing it.

I'll just assume that you do indeed need the flexibility, but it's something to think about.

Two Possible Alternatives

I could think of two different ways to improve your code, one offering reduced flexibility, the other keeping the existing flexibility and increasing readability.

Option 1

The first option is to write additional functions which access your find_all function internally:

function find_all_sorted($order_by) {
    // call find_all
}

function find_from_to($from, $to) {
    // call find_all
}

// [...]

You loose some flexibility (or you have to add quite a lot of methods to get the same flexibility), but your controller will be slimmer. If you can cover all needed use-cases with a couple of these methods, this might be the way to go.

Option 2

The second possibility is to expose the database methods to the outside as you did with the where method. You will have the same flexibility as before, but your code will be more readable.

If you don't want to expose the actual methods of the database (which would be cleaner, as the controller shouldn't actually care about the way the database is accessed), you could also just add setters to it:

function from_to($from, $to) {
    $this->from = $from;
    $this->to = $to;
}

function order_by($order_by) {
    $this->from = $from;
    $this->to = $to;
}

function select($fields) {
    $this->fields = $fields;
}

function query() {
    // call find_all with the set parameters, like you did before from the controller
}

In the controller, it would then look like this:

$this->request_form_model
    ->select(['title', 'id', 'service_category_id'])
    ->where('status', Request_form_model::STATUS_ON)
    ->from_to('1', '10')
    ->order_by('title')
    ->query();

Each of the method calls is optional, and it's clear for the reader what the query will return, without looking up the documentation for each method.