2
\$\begingroup\$

I'm working on the registration process for new users of a small web app, and as this is largely a learning project I'm doing it from scratch.

Here is my user class: Class_user.php

<?php
    class User {
        // The class variables are the same and have the same name as the db fields.
        private $userID; // Must be incremental + unique.  Also used as salt.
        private $userName; // Must be unique.
        private $hashedPassword;
        private $userEmail;

        function __construct($userID, $userName, $hashedPassword, $userEmail){
            $this->userID=$userID;
            $this->userName=$userName;
            $this->hashedPassword=$hashedPassword;
            $this->userEmail=$userEmail;
        }
        function getUserID(){
            return $this->userID;
        }
        function getUserName() {
            return $this->userName;
        }
        function getHashedPassword() {
            return $this->hashedPassword;
        }
        function getUserEmail() {
            return $this->userEmail;
        }
    }
?>

Here is my model for the user - Model_User.php:

<?php
    require_once('Class_DB.php');
    require_once('Class_User.php');
    require_once('DAO_User.php');

    class Model_user{
    // Object that represents a connection to the user DB
        private $dbInstance;

        function __construct($dbInstance){ // Using Dependency Injection passing a Class_DB object rather than global variable.
            $this->dbInstance=$dbInstance;
        }
        function insertNewUser($user, $userPassword){ // INCOMPLETE
            $userDAO=new DAO_User($this->dbInstance);
            $insertedUser=$userDAO->createNewUser($user, $userPassword);
            $userPassword=""; // We clear the user's password from memory.
            if ($insertedUser){ // User was correctly inserted in db
                return true; // Should return the $user object.
            } else {
                return false;
            }
        }
    }
?>

Here is the key snippet from my user registration controller, Controller_Register.php

<?php
    require_once("Controller.php");
    require_once("Model_User.php");
    require_once("General.php");

    class Controller_Register extends Controller {
        protected $page='UI_Register.php';
        function execute($view){
         // Several lines to make sure the form is filled adequately.
            While (!$userInserted){ // As long as the user isn't correctly inserted...
                $userName=$_POST["userName"];
                $userEmail=$_POST["userEmail"];
                $userPassword=$_POST["userPassword"];
                $_POST["userPassword"]=""; // We don't keep the pw in memory.
                $user=new user("", $userName, "", $userEmail); // User ID will be generated by the db, and the hashed pw has not been generated at this point.
                $userInserted=$userDBConnection->insertNewUser($user,$userPassword); // We insert the user name not knowing what the autoincremented user ID is.
                $userPassword=""; // We don't keep the pw in memory.
                }
            $_POST["password"]=""; // We clear the user's password from memory.
            if ($userInserted){// The value is true if the registration was succesful.
            $msg=new Message("Congratulations ".$_POST['userName']."! You are now registered."); 
            }
        }
    return $view;
    }
?>

Finally, here is my user DAO code, with the key item I would like guidance on, the registration - DAO_User.php

<?php
    require_once('Class_User.php');
    require_once('Class_DB.php');
    require_once('General.php');

    class DAO_User {
        private $dbInstance; // This is an instance of Class_DB to be injected in the functions.

        function __construct($dbInstance){
            $this->dbInstance=$dbInstance; // Using Dependency Injection passing a Class_DB object rather than global variable.
        }

        function createNewUser($user, $userPassword){ // The $user object only has a userName and a userEmail at this point.
            $dbConnection=$this->dbInstance->createConnexion(); // This connection is local, so automatically closed after the function is completed.
            $inserted=false;
            while (!$inserted){ // This insert a new user, without any value for pw, and generates an autoincrement user ID on the db side.
                $query=$dbConnection->prepare("INSERT INTO users (userName, userEmail) VALUES (?,?)"); // userID is generated via autoincrement - therefore not known at time of insertion.
                $query->bindValue(1, $user->userName);
                $query->bindValue(2, $user->userEmail);
                $inserted=$query->execute(); //True if succesful, False if not.
            }
            $query=$dbConnection->prepare("SELECT LAST_INSERT_ID()"); // This allows us to retrieve the user ID as generated by the db.
            $userIDquery=$query->execute();
            $result=$userIDquery->fetch(PDO::FETCH_ASSOC); // returns an array indexed by column name as returned in result set - here column name is "userID" in the DB
            $userID=$result["userID"];
            $user->userID=$userID; // We modify the user ID of the $user object to be the autoincremented number generated by the db.
            $hashedPassword=stringHashing($userPassword,$user->userID);
            $userPassword="";
            $user->hashedPassword=hashedPassword;
            $hashedPWinserted=false;
            while (!$hashedPWinserted){ // This modifies the user table in db to add hashed PW.
                $query=$dbConnection->prepare("UPDATE users SET hashedPassword=? WHERE userID=?");
                $query->bindValue(1, $user->hashedPassword);
                $query->bindValue(2, $user->userID);
            }
            return $user;
        }
    }
?>

The General.php code contains the stringHashing function that receives a string and a salt as parameter and and returns the salted string. I'm not sure where it should live in an MVC framework.

My users table is a MySQL table with 4 fields:
- userID (INT(10), not null, autoincrement, PK) - also used as a salt for PW hashing
- userName (varchar(50), not null)
- hashedPassword (char(128), can be null)
- userEmail (varchar(255), can be null)

Some specific questions I have, mostly on the createNewUser function:
- Are the db transactions correctly and efficiently done?
- Should I split some functionalities outside of this function?
- Should I limit the use of intermediate variables?
- Does the function accomplish the goals I want it to do, i.e., insert in the db a new user with an autoincremented user ID generated by the db, and then a hashed password?

Also interested in any other piece of feedback people may have, especially with regards to readability (e.g., are my commentaries too verbose and obvious) and understandability of my code, as well as best practice object programming (e.g., I suppose I should modify my $user object with a setter rather than $user->userID=$userID;).

UPDATE When I run this code, I get no error, but I also don't get any record in the db.

\$\endgroup\$
2
\$\begingroup\$

In your User class : add the setters !
You cannot modify a value from outside the class (ex: $user->userID = $userId)
Try it : http://ideone.com/We2Lh

At the end of Model_User.php, replace

if ($insertedUser){ // User was correctly inserted in db
     return true; // Should return the $user object.
} else {
    return false;
}

By

return $insertedUser;

In yout Controller :

Remove the While (!$userInserted) {. What will appened if the insertNewUser method always fail ? an infinite loop, an maybe you can crash your server.

My way to write this code :

function execute($view) {

    // check and sanitize values
    $user = new user($userName, $userEmail); // Create a specific constructor. Empty values are ...
    $userInserted=$userDBConnection->insertNewUser($user,$userPassword);

    if ($userInserted) {
        $msg = new Message("Congratulations ".$_POST['userName']."! You are now registered.");
    } else {
        $msg = new Message("Erreur during subscription. Please retry.");
    }

    // send $msg to the view
    // ...
}

If there is an error, it's not necessary to retry immediatly. Errors won't miraculously fix themselves !

$_POST["password"]=""; : Useless, the parameters will be reset at the next request.

Your DAO is very complicated.

Remove all while, and handle possible errors (you can use boolean, exceptions ...).
You can use two private methods to create and set the password.

Generally, yout code is too much commented for me.
Exemple :

private $userName; // Must be unique.
=> Check this in your DB, a comment is useless

Don't write obvious comment :

if ($insertedUser){ // User was correctly inserted in db

Don't try to explain the How or the What, comment the Why. The 'How' and the 'What' could be understand by reading your code, but you can't undestand the objective of a snippet if it's complicated.

Add an unique index to userName.

Finally, think of aerating your code ! New lines are free ^^

\$\endgroup\$
0
\$\begingroup\$

Too much code for a simple thing...why don't you consider working with a framework like CodeIgniter?

Try placing some echoes on your code and see where it is stopping. It will be easier to help you.

\$\endgroup\$
  • 2
    \$\begingroup\$ "this is largely a learning project I'm doing it from scratch" \$\endgroup\$ – Ryan Ore Feb 12 '13 at 14:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.