Codeigniter Login Controller

Question

Instead of creating an Admin_Controller or MY_Controller I was going to try and just try out all my controllers on requirements that are needed per controller. I know this may seem like additional work but then after which I want to go back and fix it all. I wanted to know how my login looks as of right now?

I'm getting shocked that I've had 56 view and 3 upvotes and not one comment on the code at least.

<?php
if (!defined('BASEPATH'))
    exit('No direct script access allowed');
class Login extends Admin_Controller
{
    public function __construct()
    {
        parent::__construct();
        $this -> load -> model('user_login_model', 'login');
        $user_id = $this->session->userdata('user_id');
        if ($user_id == TRUE)
        {
            if (is_numeric($user_id) && strlen($user_id) < 5)
            {
                $this->session->unset_userdata('user_id');
                $this->session->sess_destroy();
                current_url();
            }
            else
            {
                redirect('dashboard', 'refresh');              
            }  
        }
    }

    public function index()
    {
        $this -> template -> set_theme('saturn') -> set_layout(FALSE) -> build('admin/login');
    }

    public function process()
    {
        $output_array = $this -> general_functions -> get_default_output();
        $this -> form_validation -> set_rules('email_address', 'Email Address', 'trim|required|xss_clean|valid_email');
        $this -> form_validation -> set_rules('password', 'Password', 'trim|required|xss_clean|min_length[6]|max_length[12]|regex_match[/[a-z0-9]/]');
        $this -> form_validation -> set_rules('remember_me', 'Remember Me', 'trim|xss_clean|integer');
        if ($this -> form_validation -> run() == FALSE)
        {
            $output_array['data_array']['title'] = 'Form Validation Failed';
            $output_array['data_array']['message'] = 'The form failed to validate. Please fix the errors and try again.';
        }
        else
        {
            $user_login_data = $this -> login -> get_by('email_address', $this -> input -> post('email_address'));
            if (is_array($user_login_data) && count($user_login_data) <= 0)
            {
                $output_array['data_array']['title'] = 'User Not Found';
                $output_array['data_array']['message'] = 'The user was not found in the database.';
            }
            else
            {
                if ($user_login_data -> user_status_id == 0)
                {
                    $output_array['data_array']['title'] = 'User Not Verified';
                    $output_array['data_array']['message'] = 'The user has registered but has not validated their account.';
                }
                elseif ($user_login_data -> user_status_id == 900)
                {
                    $output_array['data_array']['title'] = 'User Suspended';
                    $output_array['data_array']['message'] = 'The user account is current suspended.';
                }
                elseif ($user_login_data -> user_status_id == 901)
                {
                    $output_array['data_array']['title'] = 'User Banned';
                    $output_array['data_array']['message'] = 'The user account is banned.';
                }
                elseif ($user_login_data -> user_status_id == 909)
                {
                    $output_array['data_array']['title'] = 'User Pending Deletion';
                    $output_array['data_array']['message'] = 'The user account is pending deletion.';
                }
                elseif ($user_login_data -> user_status_id == 999)
                {
                    $output_array['data_array']['title'] = 'User Deleted';
                    $output_array['data_array']['message'] = 'The user account was deleted.';
                }
                else
                {
                    if (strtotime($user_login_data -> lock_date) > 0)
                    {  
                        if (strtotime(gmdate('Y-m-d H:i:s', time())) < strtotime($user_login_data -> lock_date))
                        {
                            $output_array['data_array']['title'] = 'User Locked';
                            $output_array['data_array']['nessage'] = 'The user is locked out of their account.';
                        }
                        else
                        {
                            $this -> login -> update($user_login_data -> user_id, array('lock_date' => '0000-00-00 00:00:00'));
                            $this -> process();
                        }
                    }
                    else
                    {
                        $regenerated_post_password = $this -> general_functions -> regenerate_password_hash($this -> input -> post('password'), $user_login_data -> password_hash);
                        $failed_logins = $this -> session -> userdata('failed_logins');
                        if ($failed_logins == FALSE)
                        {
                            $this -> session -> set_userdata('failed_logins', 0);
                            $failed_logins = 0;
                        }
                        if ($regenerated_post_password !== $user_login_data -> password)
                        {
                            if ($failed_logins >= 0 && $failed_logins < 5)
                            {
                                $failed_logins++;
                                $this -> session -> set_userdata('failed_logins', $failed_logins);
                                $output_array['data_array']['title'] = 'Inccorect Login Credentials';
                                $output_array['data_array']['message'] = 'You supplied the wrong username and password combination.';
                            }
                            else
                            {
                                $this -> login -> update($user_login_data -> user_id, array('lock_date' => gmdate('Y-m-d H:i:s', time() + (60 * 15))));
                                $output_array['data_array']['title'] = 'User Locked';
                                $output_array['data_array']['message'] = 'The user is locked out of their account. Your account is currently locked, we apologize for the inconvienence. You must wait 15 minutes before you can log in again! An email was sent to the owner of this account. Forgotten your username or password? <a href="forgotusername">Forgot Username</a> or <a href="forgotpassword">Forgot Password</a>';
                            }
                        }
                        else
                        {
                            $this -> session -> unset_userdata('failed_logins');
                            $this -> session -> set_userdata('user_id', $user_login_data -> user_id);
                            $output_array['status'] = 'success';
                            $output_array['data_array']['title'] = 'Login Validated';
                            $output_array['data_array']['message'] = 'You have successfully logged in and will be redirected to the dashboard.';
                        }
                    }
                }
            }
        }
        $this -> general_functions -> publish_output($output_array['status'], $output_array['data_array'], NULL, NULL, 'json', TRUE);
    }
}

Any thoughts? I'm looking for a few suggestions.

Answer 1

1. Formatting is not consistent:

   $this -> load -> model('user_login_model', 'login');
   $user_id = $this->session->userdata('user_id');
   

   Sometimes there are spaces around ->, sometimes aren't.

2. You could use a guard clause to make the code flatten:

   $user_id = $this->session->userdata('user_id');
   if ($user_id != TRUE)
   {
       return;
   }
   if (is_numeric($user_id) && strlen($user_id) < 5)
   {
       $this->session->unset_userdata('user_id');
       $this->session->sess_destroy();
       current_url();
   }
   else
   {
       redirect('dashboard', 'refresh');              
   }  
   

   It would help a lot to make it easier to read the process function. (You should extract a few functions there too.)

3. current_url();
   

   It's not clear what does this function do. Does it get the current url? Log it? Modify it? Its name should contain an action, like doSomething.

4. Instead of magic numbers like 0, 900, 901 etc. you should use named constants which describe their purpose. It would be easier to read.

5. $this -> general_functions -> publish_output($output_array['status'], $output_array['data_array'], NULL, NULL, 'json', TRUE);
   

   I would create local variables for the two NULL values and the TRUE value too to explain their intent. It would make the code readable and don't force readers to check the parameters of the publish_output function.

   (Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)