Guidance trying to make skinny controllers & fat m

Question

I'm new to Cake, and to MVC's in general. I want to establish good habits from the get go. Among the good habits are keeping controllers lean, and making the models fat. But it's a bit of a moving target for a noob like me. If I need to pass info from one model to another, do I just dump all that into the controller? Try to make it work in a model?

Here's an action that is a prime example of the kind of confusion I'm trying to sort out.

Everything seems like it should be in the controller, but I'm likely wrong. This action gets a list of members, sends that to a view. In the view I can tick members whose accounts I want "activate." No ACL, just simple auth. I make sure that "sub-admins" only see the users they're allowed to manage by use of a db field, client_id. The two models I'm using are User and Client.

public function activate() {
    if ($this->request->is('get')) {
        $id = $this->Auth->user('id');
        $this->User->id = $id; //make sure current User is the logged in user
        $currentClient = $this->User->field('client_id'); // get client_id based on logged in user
        $members = $this->User->Client->find('first', array( // find users that have the same client_id
            'conditions' => array('id' => $currentClient),
            'recursive' => 1
        ));
        $this->set('clients', $members); // send the users to the view                  
    } else if ($this->request->is('post') || $this->request->is('put')) {
        $members = $this->request->data['Members']; // grab players submitted from push form
        $memberIds = array(); // this will hold the selected users
        foreach($members as $a){
            $memberIds[$a['id']] = $a['id']; // loop over user's that were selected
        }
        $usersToActivate = $this->User->find('all', array( //find user records, based on the array of id's
            'conditions' => array(
                "User.id" => $memberIds
            )
        ));
        $this->Ticket->bulkActivate($usersToActivate); // send array of members into model for processing
        $this->Session->setFlash('Activations sent.', 'default', array('class' => 'success'));
        $this->redirect(array('action' => 'index'));
    }
}

To my eye, it doesn't look drastically wrong... and I'm already doing some processing in the model (as seen with the bulkActivate that actually takes the user records, and generates activation tickets).

But I can't help but feel that it's not quite 100% yet.

Answer 1

I do not think you want to get only ONE client?

$members = $this->User->Client->find('first', array

I guess this should find all. I've improved this to use pagination in the case there are lots of users. I might be wrong with this but neither your associations nor the real goal is clear to me by looking at this code. I don't know how for example the Ticket model is associated with any other data. But I guess you're using Controller::uses, you should not do that but access related models through their associations.

Do not use horrible variable names like $a, that just sucks and nobody will ever know what that means in a larger code block or application. You also named an array containing client data $members, why not $clients? Read this: Clean Code and for CakePHP I suggest you to follow the CakePHP coding standards.

Describe the goal and this could be refactored even better I think. If you want to activate clients to have access to a ticket (thats what it looks like) why have you not done it in the ticket or client controller/model?

Also this huge amount of inline comments is just causing more mess than it helps. Write clean and readable code and the code will speak for itself. You have not done any super complex code or super complex math there. Again, I can just recomment you to read "Clean Code", it is in my opinion a "must read" for every developer.

<?php
    // UsersController.php
    public function activate() {
        if ($this->request->is('post') || $this->request->is('put')) {
            $this->User->activate($this->request->data);
            $this->Session->setFlash('Activations sent.', 'default', array('class' => 'success'));
            $this->redirect(array('action' => 'index'));
        }

        this->Paginator->settings['Client'] = array(
            'conditions' => array('id' => $this->Auth->('current_id')),
            'contain' => array(
                'OnlyModelsYouNeedHere'));
        $this->set('clients', $this->Paginator->paginate($this->User->Client)); 
    }
?>

<?php
    // User.php - the model
    public function activate($data) {
        $memberIds = array();
        foreach($data['Members']members as $member) {
            $memberIds[$member['id']] = $member['id'];
        }
        $usersToActivate = $this->find('all', array(
            'conditions' => array(
                'User.id' => $memberIds)));
        return $this->Ticket->bulkActivate($usersToActivate);
    }
?>

Passing the ids could be also trimmed down more, but hey, its late here now. :) Take this as a rough refactor of your code and think about what I have changed and more important why.

If you want to see proper examples of skinny controllers and fat models check our plugins out, the users plugins UsersController and Model might give you a bigger picture.