Fat Model Skinny Controller in CodeIgniter

Question

I've been using CodeIgniter for a while, and have a decent knowledge of MVC, PHP etc.

However, I'm finding it hard to adhere to the Fat Model Skinny Controller ethos.

I've seen a lot about it; including what pseudo code to include in each file, but no actual examples. (Please link to some articles if I've missed any obvious ones!)

I'm finding it hard to move the form logic to a model. For instance, I am using a custom library for my auth system, which has it's own model. Should I then make a site user Model to log users in? Or should I just make a site Model to do that? Or a form Model?

To help me out, can anyone advise me on how to skinnify this Controller? I realise it's a lot of code, but simple pointers would be great. (Please note, I've only just written this code, so it hasn't been refactored much, but it should give a good example of how some of my methods are getting out of hand.)

public function register()
{
    session_start();
    if ($this->tf_login->logged_in())
    {
        redirect('profile');
    }
    if ($_GET['oauth'] == 'true')
    {
        $type = $_GET['type'];
        try 
        {
            $token = $this->tf_login->oauth($type, '', 'email');
        }
        catch (TFLoginCSRFMismatchException $e)
        {
            $this->tf_assets->add_data('error_message', $e->getMessage());
        }
        catch (TFLoginOAuthErrorException $e)
        {
            $this->tf_assets->add_data('error_message', $e->getMessage());
        }
        if ($token)
        {
            $user_details = $this->tf_login->call('https://graph.facebook.com/me?fields=email,first_name,last_name,username&access_token=' . $token);
            $user_details_decoded = json_decode($user_details);
            if ($user_details_decoded->email)
            {
                try 
                {
                    $id = $this->tf_login->create_user($user_details_decoded->username,
                    md5($user_details_decoded->username . time()),
                    $user_details_decoded->email,
                    '',
                    TRUE,
                    TRUE);
                }
                catch (TFLoginUserExistsException $e)
                {
                    try
                    {
                        if ($this->tf_login->oauth_login($type, $user_details_decoded->email, $token))
                        {
                            $this->session->set_flashdata('success_message', 'You have successfully logged in.');
                            redirect('profile');
                        }
                        else
                        {
                            $this->session->set_flashdata('error_message', 'An account with these details exists, but currently isn\'t synced with ' . $type . '. Please log in to sync the account.');
                        }
                    }
                    catch (Exception $e)
                    {
                        $this->session->set_flashdata('error_message', $e->getMessage());
                    }
                }
                catch (TFLoginUserNotCreated $e)
                {
                    $this->tf_assets->add_data('error_message', 'You could not be registered, please try again.');
                }
                if ($id)
                {
                    $this->tf_login->add_user_meta($id, 'first_name', $user_details_decoded->first_name);
                    $this->tf_login->add_user_meta($id, 'surname', $user_details_decoded->last_name);
                    $this->tf_login->sync_accounts($id, $type, $token);
                    $this->session->set_flashdata('success_message', 'Welcome ' . $this->input->post('first_name', TRUE) . ' ' . $this->input->post('surname', TRUE) . '. Your account has been sucessfully created. You will shortly receive an email with a verification link in.');
                    redirect('login');
                }
            }
            else
            {
                $this->session->set_flash_data('error_message', 'You could not be logged in, please try again.');
            }
        }
        // Redirect to clear URL
        redirect(current_url());
    }

    if ($this->form_validation->run() !== FALSE)
    {
        try
        {
            $id = $this->tf_login->create_user($_POST['username'], $_POST['password'], $_POST['email'], '', FALSE);
        }
        catch (Exception $e)
        {
            $this->tf_assets->add_data('error_message', $e->getMessage());
        }
        if ($id)
        {
            $this->tf_login->add_user_meta($id, 'first_name', $_POST['first_name']);
            $this->tf_login->add_user_meta($id, 'surname', $_POST['surname']);
            if ($this->tf_login->register_verification_email())
            {
                $this->session->set_flashdata('success_message', 'Welcome ' . $this->input->post('first_name', TRUE) . ' ' . $this->input->post('surname', TRUE) . '. Your account has been sucessfully created. You will shortly receive an email with a verification link in.');
                redirect('login');
            }
            else
            {
                $this->tf_login->login_user($id);
                $this->session->set_flashdata('success_message','Your account has been sucessfully created.');
                redirect('profile');
            }
        }
        else
        {
            $this->tf_assets->add_data('error_message', $this->tf_login->get_errors());
        }
    }
    if (validation_errors())
    {
        $this->tf_assets->add_data('error_message', validation_errors());
    }
    $this->tf_assets->set_content('public/register');
    $this->tf_assets->add_data('page_title', "Register");
    $this->tf_assets->render_layout();
}

Thanks in advance!

The "Fat Model" preaching applies to the original MVC pattern. [It's not what the PHP frameworks do](http://stackoverflow.com/questions/1549857/simple-php-mvc-framework/1549970#1549970). It doesn't do you much good with CI either. Treating the Controller as form processing interface there is not really a factual problem. — mario, Sep 25 '11 at 20:55
possible duplicate of [Understanding MVC: Whats the concept of "Fat" on models, "Skinny" on controllers?](http://stackoverflow.com/questions/3109715/understanding-mvc-whats-the-concept-of-fat-on-models-skinny-on-controllers) — Gordon, Sep 25 '11 at 21:26
When I searched for previous questions that didn't appear. I'll have a read through, cheers :) — Toddish, Sep 25 '11 at 21:31
Just do what is necessary and do not worry about someone elses idea of perfection. Remmeber, a controller(function), model(function) and view are all part of a page. Frameworks are supposed to speed things up not make you worry where every piece of code should or shouldnt be. — Brad, Sep 26 '11 at 21:45
Very true, I do often get bogged down with the best way to do things, instead of just getting them done! — Toddish, Sep 29 '11 at 10:42
possible duplicate of [MVC in PHP - fat model or fat controller?](http://stackoverflow.com/questions/4568553/mvc-in-php-fat-model-or-fat-controller) — PeeHaa, May 19 '12 at 00:46

score 3 · Accepted Answer · answered Sep 28 '11 at 16:15

From what I can tell, most or all of this code belongs in a controller or component, so I don't think your problem is Model/Controller confusion.

The code is difficult to read, however, because of the deep nested structures and the failure to break out specific tasks into their own methods. The main refactoring you would benefit from here is creating new private methods to separate out the discrete subtasks that you are performing. This is has the additional important benefit of clarifying the high-level structure of your current method. So you would end up with something that looked like (just to give you a rough example):

public function register()
{
    session_start();
    if ($this->tf_login->logged_in())
    {
        redirect('profile');
    }
    if ($_GET['oauth'] == 'true')
    {
        $this->oauthRegister();
    }

    $this->normalRegister();
}

Similarly, the oatuhRegister method and normalRegister methods would be broken down into smaller methods themselves, so that when you were completely finished each method would be adhering to the SRP and would probably be fewer than 10 lines of code. This will drastically improve the readability and maintainability of your code. I'd also recommend checking out Clean Code, which makes a strong argument for keeping your method short.

I had thought about splitting it up into separate private methods, but then I thought these could be handed off to a model. THEN I thought that I didn't really like a model handeling redirects, setting errors etc. I think I will split it up, concentrate on making it more readable. I'll mark this as the accepted answer soon, unless someone else posts something enlightening, because so far this makes the most sense. — Toddish, Sep 29 '11 at 10:49
You are DEFINITELY right in your instinct to keep redirects out of the model. One other thing that might help you, an answer I received somewhere else when asking if authentication should be done in the model or controller: http://pastebin.com/SKvpjrk0. So while you're right that generally code that changes the model goes in the model, it's typical that authentication goes in the controller, which makes sense bc it is, literally, an act of "controlling" access. — Jonah, Sep 29 '11 at 14:03

Fat Model Skinny Controller in CodeIgniter

1 Answers1