Order of Logic

[Xtremer360]

I'm just trying to see if the order of my logic is correct or if there's something I can do differently.  I'm also curious to know at what point I should get the 2 passwords that exists in the database for the user. These are two different passwords.

 

function login_submit()
    {
        $this->form_validation->set_rules('username', 'Username', 'trim|required|xss_clean');
$this->form_validation->set_rules('password', 'Password', 'trim|required|xss_clean');
$this->form_validation->set_rules('remember', 'Remember me', 'integer');
        
        $user_id = $this->users->get_user_id_by_username($this->input->post('username'));
        
        if ($user_id !== 0)
        {
            if ($this->kow_auth->is_max_login_attempts_exceeded($user_id))
            {
                echo json_encode(array('error' => 'yes', 'message' => 'Your account is currently locked, we appologize for the inconvienence. You must wait 10 minutes before you can login again!'));
            }
            else
            {
                $user_status = $this->users->get_user_status($user_id);
                
                if ($user_status == 1)
                {
                    echo json_encode(array('error' => 'yes', 'message' => 'Sorry you must verify your account before logging in!'));                    
                }
                elseif ($user_status == 3)
                {
                    echo json_encode(array('error' => 'yes', 'message' => 'Your account has been suspended!'));          
                }
                elseif ($user_status == 4)
                {
                    echo json_encode(array('error' => 'yes', 'message' => 'Your account is currently banned!'));          
                } 
                elseif ($user_status == 5)
                {
                    echo json_encode(array('error' => 'yes', 'message' => 'Your account has been deleted!'));     
                }
                else
                {
                    if ($this->form_validation->run()) {
                        if ($this->kow_auth->login($this->form_validation->set_value('username'), $this->form_validation->set_value('password'), $this->form_validation->set_value('remember'))) 
                        {								
                            redirect('');
    				    }
                        else
                        {
                            echo json_encode(array('error' => 'yes', 'message' => 'Incorrect username and password combination!'));
                        }
                    }
                }                                             
            }               
        } 
        else 
        {
            echo json_encode(array('error' => 'yes', 'message' => 'Incorrect username and password combination!'));
        }            
        
    }

[Psycho]

Here are my suggestions/recommendations:

 

1. When doing validations, do a negative test so you can immediately follow with the error message. That way you have a logical flow of the validations/errors. Otherwise the validations and the associated errors are difficult to follow since they are separated at the top and bottom of the code.

 

2. For the user status check a switch() would be more appropriate

 

3. Functions, as a general rule, should not be echoing content. They should instead return a result to the instance it was called.

 

4. Add some comments! Be liberal with your comments so you know what your code is doing months from now when you have to make changes.

 

[EDIT]5. Also, the first few lines that call the method $this->form_validation->set_rules: I'm not sure what that does, but I have a suspicion that what is done there is only used if the method $this->kow_auth->login is called. If so, it is a waste to always call those three lines of code for the instances when any of the validations fail. Instead, move those three lines just before $this->kow_auth->login is called so they are only executed if all of the previous validations pass. I have updated the code below to do this.

 

Also, not sure what the function redirect() does. But, I would leave that out of the function. Instead the function can do a return true; if all validations pass or the error message if there is a failure. Then the place that calls the function can determine what to do.

 

You also have one un-handled scenario. You have this condition

if ($this->form_validation->run()) {

But there is no else condition. So if that condition does not pass there will be no error message and login will not succeed. I have added an error condition for this, but leave it to you to provide appropriate text

 

Below I have included how I would personally write that function and then an example of the usage:

 

function login_submit()
{
    $user_id = $this->users->get_user_id_by_username($this->input->post('username'));

    if ($user_id == 0)
    {
        $error = 'Incorrect username and password combination!';
    }
    elseif ($this->kow_auth->is_max_login_attempts_exceeded($user_id))
    {
        $error = 'Your account is currently locked, we appologize for the inconvienence. You must wait 10 minutes before you can login again!';
    }
    else
    {
        $user_status = $this->users->get_user_status($user_id);
        switch($user_status)
        {
            case 1:
                $error = 'Sorry you must verify your account before logging in!';
                break;
            case 3:
                $error = 'Your account has been suspended!';
                break;
             case 4:
                $error = 'Your account is currently banned!';
            break;

            case 5:
                $error = 'Your account has been deleted!';
                break;

           default:
                $this->form_validation->set_rules('username', 'Username', 'trim|required|xss_clean');
                $this->form_validation->set_rules('password', 'Password', 'trim|required|xss_clean');
                $this->form_validation->set_rules('remember', 'Remember me', 'integer');
                if (!$this->form_validation->run())
                {
                    $error = 'NEED TO ADD AN ERROR MESSAGE!';
                }
                else
                {
                    if (!$this->kow_auth->login($this->form_validation->set_value('username'),
                                                $this->form_validation->set_value('password'),
                                                $this->form_validation->set_value('remember'))) 
                    {                        
                        $error = 'Incorrect username and password combination!';
                    }
                    else
                    {
                        return true;
                    }
                }
                break;
        }
    }
    //There was a failure
    return json_encode(array('error' => 'yes', 'message' => $error));
}

 

Usage:

if(login_submit()===true)
{
    redirect('');
}
else
{
    echo $login_result;
}

[Xtremer360]

I moved a lot of stuff around and wanted to know how this part looks so far which your suggestions.

 

<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class Login extends CI_Controller { 

public function __construct()
{
        parent::__construct();
        $this->load->library('kow_auth');  
        $this->load->model('kow_auth/users');           
    }	

public function index()
{
	//Config Defaults Start
	$msgBoxMsgs = array();//msgType = dl, info, warn, note, msg
	$cssPageAddons = '';//If you have extra CSS for this view append it here
	$jsPageAddons = '<script src="http://www.kansasoutlawwrestling.com/kowmanager/assets/js/loginvalidate.js"></script>';//If you have extra JS for this view append it here
	$metaAddons = '';//Sometimes there is a need for additional Meta Data such in the case of Facebook addon's
	$siteTitle = '';//alter only if you need something other than the default for this view.
	//Config Defaults Start


	//examples of how to use the message box system (css not included).
	//$msgBoxMsgs[] = array('msgType' => 'dl', 'theMsg' => 'This is a Blank Message Box...');

	/**********************************************************Your Coding Logic Here, Start*/

        $bodyContent = "login_form";//which view file
	$bodyType = "full";//type of template

	/***********************************************************Your Coding Logic Here, End*/

	//Double checks if any default variables have been changed, Start.
	//If msgBoxMsgs array has anything in it, if so displays it in view, else does nothing.      
	if(count($msgBoxMsgs) !== 0)
	{
		$msgBoxes = $this->msgboxes->buildMsgBoxesOutput(array('display' => 'show', 'msgs' =>$msgBoxMsgs));
	}
	else
	{
		$msgBoxes = array('display' => 'none');
	}

	if($siteTitle == '')
	{
		$siteTitle = $this->metatags->SiteTitle(); //reads 
	}

	//Double checks if any default variables have been changed, End.

	$this->data['msgBoxes'] = $msgBoxes;
	$this->data['cssPageAddons'] = $cssPageAddons;//if there is any additional CSS to add from above Variable this will send it to the view.
	$this->data['jsPageAddons'] = $jsPageAddons;//if there is any addictional JS to add from the above variable this will send it to the view.
	$this->data['metaAddons'] = $metaAddons;//if there is any addictional meta data to add from the above variable this will send it to the view.
	$this->data['pageMetaTags'] = $this->metatags->MetaTags();//defaults can be changed via models/metatags.php
	$this->data['siteTitle'] = $siteTitle;//defaults can be changed via models/metatags.php
    $this->data['bodyType'] = $bodyType;
    $this->data['bodyContent'] = $bodyContent;
	$this->load->view('usermanagement/index', $this->data);
}
    
    function login_submit()
    {
        $this->form_validation->set_rules('username', 'Username', 'trim|required|xss_clean');
	$this->form_validation->set_rules('password', 'Password', 'trim|required|xss_clean');
	$this->form_validation->set_rules('remember', 'Remember me', 'integer');
        
        if (!$this->form_validation->run()) 
        {
            redirect('login');            
        }
        else
        {
            $user_id = $this->users->get_user_id_by_username($this->input->post('username'));
            
            if ($user_id == 0)
            {
                echo json_encode(array('error' => 'yes', 'message' => 'Incorrect username and password combination!'));
            }
            else
            {
                if ($this->kow_auth->is_max_login_attempts_exceeded($user_id))
                {
                    echo json_encode(array('error' => 'yes', 'message' => 'Your account is currently locked, we appologize for the inconvienence. You must wait 10 minutes before you can login again!'));
                }
                else
                {
                    $user_status = $this->users->get_user_status($user_id);
                    
                    switch($user_status)
                    {
                        case 1:
                            echo json_encode(array('error' => 'yes', 'message' => 'Sorry you must verify your account before logging in!'));
                            break;
                    
                        case 3:
                            echo json_encode(array('error' => 'yes', 'message' => 'Your account has been suspended!'));    
                            break;
                        case 4:
                            echo json_encode(array('error' => 'yes', 'message' => 'Your account is currently banned!'));              
                            break;
                        case 5:
                            echo json_encode(array('error' => 'yes', 'message' => 'Your account has been deleted!'));     
                            break;
                        default:
                            if ($this->kow_auth->login($this->form_validation->set_value('username'), $this->form_validation->set_value('password'), $this->form_validation->set_value('remember'))) 
                            {								
                                redirect('cpanel');
        				    }
                            else
                            {
                                echo json_encode(array('error' => 'yes', 'message' => 'Incorrect username and password combination!'));
                            }
                    }                           
                }               
            }  
        }          
    }
}

/* End of file login.php */ 
/* Location: ./application/controllers/login.php */ 

[Xtremer360]

I saw your edit and I wanted to point out that I'm using the codeigniter framework and their validation library. Its my understanding that you should put those 3 lines at the beginning because it setting the rules before ANY validation is done but maybe I'm just confusing myself.

 

http://codeigniter.com/user_guide/libraries/form_validation.html

[scootstah]

I saw your edit and I wanted to point out that I'm using the codeigniter framework and their validation library. Its my understanding that you should put those 3 lines at the beginning because it setting the rules before ANY validation is done but maybe I'm just confusing myself.

 

http://codeigniter.com/user_guide/libraries/form_validation.html

 

That's correct.

 

What he meant was that instead of saying

if (validation passed)
     bunch of code
else 
     error way down here

you can do

if (validation DIDNT pass)
     error up here 
else
     bunch of code

 

Which you seem to already be doing. And this is really just a preference thing, it's not a rule. It works fine either way.

[Psycho]

@scootstah: Not exactly. What I meant was it is inefficient to run those three lines before they are needed. For example, I see a lot of people in form processing scripts where the first thing they do is connect to the database. Then they run a bunch of validations (e.g. string length, characters, etc.). If the validations pass then they run an INSERT/UPDATE query. But, if you aren't going to run the query if the validations don't pass - then why connect to the DB before doing the validations? Of course if one of the validations requires a query, that is a different story. But, the DB validations should only be done after all the other 'easy' validations have passed. DB transactions are costly and should not be run unless needed.

 

In the previous code, I assumed the set_rules() method was setting some value to be used by the run() method. If that is the case, and it seems so, then it made no sense to run the set_rules() at the top since the run() would only be executed if the validations passed. But, the change in logic makes that moot.

 

@CoolAsCarlito: I already gave you my recommendations. You've incorporated some and not others. And that's your choice. If you ask me my opinion on the new code I'll just suggest you follow all of my suggestions. I'm not being arrogant, but my opinions aren't going to change because you didn't want to heed them. If it works for you then fine.

 

But, since you ask, I still think you are taking the wrong approach by echo'ing errors in the function. Since you have a class you should create an error message property. Then if there are validation errors, set the error message property. Then the function can return true/false. If false, then access the error message property and display it.

[Xtremer360]

The issue I have with the 3 lines is because this line $user_id = $this->users->get_user_id_by_username($this->input->post('username'));  you have to validate that the username meets the rules before you can  even post it to the function.

 

[Xtremer360]

Also should I be using set_value or the post function of the login function parameter.

[scootstah]

But, since you ask, I still think you are taking the wrong approach by echo'ing errors in the function. Since you have a class you should create an error message property. Then if there are validation errors, set the error message property. Then the function can return true/false. If false, then access the error message property and display it.

 

He is using an MVC framework, it doesn't return to anything. I think echo'ing the json in this case is acceptable.

[Psycho]

Also should I be using set_value or the post function of the login function parameter.

 

No idea since I don't know what set_value() even does exactly.

 

But, since you ask, I still think you are taking the wrong approach by echo'ing errors in the function. Since you have a class you should create an error message property. Then if there are validation errors, set the error message property. Then the function can return true/false. If false, then access the error message property and display it.

 

He is using an MVC framework, it doesn't return to anything. I think echo'ing the json in this case is acceptable.

 

Well, login_submit() is a method of that class. The class has to to instantiated somewhere and then there has to be something that initiates the method login_submit(). My position is that the method should set an error message parameter for the class then return false. If one of the redirect() conditions was met then there probably isn't a need to return anything (I assume).

 

Example:

$login = new Login();
if(!login_submit())
{
    echo $login->errorMessage;
}

[scootstah]

That's not generally how MVC works. The class is dynamically instantiated and you don't ever have to deal with return values.

 

If echoing directly in the controller is bothersome then you could use a response class to output the JSON, but it really isn't necessary.

[Psycho]

That's not generally how MVC works. The class is dynamically instantiated and you don't ever have to deal with return values.

 

If echoing directly in the controller is bothersome then you could use a response class to output the JSON, but it really isn't necessary.

 

It is not bothersome to me. As I quite plainly stated previously I could care less what any one does or doesn't do - if it works for them great. but, if you ask my opinion I will give it. And, I still disagree with you regarding the output in the functions/methods. The output belongs in the View component not the model or controller components.