Let’s take a controller ridden with problems and see how we can improve it. Hopefully this little experiment can help you beautify your code and optimize your app…
This is our ugly Users controller:
var $name = 'Users';
var $uses = array('User', 'Profile');
function add() {
if(!empty($_POST)) {
$this->User->create();
$this->User->set($this->data);
if($this->User->validates()) {
//create the process_type hash
$this->data['User']['process_type_hash'] = md5($this->data['User']['process_type']);
//prepare user code messages
switch($this->data['User']['error_code']) {
case('00'):
$this->data['User']['error_message'] = 'Success';
break;
case('01'):
$this->data['User']['error_message'] = 'User suspended';
break;
case('02'):
$this->data['User']['error_message'] = 'User account cancelled';
break;
case('03'):
$this->data['User']['error_message'] = 'User not verified';
break;
}
$this->User->save($this->data);
$userId = $this->User->getLastInsertId();
$this->Profile->create();
if($this->Profile->validates()) {
$this->data['Profile']['user_id'] = $userId;
$this->data['Profile']['temp_id'] = $this->generateTempId();
$this->Profile->save($this->data);
}
}
}
}
function view($id=null) {
$this->set('users', $this->User->findAll('id='.$id));
$this->set('profiles', $this->Profile->findAll('user_id ='.$id));
}
}
Let’s analyze the problems line by line and see what we can do it make it better…
Line 4
The $uses array is not needed (as in the vast majority of cases). Our models are related, therefore both User and Profile are already available to your controller
Line 7
When checking for POST’ed data, it’s best to rely on $this->data rather than $_POST
Line 8
No need to create() a model in this case, save() will handle that for you later on.
Line 9
This line really depends on the validates() method. Generally speaking there is no need to call this method separately, since it is called by save() for you, therefore setting the model’s data is no longer necessary.
Line 11
Based on the point above, we will not call validates() method, but will rely on save() instead.
Well, what about the data we need to prepare before saving?…
Lines 12 – 33
This work should never be done in the controller, and definitely not in this manner. Considering our points above, whenever we have any data, which has to be processed/prepared prior to save(), we should build a beforeSave() method in a given model to accomplish that.
Line 35
Calling save() without a false param, (as in save($data, false)) will trigger validation again. Either we get rid of the validates() method and rely on save() to handle validation for us, or we accept that data is already validated and we use ‘false’. Regardless of the approach, certainly there is no need to validate the same data twice.
Line 36
There is no need to call getLastInsertId(); After saving model’s id is automatically set to the last inserted record’s id. So in our case $this->User->id is already set.
Lines 38 – 45
Related models can be easily saved in one shot with saveAll(), there is simply no need of all this extra code.
Line 42
Looks like we’ve made some utility function in App Controller to generate our temp id. There are a few problems with this approach… First of all, the function is not protected, so we could access it by a URL, which is probably not desired. An easy way to avoid this would be to rename the function to function _generateTempId(), then you’d call it like $this->_generateTempId();. Still, this type of processing is better done in the model’s beforeSave() method.
Lines 51-52
We’ve got a few problems with our view() action. First of all our models are related, so we can easily retrieve the information for both by using $this->User->find(‘all’); Secondly, findAll() is deprecated so it’s best to use find(‘all’) instead. Third, the conditions, while working, are written poorly. A better syntax would be: find(‘all’, array(‘conditions’=>array(‘User.id’=>$id)));
And last, but not least, we are not checking for the value of $id prior to find(). If $id is indeed ‘null’, we should not call find() at all (a simple if() statement should handle it for us).
So what would a fixed-up controller look like?
var $name = 'Users';
function add() {
if(!empty($this->data)) {
$this->User->saveAll($this->data, array('validate'=>'first'));
}
}
function view($id=null) {
if(!empty($id)) {
$this->set('users', $this->User->find('all', array('conditions'=>array('User.id'=>$id))));
}
}
}
That’s all folks. A lot cleaner and prettier.
Well, what happened with all the code?
saveAll() will handle validation and saving of our User and Profile models, so all that code above can be easily condensed into a single line of code.
The data preparation i.e. the hash and error message creation will be moved into the beforeSave() of the User model.
And the preparation of temporary id will be moved to beforeSave() of the Profile model.
One note here, is that placing such a switch statement directly into your beforeSave() method is ugly. Instead, make a little utility function that you could use to process your data, for example:
$this->data = $this->_processErrorMsgs($this->data); … and move your switch code block into that utility function, it will make your code more flexible in the long run.
The main point here is that if you feel/see that your controllers are getting a little fat and your actions seems to have all sorts of ‘if’ statements, consider taking a step back and evaluating whether some of the functionality should be offloaded to models, utility methods or plain old simplified by using cake’s built-in tools.
