Skip to main content
2 of 3
Added larger chunk data example
Paul
  • 4k
  • 2
  • 23
  • 38

Another recommendation apart from all of the good ones you have already received.

Don't create variables that are not variables.

                // Assign variables to returned data 
                $passwordDB = $user_data->password;
                $passwordDB2 = $user_data->password2;
                $first_name = $user_data->first_name;
                $last_name = $user_data->last_name;
                $email = $user_data->email;
                $users_roles_id = $user_data->users_roles_id;

These variables are assigned once and used once (or possibly twice), however they are never modified. They only abstract away the details of what is being done later in the function. Accessing the object properties is more descriptive than referring to local variables. When you have a local variable you have to track back to where it was last set to see its value. Also, there is less clutter without these lines that do very little.

Personally I like to handle data in larger chunks rather than lots of fields spread everywhere. I would be using arrays to hold the data and avoid line by line first name, last name, email access. Here is how I would do it for the if block that follows the variables i referred to above.

                // No need to Assign variable to returned data.

                if ($user_data->valid_password())
                {
                    // loggedin doesn't feel like a real object, it looks like
                    // something you are using to call functions with.  I would
                    // call it login_manager if it was managing the logins.
                    $this->loggedin->update_logins($user_id);
                    $this->loggedin->clear_login_attempts($user_id); 

                    // Again, I wouldn't Create variables about user and assign them to session for database entry

                    // This call makes loggedin do something to the session.
                    // loggedin should be called with things about logins, not
                    // with direct calls to modify the session.
                    // $this->loggedin->insert_session($user_id, $session_id, $user_ip, $user_browser, $date_time);

                    // The logic for this belongs in the session.
                    $this->session->set_userdata($user_data);
                    redirect('cpanel/index');     
                }
Paul
  • 4k
  • 2
  • 23
  • 38