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);
}
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';
What version of PHP are you using? PHP 5 has support for real OO, so you can declare private function that will be treated as such by the interpreter:
private function foo(){
...
}
If you want classes that extend your class (child classes) to have access to the function, replace private with protected.
I've never used CodeIgnniter so I'm afraid I can't help you with your specific problem domain. However, refactoring a function that's grown to long is a very common problem, with common solutions. Martin Fowler is a smart guy who's written some books on the topic that are well-regarded, so you might see if you can find one of his books. There are also tutorials online to get you started refactoring.
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
A Service Layer would help.
If you want to keep your logic in the same controller, you can simulate a private method by placing an underscore before the function name, eg: _myMethod(). Like the link says, an underscore before the function name, prevents CI from calling it from the URL. You could, for instance, create _delete(), _preview(), _order() etc. methods in the Categories controller. If, however, you use the same logic to delete, preview, order etc. other stuff, maybe you should move these methods in a model or helper.
Personally, I think you're doing too much with a single controller method. One of the first things I would do is separate your CRUD (Create Read Update Delete) functions as separate methods. For example, your example is for working with "categories", why not have a separate "categories" controller?
class Categories extends Controller
{
function __construct()
{
parent::Controller();
}
function index()
{
//display logic/code here
}
function edit()
{
//get the category to update from the post or url for editing
//do the editing, etc
}
function delete()
{
//delete the category
}
function add()
{
//create the new category
}
}
Your URLs would reference the categories controller:
http://www.example.com/categories/edit http://www.example.com/categories/delete etc.
The second thing I'd suggest is upgrading to CodeIgniter 1.7.1 - the updated form_validation library makes it easy to move all of your validation rules to a separate config file.
Try looking into the _remap() function of the controller in codeigniter.
Using it you can keep the common code in the _remap function, and then call any other function from within _remap for delete, update etc (based on the uri_segment(3)).
You could put common functions in a library and call it.