patternphpMinor
PDO Login/Register system I've been working on
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
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
So to sump up: you wrote 4 functions that do, well nearly everything. For instance your
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:
We can use it as follows:
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
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:
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
```
class Auth
{
public function __construct(SessionHandler $session, UserRepository $user)
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 (
$_POSTstuff)
- 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 (
$_SESSIONstuff)
- 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']); //booleanLater 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']); //booleanclass 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.