I'm pretty new to PHP and programming general so I'm quite sure my code is an absolute mess. My goal is to write good code, that others can understand just by looking at it.
This is a simple login system I've been working on. Critique at will.
General area of interest are the classes:
<?php
/**
* Handles the login of the user
*/
class Login
{
/**
* This starts whenever an object of this class is created
* For example $login = new Login();
* Coincidently one just like this exists in index.php
*/
public function __construct(){
//create or read session
session_start();
/**
*If a cookie exists on the users PC named user and id
*Just redirect him to logged_in page.
*TODO: I should really encrypt these cookies
*/
if((isset($_COOKIE['user'])) && (isset($_COOKIE['id']))){
header('Location: logged_in.php');
}
require_once 'forms/login_form.php';
//If not logged it, log the user in
if(isset($_POST['login'])){
$this->doLogin();
}
}
/**
*@bool Return true or false
* Note: This function repets in the other 2 classes
* I should probably make it not repet
*/
private function isEmpty($var){
$var = trim($var);
if (empty($var)){
return true;
}
//default case
return false;
}
/**
* log in with post data
*/
public function doLogin(){
if( ! $this->isEmpty($_POST['username']) ||
! $this->isEmpty($_POST['password'])){
$username = ($_POST['username']);
$password = ($_POST['password']);
/*
* sha1 is just for demonstration purposes
* it will be replaced with password_hash once I update
* to PHP 5.4.x I'm currently using 5.3 due to Win XP limitations
*/
$password = sha1($password);
/**
* Require the files needed
*/
require_once 'db/db_connect.php';
require_once 'db/db_tables.php';
/**
* Note that I'm using variables here
* Don't worry, they don't come from user input
* They come from the db_tables.php file
*/
$sqlQuery = $dbPDO->prepare("SELECT $loginUsername, $loginEmail, $loginId
FROM $tableName
WHERE $loginUsername=:username
AND $loginPassword=:password");
$sqlQuery->execute(array(':username' => $username,
':password' => $password));
/**
* Note: I could have used PDO's rowCount() here but chose not to
* There's no particular reason why I did that
*/
$isInDb = $sqlQuery->fetch(PDO::FETCH_ASSOC);
if( ! $isInDb){
echo '<p> The username or password do not match any registered users </p>';
} else {
/**
* Great, the user's logged in
* Time to set the cookies and redirect him
*/
setcookie('user', $isInDb['login_username']);
setcookie('id', $isInDb['login_id']);
$_SESSION['id'] = $isInDb['login_id'];
header('Location: ' . 'logged_in.php');
}
} else {
echo 'You must fill in all fields';
}
}
}
RecoverPassword class
<?php
/**
* Handles the password recovery
*/
class RecoverPassword
{
/**
* This gets execute whenever an object from this class is created
*/
public function __construct(){
if(isset($_POST['recoverPassword'])){
$this->doRecover();
}
}
/**
* @bool checks if the email is in a valid format
* Note: This has some flaws
* For example using numbers in the name of the email MAY give back some false negatives
* For more details see this question: https://stackoverflow.com/questions/3722831/does-phps-filter-var-filter-validate-email-actually-work
*/
private function isEmailValid($email){
if (filter_var(trim($email), FILTER_VALIDATE_EMAIL)){
return true;
}
return false;
}
/**
*@bool Return true or false
* Note: This function repets in the other 2 classes
* I should probably make it not repet
*/
private function isEmpty($var){
$var = trim($var);
if (empty($var)){
return true;
}
return false;
}
/**
* @bool Simple regex match
*/
private function isPasswordValid($password){
return (preg_match('/^(?=.*\d)(?=.*[A-Za-z])[0-9A-Za-z!@#$%]{5,40}$/', trim($password)));
}
/**
* @bool Here is the actual recovery of the password...which doesn't work.
*/
private function doRecover(){
if( ! $this->isEmpty($_POST['email'])){
if($this->isEmailValid($_POST['email'])){
$email = $_POST['email'];
/**
* Require the connect to DB and tables files
*/
require_once 'db/db_connect.php';
require_once 'db/db_tables.php';
/**
* Note that I'm using variables here
* Don't worry, they don't come from user input
* They come from the db_tables.php file
*/
$sqlQuery = $dbPDO->prepare("SELECT $loginEmail FROM $tableName WHERE $loginEmail= :email");
$sqlQuery->execute(array(':email' => $email));
/**
* Note: I could have used PDO's rowCount() here but chose not to
* There's no particular reason why I did that
*/
$isInDb = $sqlQuery->fetch(PDO::FETCH_ASSOC);
//Does the email exist?
if( ! $isInDb){
die('<p>Email doesn\'t exist!</p>');
} else {
echo 'The email exists. Currently the recover password email will not send due to not having a proper SMPT configured.';
//TODO - Add mail function to send recover password
}
} else {
echo '<p>Email is invalid</p>';
}
} else {
echo ('<p>Email field is empty!</p>');
}
}
}
Register class
<?php
/**
* Handles the register of the user
*/
class Register
{
/**
* This gets execute whenever an object from this class is created
*/
public function __construct(){
//Require the HTML part of page
require_once 'forms/register_form.php';
//If user presses the register button, register the user
if(isset($_POST['register'])){
$this->doRegister();
}
}
/**
*@bool Return true or false
* Note: This function repets in the other 2 classes
* I should probably make it not repet
*/
private function isEmpty($var){
$var = trim($var);
if (empty($var)){
return true;
}
//default case
return false;
}
/**
* @bool
*/
private function isEmailValid($email){
#$email = trim($email);
if (filter_var(trim($email), FILTER_VALIDATE_EMAIL)){
return true;
}
//default case
return false;
}
/**
* @bool
*/
private function isUsernameValid($username){
return (preg_match('/^[a-z\d_]{5,20}$/i', trim($username)));
}
private function isPasswordValid($password){
return (preg_match('/^(?=.*\d)(?=.*[A-Za-z])[0-9A-Za-z!@#$%]{5,40}$/', trim($password)));
}
/**
* Here we register the user
* Add him to the database etc etc
*/
private function doRegister(){
//Make sure the fields are not empty
if( ! $this->isEmpty($_POST['username']) ||
! $this->isEmpty($_POST['password']) ||
! $this->isEmpty($_POST['email'])){
} else {
/**
* I'm using die here because I chose not to do the
* check using java-script or HTML
* This is purely done for the hell of it
* However, java-script or HTML would have been the better choice
* This applies to all the die();s in this page
*/
die('<p>Some fields are empty.</p>');
}
//Is the email valid?
if($this->isEmailValid($_POST['email'])){
$email = trim($_POST['email']);
} else {
die('<p>Email is invalid</p>');
}
//Is username valid?
if($this->isUsernameValid($_POST['username'])){
$username = trim($_POST['username']);
} else {
die('<p>The username must be between 5 and 20 characters long. And it can only contain numbers and letters!</p>');
}
if($this->isPasswordValid($_POST['password'])){
$password = trim($_POST['password']);
/*
* The password is hashed with sha1 simply because I'm using
* PHP 5.4 - I will replace it with password_hash() when I upgrade to PHP 5.4
*/
$password = sha1($password);
} else {
die('<p>The password must be between 5 and 40 characters long, must contain at least one number,
at least one letter, and any of the following characteres: ! @ # $ %</p>');
}
require_once 'db/db_connect.php';
require_once 'db/db_tables.php';
/**
* The queries below are repetive
* in order to display an error for
* username or email
* I think I could marked the coulmns in the DB
* as Unique to make things easier however I didn't get around to trying that
* and I'm not sure if it works
*/
$sqlQuery = $dbPDO->prepare("SELECT $loginUsername FROM $tableName WHERE $loginUsername=:username");
$sqlQuery->execute(array(':username' => $username));
$isInDb = $sqlQuery->fetch(PDO::FETCH_ASSOC);
if($isInDb['login_username'] == $username){
die('<p>That username is already registered!</p>');
}
$sqlQuery = $dbPDO->prepare("SELECT $loginEmail FROM $tableName WHERE $loginEmail=:email");
$sqlQuery->execute(array(':email' => $email));
$isInDb = $sqlQuery->fetch(PDO::FETCH_ASSOC);
//Does the email already exist?
if($isInDb['login_email'] == $email){
die('<p>That email is already registered!</p>');
}
/**
* Here we actually register the user
* He passed all the checks and we can sefely
* insert him into the database
*/
$sqlQuery = $dbPDO->prepare("INSERT INTO $tableName($loginUsername, $loginPassword, $loginEmail)
VALUES (:username, :password, :email)");
if($sqlQuery->execute(array(':username' => $username,
':password' => $password,
':email' => $email))){
echo '<p>Registration succesful!</p>';
} else {
die('<p> There was an error in the registration process.</p>');
}
}
}
