How do you refactor a Codeigniter controller function that is too long?


Question

I have a function in my controller that has grown longer than I'd prefer and I'd like to refactor it to call a few discrete functions to make it easier to manage. How can I better organize a long function in a Codeigniter controller?

What I've tried:

I know you can create private functions in a controller by naming them with a leading underscore (_myfunc), but then the variables in the function are out of scope for the calling controller function. So you have to return all the needed data from the function which is a hassle.

Is this the best option for managing a complex controller function? Is there an easier way where the variables could all be global to the controller class like a standard class member variable?

Suggestions? Thanks in advance!

EDIT: Someone requested the code so I added code for giant controller below. One opportunity for improvement is to move logic in switch statements to separate functions (delete, preview, order, etc). But I'm trying to decide on the next step after that. Moving the big validation setup code into it's own function would really take some weight out, but where should I move it to?

    function categories() {
    $this->load->library('upload');
    $this->load->model('categories_m');
    $this->load->model('products_m');
    $this->load->model('pages_m');
    $this->load->model('backoffice/backofficecategories_m');
    $data['body'] = $this->load->view('backoffice/categories/navigation_v', '', TRUE);
    $data['cat_tree'] = $this->categories_m->getCategoryTree();
    $data['page_list'] = $this->pages_m->getPageList();
    $data['category_dropdown'] = $this->load->view('backoffice/categories/category_dropdown_v',$data,TRUE);

    switch ($this->uri->segment(3)) { //display views based on parameter in URL.
    case 'delete':          
        $categoryTreeID = $this->sitewide_m->checkURLParam($this->uri->segment(4),'CategoryTree'); //if parameter is in URL, show 404 if invalid parameter is passed. Otherwise, set variable known to be safe.
        if (isset($_POST['delete'])) {
            $this->backofficecategories_m->deleteCategory($categoryTreeID);
            $data['body'] .= '<span class="error">Category Deleted.</span>';
        } else {
            $data['cat_details'] = $this->categories_m->getCategoryDetails('',$categoryTreeID);
            $data['parent_category'] = $this->categories_m->getParentCategory($categoryTreeID);
            $data['products_to_reassign'] = $this->products_m->getProductsInCategory('',$categoryTreeID);   
            $data['body'] .= $this->load->view('backoffice/categories/delete_v',$data,TRUE);  //pull fresh category tree data since tree was just updated.
        }
        break;
    case 'preview':
        if ($this->uri->segment(4)) $data['categoryTreeID'] = $this->sitewide_m->checkURLParam($this->uri->segment(4),'CategoryTree'); //if parameter is in URL, show 404 if invalid parameter is passed. Otherwise, set variable known to be safe.
        $data['cat_details'] = $this->categories_m->getCategoryDetails(NULL,$data['categoryTreeID']); //get category ID being edited from the URL and store it. Returns false if category ID isn't found.
        foreach ($data['cat_details']->result() as $detail) {
            $data['categoryName'] = $detail->Name;
            $data['categoryID'] = $detail->ID;
        }
        $data['body'] .= $this->load->view('backoffice/categories/preview_v', $data, TRUE);
        break;

    ...cases continue...
    default:
        $this->load->library('table');
        $data['body'] .= $this->load->view('backoffice/categories/categories_v', $data, TRUE);
        break;
    }
    $this->load->view('backoffice/template_v',$data);       
}
1
2
8/3/2013 12:26:17 PM

Accepted Answer

Looking at your code, you are using one method for several actions. I would make each action its own method. Common resources could be class members and loaded in the constructor.

So instead of a url like "your_controller/categories/add" you could change your url to "category_controller/add" and have a method for each action. If you don't want to change your urls, then use a route:

$route['your_controller/categories/(.*)'] = 'your_controller/$1';
4
2/11/2009 8:11:35 AM

Are you using models? Code igniter doesn't enforce this, but using models in addition to controllers and views is a good way to have a shorter controller function. Alternatively, you could place some of the functions in your own helper, then import it.

And if you want to set some default values for the entire constructor, you can use the class constructor. This is outlined here:

http://codeigniter.com/user_guide/general/controllers.html#constructors


Licensed under: CC-BY-SA with attribution
Not affiliated with: Stack Overflow
Icon