zen of coding

Controller makeover (from ugly to beautiful)

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:

class UsersController extends AppController {

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?

class UsersController extends AppController {

    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.

  • I like the idea of using the Model’s beforeSave to prepare any remaining data prior to the actual save, something I’ll need to look in to!

    I do have a question though: what is the point of $this->Model->create()? As the bake script creates it, I assumed it was necessary (and one of the recent books just published for CakePHP also said as much).

  • teknoid

    @Brendon Kozlowski

    create() is needed to “reset” the model, maybe to ensure that no primary key is set and a new record will be created. Seems like if you know what’s going on with your model/data then calling create() is only necessary when you are trying to do multiple saves in a row.

  • Phally

    this:
    $this->set(‘users’, $this->User->find(‘all’, array(‘conditions’=>array(‘User.id’=>$id))));

    can also be done like:
    $this->set(‘users’, $this->User->findAllById($id));

  • deizel

    /me wonders why the method is called create() when you only really need to use it to reset :)

  • teknoid

    @Phally

    Call me crazy, but I’m not a big fan of the findBy methods :)
    That being said, you’re right… it’s certainly is shorter and quicker to write findAllById();

  • teknoid

    @deizel

    Technically the “reset” functionality of create() can be considered a side-effect in a way. At the very basic level, create() actually prepares the model object for… well… creating a new record.
    So, $this->SomeModel->create($myDataToSave); $this->SomeModel->save(); makes it more clear as to why create() is not named reset().

    But we’re lucky that cake is sweet enough to call create() for us when doing a save().

  • Phally

    @teknoid
    You are crazy! I love the findBy methods, but only if i have only one condition.

  • teknoid

    @Phally

    LOL, fair enough :)

  • I have a mode called Phone which is related to User table.

    I have a Ajax way of adding n number of phone in the form.

    when i use saveAll() incase the phone is empty or by mistake the user has added a extra phone field its adds a blank field to the database but with an ID and User_id but a blank phone.

    Note: Adding a phone is not compulsary.

    So now how to check if the phone data has been entered or not and then save it. In case only one phone number is added only that should be saved.

  • I have a mode called Phone which is related to User table.

    I have a Ajax way of adding n number of phone in the form.

    when i use saveAll() incase the phone is empty or by mistake the user has added a extra phone field its adds a blank field to the database but with an ID and User_id but a blank phone.

    Note: Adding a phone is not compulsary.

    So now how to check if the phone data has been entered or not and then save it. In case only one phone number is added only that should be saved.

  • I have a mode called Phone which is related to User table.

    I have a Ajax way of adding n number of phone in the form.

    when i use saveAll() incase the phone is empty or by mistake the user has added a extra phone field its adds a blank field to the database but with an ID and User_id but a blank phone.

    Note: Adding a phone is not compulsary.

    So now how to check if the phone data has been entered or not and then save it. In case only one phone number is added only that should be saved.

  • I have a mode called Phone which is related to User table.

    I have a Ajax way of adding n number of phone in the form.

    when i use saveAll() incase the phone is empty or by mistake the user has added a extra phone field its adds a blank field to the database but with an ID and User_id but a blank phone.

    Note: Adding a phone is not compulsary.

    So now how to check if the phone data has been entered or not and then save it. In case only one phone number is added only that should be saved.

  • @Harsha M V

    Sounds like you’d need a custom validation method in the model. I don’t have a solution off top of my head.

%d bloggers like this: