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

Simple login system

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

Problem

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.

Link to said code - GitHub

General area of interest are the classes:

```
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));

/**

Solution

Just a couple of things I wanted to point out:

-
Your classes are violating the Single Responsibility Principle heavily. They have so many responsibilities; handling sessions, rendering templates, creating PDO objects, writing to the database, accessing superglobals, validating input, redirecting, and more.

-
Just because your code is encapsulated in classes does not make it OOP (if that was your intent). You have pretty much just wrapped procedural code into a class.

-
I would try to keep the logic in the constructors to a minimum.

-
No useful comments; they are stating the obvious. Do you really find that the comment //create or read session is necessary for session_start()? Or // This gets execute whenever an object from this class is created for __construct()? I think we both agree that they are self-explanatory.

I would abstract all your code into different classes to give each it's own responsibility. Then, with some type of controller, you could make them all work together (by utilizing dependency injection) to accomplish any goal (registering users, logging in, etc).

You might be interested in the MVC pattern as a starting point to guide you further in this abstraction process. It enforces business logic separation from UI logic. Your classes have traits of controllers of this pattern.

As for the rest, I think your code looks pretty clean.

Context

StackExchange Code Review Q#61516, answer score: 3

Revisions (0)

No revisions yet.