User management OOP php class

Question

I found a class that i coded for an old project about a year ago and i haven't really touched php in a while. Now i need to use similar functionality again i thought it would be the perfect time to rewrite/improve old class. i would appreciate feedback on any errors you notice or advice on anything i should use such as design patterns etc.

include "db_config.php";

/**
 * 
 */
class User
{
    //object variables
    public $pdo;
    public $user_data;

    public function __construct()
    {

        $this->pdo = new PDO(
            "mysql:host=" . DB_SERVER . ";dbname=" . DB_DATABASE, //DSN
            DB_USERNAME, //Username
            DB_PASSWORD //Password
        );

        $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        $this->pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

        //add pdo error check


        //check to see if the 
        if (isset($_SESSION['uid'])) {
            $this->user_data = array('uid' => $_SESSION['uid'], 
                               'email' => $_SESSION['email'], 
                               'fname' => $_SESSION['fname'], 
                               'lname' => $_SESSION['lname']);
        }
    }

    /**
     *  Register Function 
    */
    public function user_register($email, $password, $fname, $lname)
    {
        $sql = "SELECT uemail FROM user_profile WHERE uemail = :email";
        $stmt = $this->pdo->prepare($sql);

        $stmt->bindValue(':email', $email);

        $stmt->execute();
        $row = $stmt->fetch(PDO::FETCH_ASSOC);
        $rowCount = $stmt->rowCount();
        if ($rowCount > 0) {
            //Email is already registered
            return 0;
        } else {//if email doesnt exist

            //hash the password
            $hashPassword = password_hash($password, PASSWORD_BCRYPT, array("cost" => 12 ));

            //prepare insert statement
            $sql = "INSERT INTO user_profile (uemail, upass, fname, lname) VALUES (:email, :password, :fname, :lname)";
            $stmt = $this->pdo->prepare($sql);

            //bind varaiables
            $stmt->bindValue(':email', $email);
            $stmt->bindValue(':password', $hashPassword);
            $stmt->bindValue(':fname', $fname);
            $stmt->bindValue(':lname', $lname);

            //execute instert statment
            $result = $stmt->execute();

            if ($result) {
                //register success
                $this->user_login($email, $password);
                return 1;
            }
        }
    }

    //login function
    public function user_login($email, $password)
    {
        $sql = "SELECT * FROM user_profile WHERE uemail = :uemail";
        $stmt = $this->pdo->prepare($sql);

        $stmt->bindValue(':uemail', $email);

        $stmt->execute();
        $row = $stmt->fetch(PDO::FETCH_ASSOC);
        $rowCount = $stmt->rowCount();
        if ($rowCount > 0) {
            $test = password_verify($password, $row['upass']);
            if (password_verify($password, $row['upass'])) {
                $_SESSION['uid'] = $row['id'];
                $_SESSION['email'] = $row['uemail'];
                $_SESSION['fname'] = $row['fname'];
                $_SESSION['lname'] = $row['lname'];
                $_SESSION['logged_in'] = date("Y-m-d H:ia");
                header("Location: index.php");
            }
        } else {
            return false;
        }
    }

    //get first name
    public function get_fname($uid)
    {

        return $user_data['fname'];
    }

    public function get_lname($uid)
    {
        return $user_data['lname'];
    }

    // starting session 
    public function get_id()
    {
        return $_SESSION['uid'];
    }

    public function check_login()
    {
        if (isset($_SESSION['logged_in'])) {
            return true;
        } else {
            return false;
        }
    }
    //logout function to destroy session
    public function user_logout()
    {
        $_SESSION['uid'] = FALSE;
        $_SESSION['email'] = FALSE;
        $_SESSION['fname'] = FALSE;
        $_SESSION['lname'] = FALSE;
        $_SESSION['logged_in'] = FALSE;
        session_unset();
        session_destroy();
        header("Location: login.php");
    }

}

also i think this was my first time using PDO so any feedback on the use or implementation of that would be appreciated Thanks in advance for the feedback

Answer 1

here are some comments on the class code which I hope are of some use to you.

Dependency Injection Creating the PDO object inside the constructor introduces a POD dependency on User class. Think about SOLID principles when writing classes. The "S" in "SOLID" stands for Single Responsibility. Classes should have a Single responsibility - one specific purpose. The User class should not be dependent about what database connection it uses.

When building more complex software with many classes this becomes extremely important to reduce class coupling (and reduce complexity). Say if you need to mock an object (that class you want to mock because it not be written yet, or may not have been purchased yet from an external vendor) you can still test your class.

So, pass the PDO object into the User class constructor:

 // PDO object is injected into User class
    public function __construct(PDO $pdo)
    {
        $this->pdo = $pdo;

You may also consider (depending on your requirements) managing the user's session in another class and injecting that class into User.

Type Declarations Type declarations (or hinting as it was known in PHP5) has been around a while now. It was available for some types in PHP5. Now in PHP7 the feature is more extensive. Also we can enforce the type of parameters and return types. What benefits does this have?

• It states intent: making it clear what types functions and methods support. It communicates to others (your team members say) what types are valid.

• It promotes better code: any use of the wrong type can cause an
  exception to throw. No-one can ignore an exception.

• Helps when using a PHP IDE.

• Can help eliminates issues due to type coercion. For example, we may not wish our string to be converted to an integer.

For more details read PHP7 new features

Rather than return either 0 or 1, enforce the return type as bool. It more clearly states the intent of the method.

/**
 *  Register Function 
 *
 * Register a user to the system by adding record to the 
 * user_profile table, and then calls login method.

 * @param string $email - User's email
 * @param string $password - User's password
 * @param string $fname - User's first name
 * @param string $lname - User's last name
 * @return bool - true if user is registered without error.
*/
public function user_register(string $email, 
    string $password, 
    string $fname, 
    string $lname) : bool
{
   ....
    if ($result) {
      // register success
      $this->user_login($email, $password);
      return true;
    }

    ....
    return false; // user not registered
 }

Documentation You've commented some methods which is good. Consider extending this by using a documentation generator such as PHPDoc. In fact you've used PHPDoc format for register_user(). But the method comments should document the method purpose, what it returns and what the parameters are for.

This helps you remember when returning to code you've written some time ago (as we all have done and you have mentioned you have in your post). Many IDE's (such as Netbeans, PHPStorm) use the documentation to help interpret code. See above code for example.

Answer 2

Abstraction Your class represents a user, user registration, authentication, and data access all at the same time. This is not ideal because with so many responsibilities, it is difficult to reason about and to maintain if your requirements change. To correct this, first break your class into:

• User
• Registration
• Authentication
• UserDb

Once you have done this, begin the allocation of responsibilities. You do this by thinking about the questions and requests that your application must respond to; for example, 'can a user log in with this username and password?', 'register a new user with these details', 'what is the name of the current user?', 'is the current user logged in?', etc.

Let's tackle a couple of these examples--

'Can a user log in with this username and password?' This can be answered by classes User and Authentication. In addition, Authentication needs the collaboration of UserDb to get user data from the database. So, you need methods Authentication::validate($username, $password), UserDb::findUserByUsername($username), and User::hasPassword($password). Finally, the answer to the question is obtained with the following sequence of operations:

$pdo = new PDO(...);
$userdb = new UserDb($pdo);
$authentication = new Authentication($userdb);
$authenticated = $authentication->validate("user1", "password1");

Let's look at how your UserDb and Authentication classes might look--

class UserDb {

    private $pdo;

    public function __construct($pdo) {
        $this->pdo = $pdo;
    }

    public function findUserByUser($username) {
        $data = ...;
        return new User($data["username"], $data["password"], ...);
    }
}

class Authentication {

    private $userdb;

    public function __construct($userdb) {
        $this->userdb = $userdb;
    }

    public function validate($username, $password) {
        $user = $this->userdb->findUserByUsername($username);
        return $user != null && $user->hasPassword($password);
    }    

To address the second request, 'register a new user with these details', you need classes Registration and User to own the responsibilities, for example:

class Registration {

    private $userdb;

    public function __construct($userdb) {
        $this->userdb = $userdb;
    }

    public function register($username, $password) {
        $user = new User($username, $password);
        $this->userdb->saveUser($user);
    }
}

class User {

    private $username, $password;

    public function __construct($username, $password) {
        // validate $username and $password
        $this->username = $username;
        $this->password = $password;
    }
}

And, they can be used like this, with the existing $userdb object.

$registration = new Registration($userdb);
$registration->register("user1", "password1");

And so on--

The idea is that each class is responsible for satisfying a set of requirements and must collaborate with other classes (with their own unique requirements) to meet them if necessary.

Dependency injection I won't comment on this, as it is already covered by @suspectus. But, I will point out that it is applied in the example above. You can see that the same UserDb object can be injected in both Authentication and Registration, so it is reasonable that you want $userdb to be a singleton.

Answer 3

The answer by suspectus covers DI, type declarations and documentation well. There are a few other aspects I noticed, covered in the sections below.

Avoiding else when possible

Most of the methods aren't too long, though user_register() is a bit on the long side. There isn't really a need to have the else keyword, since the if block before it has a return statement.

In this presentation about cleaning up code Rafael Dohms talks about limiting the indentation level to one per method and avoiding the else keyword. (see the slides here). For instance, in the user_login() method, the else could be avoided if the logic to return false came first... something like:

if ($rowCount < 1) { 
    return false;
} //implicit else
$test = password_verify($password, $row['upass']);
if (password_verify($password, $row['upass'])) {
    $_SESSION['uid'] = $row['id'];
    $_SESSION['email'] = $row['uemail'];
    $_SESSION['fname'] = $row['fname'];
    $_SESSION['lname'] = $row['lname'];
    $_SESSION['logged_in'] = date("Y-m-d H:ia");
    header("Location: index.php");
}

---

Methods accessing instance variables/properties

I noticed that the methods get_fname() and get_lname() utilize $user_data which is not a local variable but instead an instance variable. I believe that would work in Java but not PHP. It should utilize $this->user_data to work properly in PHP.

//get first name
public function get_fname($uid)
{
    return $this->user_data['fname'];
}

public function get_lname($uid)
{
    return $this->user_data['lname'];
}

---

Simplifying boolean logic

The method check_login() can be simplified from:

public function check_login()
{
    if (isset($_SESSION['logged_in'])) {
        return true;
    } else {
        return false;
    }
}

To just return the condition:

public function check_login()
{
    return isset($_SESSION['logged_in']);
}

This is because isset() returns a bool