Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This PHP User System was built with MySQLi and I also used Composer. I'm planning to improve this, and add more stuff and release it as a sort of a module for Composer.

User.php (Controller File in the src/Controller) folder.

<?php
namespace Application\Controllers;

use Http\Request;
use Http\Response;
use Application\Users\Mapper;

class User
{
    private $request;
    private $response;
    private $userMapper;

    public function __construct(Request $request, Response $response, Mapper $mapper)
    {
        $this->request = $request;
        $this->response = $response;
        $this->userMapper = $mapper;
    }

    public function login()
    {
        $username = $this->request->getParameter('username');
        $password = $this->request->getParameter('password');


        if($this->userMapper->authenticateLoginDetails($username, $password)) {
            $this->userMapper->createSession($username);
            $this->userMapper->fetchUserDataAndPopulateUserObject($username);
            $this->response->redirect('/dashboard');
            return true;
        }
        $this->response->redirect('/');
        return false;
    }

    public function logout()
    {
        $this->userMapper->destroySession();
        $this->userMapper->cleanUserObject();
        $this->response->redirect('/');
    }
}

Module Files:

User.php (src/Users/User.php)

<?php
namespace Application\Users;

use \mysqli;

abstract class User
{
    protected $database;
    private $id;
    private $username;
    private $password;
    private $email;

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

    public function setID($value)
    {
        $this->id= $value;
    }

    public function getID()
    {
        return $this->id;
    }

    public function setUsername($value)
    {
        $this->username = $value;
    }

    public function getUsername()
    {
        return $this->username;
    }

    public function updateUsername($newUsername)
    {
        $this->setUsername($newUsername);

        $query = $this->database->prepare("UPDATE users SET username=? WHERE id=?");
        $query->bind_param("ss", $newUsername, $this->getID());
        $query->execute();
        $query->close();
    }

    public function setPassword($value)
    {
        $this->password = $value;
    }

    public function getPassword()
    {
        return $this->password;
    }

    public function updatePassword($newPassword)
    {
        $this->setPassword($newPassword);

        $query = $this->database->prepare("UPDATE users SET password=? WHERE id=?");
        $query->bind_param("ss", $newPassword, $this->getID());
        $query->execute();
        $query->close();
    }

    public function setEmail($value)
    {
        $this->email = $value;
    }

    public function getEmail()
    {
        return $this->email;
    }

    public function updateEmail($newEmail)
    {
        $this->setEmail($newEmail);

        $query = $this->database->prepare("UPDATE users SET email=? WHERE id=?");
        $query->bind_param("ss", $newEmail, $this->getID());
        $query->execute();
        $query->close();
    }
}

ApplicationUser.php:

<?php
namespace Application\Users;

class ApplicationUser extends User {
    private $rank;
    private $balance;
    private $participatingTournaments;
    private $points;

    public function clean()
    {
        $this->setID(null);
        $this->setUsername(null);
        $this->setPassword(null);
        $this->setEmail(null);
        $this->setRank(null);
        $this->setPoints(null);
        $this->setBalance(null);
        $this->setParticipatingTournaments(null);

        return true;
    }

    public function setRank($value)
    {
        $this->rank = $value;
    }

    public function getRank()
    {
        return $this->rank;
    }

    public function setBalance($value)
    {
        $this->balance = $value;
    }

    public function getBalance()
    {
        return $this->balance;
    }

    public function updateBalance($newBalance)
    {
        $this->setBalance($newBalance);

        $query = $this->database->prepare("UPDATE users SET balance=? WHERE id=?");
        $query->bind_param("ss", $newBalance, $this->getID());
        $query->execute();
        $query->close();
    }

    public function setParticipatingTournaments($value)
    {
        $this->participatingTournaments = $value;
    }

    public function getParticipatingTournaments()
    {
        return $this->participatingTournaments;
    }

    public function setPoints($value)
    {
        $this->points = $value;
    }

    public function getPoints()
    {
        return $this->points;
    }

    public function updatePoints($newPoints)
    {
        $this->setPoints($newPoints);

        $query = $this->database->prepare("UPDATE users SET points=? WHERE id=?");
        $query->bind_param("ss", $newPoints, $this->getID());
        $query->execute();
        $query->close();
    }
}

Mapper.php:

<?php
namespace Application\Users;

use \mysqli;

class Mapper
{
    private $database;
    private $userObject;

    public function __construct(mysqli $database, ApplicationUser $userObject)
    {
        $this->database = $database;
        $this->userObject = $userObject;
    }

    public function authenticateLoginDetails($username, $rawPassword)
    {
        $query = $this->database->prepare("SELECT password FROM users WHERE username=?");
        $query->bind_param("s", $username);
        $query->execute();
        $query->bind_result($databasePassword);
        $query->store_result();
        while($query->fetch()) {
            if(password_verify($rawPassword, $databasePassword) == true) {
                $query->close();
                return true;
            }
            $query->close();
            return false;
        }
    }

    public function createSession($username)
    {
        session_start();
        $_SESSION['username'] = $username;
        $_SESSION['logoutTime'] = time() + 3600;
    }

    public function destroySession()
    {
        session_start();

        unset($_SESSION['username']);
        unset($_SESSION['logoutTime']);

        session_destroy();

        session_start();
        session_destroy();
    }

    public function fetchUserDataAndPopulateUserObject($username)
    {
        $query = $this->database->prepare("SELECT id, password, email, rank, balance, points FROM users WHERE username=?");
        $query->bind_param("s", $username);
        $query->execute();
        $query->bind_result($id, $password, $email, $rank, $balance, $points);
        while($query->fetch()) {
            $this->userObject->setID($id);
            $this->userObject->setUsername($username);
            $this->userObject->setPassword($password);
            $this->userObject->setEmail($email);
            $this->userObject->setRank($rank);
            $this->userObject->setBalance($balance);
            $this->userObject->setPoints($points);
        }
        $query->close();

        return true;
    }

    public function cleanUserObject()
    {
        $this->userObject->clean();

        return true;
    }

    public function isUserOnline()
    {
        session_start();
        if(isset($_SESSION["username"]) && strlen($_SESSION["username"]) > 0) {
            return true;
        }
        return false;
    }
}

Usage of the above script in my script is as follows:

User fills in a form like this:

<form method="post" action="user/login">
    <input type="text" name="username">
    <input type="password" name="password">
    <input type="submit" name="Login">
</form>

I use a FastRoute library where I have bind the /user/login URI like this:

['POST', '/user/login', ['Application\Controllers\User', 'login']],

This calls my Controller class. I've placed the source above, if you scroll up.

I'm looking for a top-notch code review which also provides an answer for the following questions:

  • Is my code flexible? How may I make it more flexible?
  • Is my code efficient? How may I make it more efficient?
  • Do you have any recommendations for my code? If so, please state.
  • Have I written SOLID code? If not, what principles have I breached and how and how may make them adhere to the SOLID principles.
  • Do you see any flaws in my code? If so please point out.
  • Is the system secure? If not, how may I make it more secure?
share|improve this question
    
Are you okay with book recommendations instead of code-review? Because you ask really a lot, most of the questions can't even be objectively answered. For example take the part where you ask about SOLID principles. Its missing the important context why you can't say that your own. But it starts already at the very first question if the code is flexible. It highly depends on flexible for what. What are your plans to refactor it and where? As long as you don't say, you might end up adding flexibility at the wrong places which wouldn't be helpful to gain more. –  hakre 2 days ago
    
For some insightful examples with the database access and your classes I recommend the Patterns of Enterprise Application Architecture - depending of the level of data-access you need, you can find patterns in there. After you've located one that pleases you, search for an existing library that works that way and use it. Best code is code you don't need to write and maintain yourself. The usage scenario you paint here doesn't look like it hasn't been already solved. So you should (again) add more context why right now you need a review. –  hakre 2 days ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.