Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Trying to see if anyone sees any potential flaws with this submit function. One concern is this line:

/* 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');
}
*/

 /**
 * 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()));
}

EDIT 2: This is what I have so far. Any additional ideas?

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()));
}
share|improve this question
    
for starts Replace if ($this->form_validation->run() == TRUE) for if ($this->form_validation->run()) you don't need to compare if(true == true) –  Michel Ayres Dec 26 '12 at 20:59

2 Answers 2

The access parameter in your doccomments is unnecessary. Any application that is sophisticated enough to provide doccomment hints is also sophisticated enough to tell you the access method automatically. At least I have yet to find one that contradicts this.

Your function is doing entirely too much. The Single Responsibility Principle states that a function/method should be in charge of doing one thing and one thing only, and should not be concerned with all of the smaller tasks associated with doing it. Additionally, the name of that function should be indicative of what it is supposed to do. Believe it or not, you've already separated your code, at least partially, by this principle. You just haven't done so with methods (Hint: the next paragraph).

Internal comments only add clutter to your code. If your code is self-documenting and following standard practices, then internal comments become unnecessary. The only comments you should really need are doccomments. Let your code speak for itself. If something isn't clear enough, either make note of it in the doccomments, or clean up the code to make it clearer.

Your code is severely suffering from the Arrow Anti-Pattern. This pattern illustrates code that has been excessively indented to come to points like an arrow, or multiple arrows. Sometimes also known as being mountainous. The arrow shapes is one indication of such a violation, but may not always be visible. There are a few things you can do to avoid violating this pattern: you can reverse long if statements for shorter else statements, dropping the else and unindenting the contents; you can return early, which also helps with efficiency; and you can create defaults. The following code accomplishes all three:

$output_status = 'Error';
$output_title = 'Form Not Validated';
$output_message = 'The form did not validate successfully!';
$error_messages = array();

if( ! $this->form_validation->run() ) {
    return json_encode( compact(
        'output_status',
        'output_title',
        'output_message',
        'error_messages'
    ) );
} //else {//the else is now implied because we returned early

//the rest of the code can be unindented now

So you may notice a few other things. The first is something that Michel pointed out in his answer. There is no need to explicitly check for a boolean unless you are explicitly looking for one. PHP automatically converts 0, NULL, FALSE, -1, or any string equivalent or empty string to FALSE. Everything else is considered "loosely" == TRUE. So unless you are explicitly === looking for that value, there is no need to explicitly ask for it.

if( "TRUE" == TRUE ) {//TRUE and unnecessary
if( "TRUE" ) {//TRUE
if( "TRUE" === TRUE ) {//FALSE because of types

The next is that I removed the echo from your code. You should avoid this as it makes your code less reusable. Returning that value allows you to do more with it and gives you something to return early.

The final thing I changed was incorporating compact(). This function accepts variables, via their name as a string, and pushes them into an array with the given name as the key. You don't have to use this function, some people find it confusing because the IDE does not register that those variables are being used, but I find it useful.

What is the point of injecting the $post_username and $post_password if all you are going to do is just reset its value? Just a reminder, this method doesn't need to know where everything is coming from. That's the whole point of injecting the values.

Instead of using a counter to determine if any errors occurred, why not just save those errors to an array and then check the array?

$errors = array();

//etc...

$error = compact( 'output_status', 'output_title', 'output_message' );
$errors[] = $error;

//etc...

if( ! empty( $errors ) ) {

As Michel mentioned, you might consider using a switch statement if you are planning on comparing the same value against multiple conditions. Additionally, this is violating the "Don't Repeat Yourself" (DRY) Principle. As the name implies, your code should not repeat itself. In this case, each of these statements starts off by redefining the $output_status variable to "Error", which is redundant. Set it as that by default and then use the default switch statement to change it if necessary. At this point I have stopped keeping track of what the last value of these variables were, but that is something you should be able to use to your advantage.

$output_status = 'Error';

switch( $user_data->user_status_id ) {
    case 1 :
        $output_title = 'Account Unverified';
        $output_message = 'Sorry you must verify your account before logging in!';
    break;

    //etc...

    default :
        $output_status = etc...;
    break;
}

Alternatively you could also create an array with these comparisons as the key and the values as an array containing the necessary output information. This is probably the ideal method.

$status = $user_data->user_status_id;
if( isset( $error_messages[ $status ] ) ) {
    $error = $error_messages[ $status ];
    $output_title = $error[ 'title' ];
    $output_message = $error[ 'message' ];
}

I'm going to stop here. For the most part it looks like more of the same. I would focus on the principles I mentioned and try to apply them to your code. Those are the biggest issues that I can see.

share|improve this answer
    
+1 "My reviews tend to be more text than code.", True story. But good answer anyway. =D –  Michel Ayres Dec 27 '12 at 12:44
    
pastebin.com/2H3k6jR1 Update –  Kevin Smith Dec 28 '12 at 19:21
    
Any ideas on improvements? –  Kevin Smith Jan 1 '13 at 18:37
    
@KevinSmith: Sorry for the delay, I was away for new years. I didn't read the whole thing, but I did scan and compare it to the old. It looks like the only change you made was the switch statement. The biggest improvement you can make would be to downsize your function into multiple smaller functions, then there is the arrow anti-pattern, and the internal comments. Each of these will immediately show improvement. There is still a lot to be gleaned from the original post, so make sure to look over it again. –  mseancole Jan 2 '13 at 17:16
    
I'm going to try and post in a few minutes. –  Kevin Smith Jan 2 '13 at 19:16

Replace

if ($this->form_validation->run() == TRUE) for if ($this->form_validation->run())

You don't need to compare if(true == true)


In this part:

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++;
}

Replace IF for ELSE IF or SWITCH.


Extra (optional) your code if ($x == 0) has another if ($x == 0) inside itself. you don't need it if you just use a bool flag instead. Changing x is not good.


Read about Sessions in CodeIgniter, will help you understand this part.

if ($this->session->userdata('failed_logins'))
{
    // User has previous failed logins in session
    $failed_logins = $this->session->userdata('failed_logins');
}

The $this->session->userdata('failed_logins') verify if you have created this session. The code you posted doesn't countain this creation.

share|improve this answer
    
Can you explain the last part further because after reading the documentation I'm still confused on what would be a better use with this bit of code. –  Kevin Smith Dec 26 '12 at 21:50
    
Also about the part of the flag what do you suggest because I do error checking then if I'm still good after the first round of error flags then I need to do a second round of error checking. –  Kevin Smith Dec 26 '12 at 23:02
    
When you start $x = 0; you can also starts a bool variable, let's say... $NoError = true then in the second if ($x == 0) you can use if($NoError). Also change the $x++; for $NoError= false; –  Michel Ayres Dec 27 '12 at 12:38
    
@KevinSmith about sessions in php the link for codeigniter is in the answer already. the $this->session->userdata('failed_logins') is NEVER created in the code that you posted, so this if will NEVER be used. Just remove it. (Or find the part that you load the session) –  Michel Ayres Dec 27 '12 at 12:42

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.