How to make controller more optimised

PHP
1

Hi,

How can I make this code a bit smaller?

Thanks

private function validate($id=NULL)
		{
			if(!is_numeric($id))
				{
					echo "Invalid request";exit();	
				}
			if(!$this->header->logged())
				{
					redirect(site_url('log_in'), 'refresh');
				}
			$data['card_details'] = $this->model_auth->card_details_id($id,$user_id);
			if(count($data['card_details'])>0)
				{
					if($data['card_details'][0]->fkUserId!=$this->user_id)
						echo "Card does not belong to this user!";exit();
				}	
			else
					echo "Card does not belong to this user!";exit();
		}
2

Why do you want the code to be smaller?

I prefer functions to

  1. always return a value on success or failure.
  2. assume and check first for valid values.
  3. prefer not to display any information within the function.
  4. prefer not to exit from a function.

private function validate( $idMsg= 'Invalid request, $id must be a valid number')
 {
   if(  $result = is_numeric($idMsg) )
   {
    if( ! $this->header->logged() ) 
    {
       redirect( site_url('log_in'), 'refresh');
     } 

    $data['card_details'] = $this->model_auth->card_details_id($idMsg, $user_id); 
    $result  =  count($data['card_details']) > 0
               && 
                  $data['card_details'][0]->fkUserId == $this->user_id;
    $idMsg = "Card does not belong to this user!";

   }//  $result = is_numeric($idMsg) 

  // Not sure what is required here 
  if( $result )
  {
     return $result;
  } 

  // not recommended
     echo $idMsg;
     exit;
}//
3

Although it might not look like it at first glance that method/function has far to many responsibilities. In a typical OO/MVC architecture routing, authentication, and authorization/permission checking would be handled in isolated areas of code. The router would typically be responsible for validating the route. That would include whether a controller/method combination exists for a given path and verifying all extra params follow the proper format using a regular expression or custom validation class for more involved route validation. Once than typically a separate layer would handle authorization and possibly redirect to a login page if the user had not been authenticated to access the page. provided a user was authorized the controller action would than check permissions/authorization for a particular action such as; a CRUD operation. It is at this point that the front-end controller might catch some type of permission exception and issue a status code of 401 if the user failed authorization. Combining all that in a single function goes against the single responsibility principle but more importantly will result in a lot of duplication for actions that require a similar workflow. generally speaking the front end controller would handle any access exceptions throughout the life cycle of the request. Using exit and echo for anything other than possibly debugging without using a template and/or view will result in a very inflexible and fragile architecture.

4

Maybe something like this or a totally new class?

private function authenticate()
	{
		if(!$this->header->logged())
            {
                redirect(site_url('log_in'), 'refresh');
            }
	}

private function validate($id=NULL)
    {
        if(!is_numeric($id))
            {
                echo "Invalid request";exit();    
            }
        $data['card_details'] = $this->model_auth->card_details_id($id,$user_id);
        if(count($data['card_details'])>0)
            {
                if($data['card_details'][0]->fkUserId!=$this->user_id)
                    echo "Card does not belong to this user!";exit();
            }    
        else
                echo "Card does not belong to this user!";exit();
    }
5

IMO, when you handle errors, you should use an array to store the errors. Because as it is now, if I understand correctly, the person would send the form and only get one error at a time.
Also, you can split your functions into multiple smaller functions so it’s easier to read.

I can see a couple of functions here :

private function validate($id)  {
  $errors = array();
  $errors = $this->validate_id($id, $errors);
  $errors = $this->validate_cards_details($id, $user_id, $errors);
  return $errors;
}

private function validate_id($id, &$errors) {
        if(!is_numeric($id))
            {
                $errors['id'] = "Invalid request";
            }

        return $errors;
}

private function validate_cards_details(...) {
...
}

Then, you can call your validate function, get the $errors back and just check if it’s empty. If it’s not, loop trough it to display the errors.

For the “authenticated” method, you should put it somewhere where you can call it from all your controllers (or at least, the controllers you need to call it from):

public function  is_autenticated() {
if (...) { 
  return true;
}

return false;
}

public function redirect_if_anonymous() {
  if (!$this->is_authenticated() {
    redirect(site_url('log_in'), 'refresh'); 
  }
}

Now you just need to call the “redirect_if_anonymous()” method.

Is this what you meant by “smaller” ? It’s not necessarily smaller, but it’s easier to read and the functions are doing one thing.