• 1

• 3k
• 13
• 17

Instead of reviewing your code, I took the liberty of reviewing your lack of design. And don't take this personally ;)

#Don't just slap your keyboard A big mistake a lot of developmers make, is to simply start coding. They tap tap tap characters to a file, use the word class and ship it. This is ok - they think - because at the end of they day, it works. Everyone happy.

##single responsibility The code that is created, slapping away at the keyboard, is however crap piled on top of crap. A very good indication of this crap are things like:

class User extends Database

Where in the design, did a guy decide that a User is a Database? The answer often is:

What design? We don't need a design...

And there is the real problem of this code. It is not bade code if you read line per line. But the overall design is just missing.

Every little piece of code you write should have One Single Responsibility. The first rule of SOLID.

##Analysing what you have You wrote 2 class: User and Database. Where User is a special case of a Database. Like a Dog extends Animal.

Your Database class does the following things:

• Keep track of the DB settings
• Create a PDO object
• Handle PDO errors
• Generate SQL queries
• Interact with the PDO object to execute the generated SQL query

Your User class extends the Database class but instead of using the methods it inherited, it construct a new Database object everytime it has to do something.

If look at the User class, it actually is not a User class. It is some kind of authentication handler that authenticates a User over HTTP POST that interacts with the $_SESSION

Again, a lot

##Requirements change Just image the following scenarios and how they would affect your code

• The host does not have PDO installed and you need to use mysqli as driver
• You want to switch to a file-storage with serialized data instead of a Database
• Your session-data should be stored in a database
• All data in the user table needs to be encrypted

These are all scenarios that can happen, and scenarios that should not require a lot of extra work. If the design is good.

##Implementing interfaces When programming, start by making everything private. Because once you make it public, it should stay that way, and it can no longer change. Or you will have a lovely weekend refractoring your code and a happy man/wife at home.

Another approach, the one I prefer is a little more strict. It uses interfaces and typehinting.

First, we start by defining a neat little interface called Storage that defines how our code will interact with Storage. We don't care (and shouldn't) where the data is stored, is the file-system, a database, ftp, ...

interface Storage {
 /**
 * Select the give $fields from the given $resource with optional $filters
 * @param {String} $resource
 * @param {Array} $fields
 * @param {Array} $filters
 * @return {Array} An array of array-results
 */
 public function select($resource, $fields, $filters=array());
 /**
 * Insert a single row into the resource
 * @param {String} $resource
 * @param {Array} $data
 * @return {void}
 * @throws DatabaseInsertException
 */
 public function insert($resource, $data);
}

Now our UserAuthentication class can use this interface:

class UserAuthenticator {
 public function __construct(Storage $storage) {
 $this->storage = $storage;
 }
 /**
 * Trys to authenticate the given username & password
 * Returns true on success, false otherwise
 * @param {String} $username
 * @param {String} $password
 * @return {Boolean:false} on failure
 * @return {int} the user.id
 */
 public function authenticate($username, $password) {
 $users = $this->storage->select(
 'user',
 array('id','password'),
 array('username' => $username)
 );
 if ( count($users) !== 1) {
 return false;
 }
 if (password_verify($password, $users[0]['password'])) {
 return $users[0]['id'];
 }
 }
}

We would use as follow:

$authenticator = new UserAuthenticator($storageObject);
if ( false !== ($userid = $authenticator->authenticate($username, $password)) ) {
 $session->set('user',$userid);
}
//somewhere else
$session->get('u')

Our session interface would thus look like

interface Session {
 public function set($key, $val);
}

A very simple session class would look like this:

class SimpleSession implements Session {
 private $data;
 public function __construct() {
 session_start();
 $this->data = $_SESSION;
 }
 public function set($key, $val) {
 $this->data[$key] = $val;
 }
 public function get($key) {
 return isset($this->data[$key]) ? $this->data[$key] : null;
 }
 public function __destruct() {
 $_SESSION = $this->data;
 }
}

I'll leave the implementation of the PDOStorageAdapter to you ;)