HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PDO Login/Register system I've been working on

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
systemloginworkingbeenregisterpdo

Problem

I'm relatively new to PHP and programming as whole. I'm sure my code could be better in a lot of ways, most of which I can't fully grasp at the moment. As it stands I'm trying to avoid the general noobie mistakes(whatever they may be). And also trying to code better and cleaner so that other people can understand without too much hassle.

This is a simple login system I've been working on for the past few days.

Be harsh, please, point out as many flaws as you can. And, if possible, offer solutions to fix said flaws.

Link on GitHub to the code.

The general area of interest would be the classes.

Login class:

```
doLogin();
}
}

/**
* log in with post data
*/
private function doLogin(){
if( ! empty($_POST['username']) &&
! empty($_POST['password'])){

$username = ($_POST['username']);
$password = ($_POST['password']);

require_once 'db/db_connect.php';
require_once 'db/db_tables.php';

/**
* PHP Version 5.4.31 doesn't support password_hash()
* so an extension called password_compat is used
* Link to lib - https://github.com/ircmaxell/password_compat
*/
require_once 'password_compat/lib/password.php';

$sqlQuery = $dbPDO->prepare("SELECT $loginPassword
FROM $tableName
WHERE $loginUsername=:username");
$sqlQuery->execute(array(':username' => $username));

$hash = $sqlQuery->fetch();

$passwordVerify = password_verify($password, $hash['login_password']);

/**
* Note that the variables used here
* come from the db_tables.php file
* not from user input
*/
$sqlQuery = $dbPDO->prepare("SELECT $loginUsername, $loginId FR

Solution

First of, good luck with your interview!
Over thinking and not thinking enough

Writing code is easy, but writing code that will last longer then a minute is hard. You obviously know how to code. And you have a notion of separation of concern. You comment your code (not 100%, but still. you comment).

But still, your code is crap. Sorry, but it really is. You are abusing classes so hard that kittens are dying. Not to speak of you overall design. You did some thinking, took the wrong decisions and then did some more thinking in that wrong direction.
Those useless objects

  • None of your objects have public methods. So what can I do with them?



  • Take away your is* methods and every class just has 1 method. And it is private but called in the __construct.



So to sump up: you wrote 4 functions that do, well nearly everything. For instance your Login::doLogin, it knows:

  • how the data is send to the application ($_POST stuff)



  • where you database files are located and what they do (require stuff)



  • that php5.4.31 doesn't support password_hash (I didn't even know that)



  • how to query a database (no very good at it, he performs the same query twice but retrieves different data.)



  • How to handle a use session ($_SESSION stuff)



  • How to construct html and when to output it to the user



  • How to send a header along with the response.



  • how to validate input



And then you write 'A simple login system'. But one you can't use with no public methods. And one that does everything his way. You want the output in a different language? Well go f@ck yourself! Oh, you want a different kind of database? Well, not-my-problem. You want to change where you save your db methods? Haha, suck3r

What you did write is a cocky login system that can't be reused without touching it (sounds so wrong).
The design is bad

When a single method does so much, something is wrong. And if it is the only method in a class, and private. Then kittens will die. So let's fix it by going back to the drawing board. And I literally mean pen and paper. Every good code starts on a drawing board (or equivalent), not an editor.
What do you actually want to make?

What is a Login? The word itself is very vague. Do you want a login form? Or a system that let's you login using different auth-providers (facebook, google, stackexchange,...) or is it a UserSession handler? OR should it be a huge piece of code that does all that and some more all in one method?
Validation is hard!

In SOLID, the first character stands for Single Responsibility. Every little class should be small. Not small as in kb, but small as in 'what it does'. It shouldn't do multiple things at the same time. Do you need Validation? Then create a Validator class:

class Validator
{
    private $rules;

    public function addRule($rule)
    {
        $this->rules[] = $regex;
    }

    public function validate($value)
    {
        foreach ($this->rules as $rule) {
            if ( 1 !== preg_match($rule, $value) ) {
                return false;
            }
        }
        return true;
    }
}


We can use it as follows:

$passwordValidator = new Validator();

//passwords shouldn't restrict the usage of certain characters.
//neither should they use a max-length.
$passwordValidator->addRule('/[.]{5,40}$/i'); //is it long enough?
$passwordValidator->addRule('[0-9]+'); //at least one digit
$passwordValidator->addRule('[A-Z]+'); //at least one uppercase
$passwordValidator->addRule('[a-z]+'); //at least one lowercase

//let's validate a given input
$passwordValidator->validate($_POST['password']); //boolean


Later on you could then extend your validator to accept a meta-syntax instead of raw regex. Laravel has some nice ones.

Having a valdiator object gives us the great advantage of removing those pointless isEmailValid checks in your classes and use our validation objects instead.

Now we have our validator, we can go on with our Authentication class. An Authentication class should give us the ability to login a user, check if the credentials are correct and also retrieve the currently logged in user. It would look something like this:

class Auth
{

    /**
     * @param  String $username The username
     * @param  String $password the password
     * @return boolean          succesfull login?
     */
    public function login($username, $password)

    /**
     * Log the user out
     * @return void
     */
    public function logout()

    /**
     * @return boolean is the user logged in?
     */
    public function check()

    /**
     * @return User the currently logged in user
     * @return null IF !check()
     */
    public function user()

}


Our Auth class however needs some other stuff to work. It needs a Session handler and a UserRepository - something that fetches user objects for us. We thus add a __construct that asks for those objects:

```
class Auth
{
public function __construct(SessionHandler $session, UserRepository $user)

Code Snippets

class Validator
{
    private $rules;

    public function addRule($rule)
    {
        $this->rules[] = $regex;
    }

    public function validate($value)
    {
        foreach ($this->rules as $rule) {
            if ( 1 !== preg_match($rule, $value) ) {
                return false;
            }
        }
        return true;
    }
}
$passwordValidator = new Validator();

//passwords shouldn't restrict the usage of certain characters.
//neither should they use a max-length.
$passwordValidator->addRule('/[.]{5,40}$/i'); //is it long enough?
$passwordValidator->addRule('[0-9]+'); //at least one digit
$passwordValidator->addRule('[A-Z]+'); //at least one uppercase
$passwordValidator->addRule('[a-z]+'); //at least one lowercase

//let's validate a given input
$passwordValidator->validate($_POST['password']); //boolean
class Auth
{

    /**
     * @param  String $username The username
     * @param  String $password the password
     * @return boolean          succesfull login?
     */
    public function login($username, $password)

    /**
     * Log the user out
     * @return void
     */
    public function logout()

    /**
     * @return boolean is the user logged in?
     */
    public function check()

    /**
     * @return User the currently logged in user
     * @return null IF !check()
     */
    public function user()

}
class Auth
{
    public function __construct(SessionHandler $session, UserRepository $user)
}
interface SessionHandler
{
    public function get($key);
    public function set($key, $value);
    public function has($key);
    public function unset($key);
}

interface UserRepository
{
    public function getBy($key, $value);
}

Context

StackExchange Code Review Q#61605, answer score: 9

Revisions (0)

No revisions yet.