Single Responsibility Principal

[Xtremer360]

This is what I am currently using for my submit function but it is doing way to much and I'm wanting to work with it so that I can apply the Single Responsibility Principle as well as I can see my code is suffering from Arrow Anti-Pattern and was looking for some ideas on how this could be improved.

 

 

 

/**
* submit function.
*
* @access public
* @param string $post_username
* @param string $post_password
* @return json data string
*/
public function submit($post_username = NULL, $post_password = NULL)
{
   // Set variable defaults
   $output_status = 'Notice';
   $output_title = 'Not Processed';
   $output_message = 'The request was unprocessed!';
   // Set validation rules for post data
   $this->form_validation->set_rules('username', 'Username', 'trim|required|xss_clean|min_length[6]|max_length[12]|regex_match[/[a-z0-9]/]');
   $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', 'Remember Me', 'trim|xss_clean|integer');
   if ($this->form_validation->run() == TRUE)
   {
    // Form validation passed
    // Number of error flags
    $x = 0;
    // Post values from login form
    $post_username = $this->input->post('username');
    $post_password = $this->input->post('password');
    // Get user data from post username value
    $user_data = $this->users_model->get_users(array('usernames' => $post_username), 'row');
    //print_r($user_data);
    //die();
    if ($user_data !== NULL)
    {
	    // User was found in database
	    if ($user_data->lock_date !== '0000-00-00 00:00:00')
	    {
		    // User is locked out
		    // Get the current GMT time
		    $current_time = now();
		    if (strtotime($user_data->lock_date) > $current_time)
		    {
			    // User is still locked out
			    $output_status = 'Error';
			    $output_title = 'Account Locked';
			    $output_message = 'This user account is current locked!';
			    $x++;
		    }
		    else
		    {
			    // User can be unlocked and form be resubmitted
			    $this->users_model->unlock_user($user_data->user_id);
			    $this->submit($post_username, $post_password);
			    return FALSE;
		    }
	    }
	    if ($x == 0)
	    {
		    // No error flags reported
		    // User is not locked out
		    if ($user_data->user_status_id == 1)
		    {
			    $output_status = 'Error';
			    $output_title = 'Account Unverified';
			    $output_message = 'Sorry you must verify your account before logging in!';
			    $x++;
		    }
		    if ($user_data->user_status_id == 3)
		    {
			    $output_status = 'Error';
			    $output_title = 'Account Suspended';
			    $output_message = 'Your account has been suspended!';
			    $x++;
		    }
		    if ($user_data->user_status_id == 4)
		    {
			    $output_status = 'Error';
			    $output_title = 'Account Banned';
			    $output_message = 'Your account has been banned!';
			    $x++;
		    }
		    if ($user_data->user_status_id == 5)
		    {
			    $output_status = 'Error';
			    $output_title = 'Account Deleted';
			    $output_message = 'Your account has been deleted!';
			    $x++;
		    }
		    if ($x == 0)
		    {
			    // No error flags reported
			    // User is registered and verified
			    $regenerated_post_password = $this->functions_model->regenerate_password_hash($post_password, $user_data->password_hash);
			    /* Not sure if this is needed
			    if ($this->session->userdata('failed_logins'))
			    {
				    // User has previous failed logins in session
				    $failed_logins = $this->session->userdata('failed_logins');
			    }
			    */
			    if ($regenerated_post_password == $user_data->password)
			    {
				    // Password from login form matches user stored password
				    // Set session variable with user id and clear previous failed login attempts
				    $this->session->set_userdata('xtr', $user_data->user_id);
				    $this->session->unset_userdata('failed_logins');
				    $output_status = 'Success';
				    $output_title = 'Login Success';
				    $output_message = 'Successful login! Sending you to the dashboard';
			    }
			    else
			    {
				    // Password from login from does not match user stored password
				    if (is_integer($failed_logins))
				    {
					    // User has atleast one failed login attempt for the current session
					    if ($failed_logins == 4)
					    {
						    $wait_time = 60 * 15;
						    $lock_out_time = $current_time + $wait_time;
						    /* Find out about if I can do this part differently.
						    $lock_out_date = gmdate('Y-m-d H:i:s', $lock_out_time);
						    */
						    $this->users_model->lock_out_user($user_data->user_id, $lock_out_date);
						    //$this->functions_model->send_email('maximum_failed_login_attempts_exceeded', $user_data->email_address, $user_data)
						    $output_status = 'Error';
						    $output_title = 'Account Locked';
						    $output_message = '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
					    {
						    // User has a few more chances to get password right
						    $failed_logins++;
						    $this->session->set_userdata('failed_logins', $failed_logins);
						    $output_status = 'Error';
						    $output_title = 'Incorrect Login Credentials';
						    $output_message = 'Incorrect username and password credentials!';
					    }
				    }
				    else
				    {
					    // First time user has not entered username and password correctly
					    $this->session->set_userdata('failed_logins', 1);
					    $output_status = 'Error';
					    $output_title = 'Incorrect Login Credentials';
					    $output_message = 'Incorrect username and password credentials!';
				    }
				    $time_of_attempt = gmdate('Y-m-d H:i:s');
				    $this->users_model->increase_login_attempt($this->input->ip_address(), $post_username, $time_of_attempt);
			    }
		    } // if ($x = 0) User is registered and verified
	    } // if ($x = 0) User is not locked out
    }
    else
    {
	    // User was not found in database
	    $output_status = 'Error';
	    $output_title = 'User Not Found';
	    $output_message = 'The user was not found in the database!';
    }
   }
   else
   {
    // Form validation failed
    $output_status = 'Error';
    $output_title = 'Form Not Validated';
    $output_message = 'The form did not validate successfully!';
   }
   echo json_encode(array('output_status' => $output_status, 'output_title' => $output_title, 'output_message' => $output_message, 'error_messages' => $this->form_validation->error_array()));
}

[Xtremer360]

Any ideas?

[KevinM1]

Can you combine conditionals? Are any candidates to be turned into guard clauses? Can you refactor certain parts (like your user lockout/ban check) into a separate private method?

[Xtremer360]

Here's what I have so far...

 

public function form_is_valid()
{
$this->form_validation->set_rules('username', 'Username', 'trim|required|xss_clean|min_length[6]|max_length[12]|regex_match[/[a-z0-9]/]');
$this->form_validation->set_rules('password', 'Password', 'trim|required|xss_clean|min_length[6]|max_length[12]|regex_match[/[a-z0-9]/]');
return $this->form_validation->run();
}

public function submit()
{
   $output_status = 'Notice';
   $output_title = 'Not Processed';
   $output_message = 'The request was unprocessed!';

 if ($this->form_is_valid())
 {
 $output_status = 'Success';
 $output_title = 'Form Submitted Successfully';
 $output_message = 'The form did validate successfully!';
 }
 else
 {
 $output_status = 'Error';
 $output_title = 'Form Not Validated';
 $output_message = 'The form did not validate successfully!';
 }

echo json_encode(array('output_status' => $output_status, 'output_title' => $output_title, 'output_message' => $output_message, 'error_messages' => $this->form_validation->get_error_array()));
}

Edited January 22, 2013 by Xtremer360