Skip to main content
added 892 characters in body
Source Link
user6208
user6208

A little refactoringIn 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 ^^

A little refactoring :

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 !

Your DAO is very complicated.

Remove all while, and handle possible errors (you can use boolean, exceptions ...).

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 ^^

Source Link
user6208
user6208

A little refactoring :

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 !

Your DAO is very complicated.

Remove all while, and handle possible errors (you can use boolean, exceptions ...).