SitePoint Sponsor

User Tag List

Results 1 to 5 of 5
  1. #1
    SitePoint Enthusiast
    Join Date
    May 2014
    Posts
    37
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)

    How to make controller more optimised

    Hi,

    How can I make this code a bit smaller?

    Thanks

    PHP Code:
    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. #2
    SitePoint Mentor bronze trophy
    John_Betong's Avatar
    Join Date
    Aug 2005
    Location
    City of Angels
    Posts
    1,883
    Mentioned
    74 Post(s)
    Tagged
    6 Thread(s)
    Quote Originally Posted by English BT View Post
    Hi,
    How can I make this code a bit smaller?
    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.

    PHP Code:
    private function validate$idMsg'Invalid request, $id must be a valid number')
     {
       if(  
    $result is_numeric($idMsg) )
       {
        if( ! 
    $this->header->logged() ) 
        {
           
    redirectsite_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;
    }
    // 
    Last edited by John_Betong; May 27, 2014 at 20:53. Reason: formatting
    Learn how to be ready for The New Move to Discourse

    How to make Make Money Now with a *NEW* look

    Be sure to congratulate Wolfshade on earning Member of the Month for August 2014

  3. #3
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2006
    Location
    Augusta, Georgia, United States
    Posts
    4,184
    Mentioned
    17 Post(s)
    Tagged
    4 Thread(s)
    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.
    The only code I hate more than my own is everyone else's.

  4. #4
    SitePoint Enthusiast
    Join Date
    May 2014
    Posts
    37
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by oddz View Post
    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.
    Maybe something like this or a totally new class?
    PHP Code:
    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. #5
    SitePoint Zealot bronze trophy xMog's Avatar
    Join Date
    Mar 2011
    Posts
    151
    Mentioned
    3 Post(s)
    Tagged
    2 Thread(s)
    Quote Originally Posted by English BT View Post
    Maybe something like this or a totally new class?
    PHP Code:
    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();
        } 

    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 :

    Code:
    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):

    Code:
    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.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •