Comments in PHP authentication system

Question

Leaving aside absolutely everything about the code itself (but if you find stuff that's not ok feel free to comment on them).

What I'm asking here is the following:

1. Is my code properly commented? Should I add more comments?
2. Is it understandable as it stands?
3. Are there some general rules that I should adhere to when commenting?
4. Could my comments be improved in any way?

I'll leave a few classes I've been working on.

class ChangePassword:

class ChangePassword{

    private $_errors = array();

    public function __construct(){

        require_once 'PasswordHash.php';
        require_once 'ValidateData.php';
        require_once 'SqlQueryController.php';

        if(isset($_POST['resetPassword'])){
            /**
            * @param associative array
            * stripAllWhiteSpaces will remove ALL white spaces.
            * example: $stringBefore = ' this is an example';
            *          $stringAfter  = 'thisisanexample';
            */
            $credentials = ValidateData::stripAllWhiteSpaces(array('passwordCurrent' => $_POST['passwordCurrent'],
                                                                   'passwordNew'     => $_POST['passwordNew'],
                                                                   'passwordNewAgain'=> $_POST['passwordNewAgain']
                                                                  )
                                                            );
            $this->doResetPassword($credentials);
        } 
    }

    public function doResetPassword($credentials){
        /**
        * @bool  returns true if value is empty
        */
        if(ValidateData::isEmpty($credentials)){

            $_errors[] = '<p>Some fields are empty</p>';  

        } else {

            if($credentials['passwordNew'] != $credentials['passwordNewAgain']){
                $_errors[] = '<p>Passwords do not match.</p>';
            }   

            if($credentials['passwordNew'] == $credentials['passwordCurrent']){
                $_errors[] = '<p>Your new password cannot be the same as your old password.</p>';
            } 

            /**
            * @bool
            * Example of valid password: Thequickbrown200!
            */
            if( ! ValidateData::pregMatch('/^(?=.*\d)(?=.*[A-Za-z])[0-9A-Za-z!@#$%]{5,200}$/', $credentials['passwordNew'])){
                $_errors[] = '<p>The password must be between 5 and 200 characters long, must contain at least one number, at least one letter and at least one non Alphanumeric character.</p>';
            }

        }

        if( ! empty($_errors)){
                foreach($_errors as $error){
                    echo $error;
            }
            return;
        }

        $this->insertPassword($credentials);
        $_SESSION = array();
        session_destroy();
    }

    public function insertPassword($credentials){

        $passwordHash = new PasswordHash();

        $hashedPassword = $passwordHash->hashPassword($credentials['passwordNew']);

        $usernameId = $_SESSION['id'];

        $sqlQueryController = new SqlQueryController();

        $query = "UPDATE users_table
                 SET login_password=:passwordNew 
                 WHERE login_id=:usernameId";

        $array = array(':passwordNew' => $hashedPassword,
                       ':usernameId'  => $usernameId);

        if($sqlQueryController->runQueryExecute($query, $array)){
            echo '<p>Successfully changed the password</p>';
        } else {
            echo '<p>An error occurred while changing the password</p>';
        }

    }

}

class Login:

<?php

class Login{
    public function __construct(){

        require_once 'PasswordHash.php';
        require_once 'SqlQueryController.php';
        require_once 'ValidateData.php';

        if(isset($_POST['login'])){
            /**
            * @param associative array
            * stripAllWhiteSpaces will remove ALL white spaces.
            * example: $stringBefore = ' this is an example';
            *          $stringAfter  = 'thisisanexample';
            */
            $credentials = ValidateData::stripAllWhiteSpaces(array('username' => $_POST['username'], 
                                                                   'password' => $_POST['password']
                                                                  )
                                                            );

            $this->doLogin($credentials);
        }
    }

    /**
    * Log in with post data
    */
    public function doLogin($credentials){
        if( ! ValidateData::isEmpty($credentials)){

            $passwordHash = new PasswordHash();

            $sqlQueryController = new SqlQueryController();

            $query = "SELECT login_password 
                      FROM users_table 
                      WHERE login_username=:username 
                      LIMIT 1";

            $array = array(':username' => $credentials['username']);
            /**
            * @param associative array
            * return an associative array using PDO's fetch();
            */
            $hash = $sqlQueryController->runQueryFetch($query, $array);

            /**
            * @bool
            * verifies password based on the $hash
            * and the password provided by the user
            */
            $passwordVerify = $passwordHash->verifyPassword($credentials['password'], $hash['login_password']);

            $query = "SELECT login_username, login_id
                      FROM users_table
                      WHERE login_username=:username LIMIT 1";

            $array = array(':username' => $credentials['username']);

            $userVerify = $sqlQueryController->runQueryFetch($query, $array);

            if(($passwordVerify == 1) && ($userVerify['login_username'] == $credentials['username'])){

                /**
                * Great, the user's logged in
                * Time to set the session and redirect him
                */
                $_SESSION['id']       = $userVerify['login_id'];
                $_SESSION['username'] = $userVerify['login_username'];

                #session_write_close();
                header('Location: logged_in.php');
                die();
            } else {
                echo '<p> The username or password do not match any registered users.</p>';
            }
        } else {
            echo '<p> You must fill in all fields.</p>';
        }       
    }
}

class RecoverPassword:

<?php

class RecoverPassword{

    public function __construct(){

        require_once 'PasswordHash.php';
        require_once 'SendMailRecoverPassword.php';
        require_once 'ValidateData.php';
        require_once 'SqlQueryController.php';

        if(isset($_POST['recoverPassword'])){
            /**
            * @param associative array
            * stripAllWhiteSpaces will remove ALL white spaces.
            * example: $stringBefore = ' this is an example';
            *          $stringAfter  = 'thisisanexample';
            */
            $credentials = ValidateData::stripAllWhiteSpaces(array('email' => $_POST['email']
                                                                   )
                                                            );
            $this->doRecoverPassword($credentials);
        }
    }

    public function doRecoverPassword($credentials){

        if(ValidateData::validateEmail($credentials['email'])){

            $sqlQueryController = new SqlQueryController();

            $query = "SELECT login_email
                      FROM users_table
                      WHERE login_email=:email LIMIT 1";
            $array = array(':email' => $credentials['email']);

            $emailExist = $sqlQueryController->runQueryFetchAssoc($query, $array);

            if($emailExist){

                /**
                * If a proper SMTP is not configured, the password
                * will not be changed and the page will die
                * with a user friendly error.
                * A more useful error can be found in the log fiels
                * The __construct() of the class is built
                * in such a way that it will throw the exception and die
                * after. Point is, don't move this further down the page
                * or the password WILL be changed but the email will NOT
                * be sent if the SMTP is not configured!
                */
                $swift = new SendMailRecoverPassword();

                $passwordHash = new PasswordHash();

                /*
                * Create a random string of letteres and numbers
                * This will be the users new password
                */
                $randomPassword = str_shuffle('abcdefghijklmnopqrstqwxz0123456789ABCDEFGHIJKLMNOPQRSTWXZ');

                /**
                * Hash the random string
                */
                $newPassword = $passwordHash->hashPassword($randomPassword);

                /**
                * Update the new hashed password
                * replacing the old password
                */
                $query = "UPDATE users_table
                          SET login_password=:password
                          WHERE login_email=:email LIMIT 1";
                $array = array(':password' => $newPassword,
                               ':email'    => $credentials['email']);

                $sqlQueryController->runQueryExecute($query, $array);

                /**
                * Create the message
                */
                $swift->createMessage($randomPassword, $credentials['email']);

                /**
                * Return the newly created message
                */
                $message = $swift->getMessage();

                /**
                * Send the message
                */
                if($swift->sendMessage($message)){
                    echo '<p>Check your inbox for the new password. Your old password will no longer work</p>';
                }

            } else {
                echo '<p>Email doesn\'t exist!</p>';
            }

        } else {
            echo '<p>Email is invalid</p>';
        }

    }
}

Answer 1

Is my code properly commented?

No. You're documenting methods where they are used (usually multiple places) as opposed to where they are declared (inherently only one place):

/**
* @param associative array
* stripAllWhiteSpaces will remove ALL white spaces.
* example: $stringBefore = ' this is an example';
*          $stringAfter  = 'thisisanexample';
*/
$credentials = ValidateData::stripAllWhiteSpaces(.....

/**
* @bool  returns true if value is empty
*/
if(ValidateData::isEmpty($credentials)){

This is not how it's supposed to be. At all. You're supposed to comment methods where they are declared. Think about it, are you going to copy-paste these comments everywhere the methods are used? That would be silly (yet, you did it, for stripAllWhiteSpaces in 3 places).

---

Should I add more comments?

If you want your code to be well-documented, then comment every public method.

---

Is it understandable as it stands?

The duplicated comments on method calls instead of declarations is noise, disturbing.

---

Are there some general rules that I should adhere to when commenting?

I recommend the advice of this guy:

http://blog.codinghorror.com/coding-without-comments/

At all those places where your comment is exactly the same as the method name, I would drop the comments. For example, saying that stripAllWhiteSpaces "strips all white spaces" is clearly pointless. Don't add comments for the sake of adding comments.

Answer 2

A comment usually implies "Sorry the code is not clear. This is what I really meant".

Don't add more comments. In fact, I'd recommend to remove some (if not all), for example these three just clutter the screen and make the code actually harder to read:

            /**
            * Create the message
            */
            $swift->createMessage($randomPassword, $credentials['email']);

            /**
            * Return the newly created message
            */
            $message = $swift->getMessage();

            /**
            * Send the message
            */
            if($swift->sendMessage($message)){
                echo '<p>Check your inbox for the new password. Your old password will no longer work</p>';
            }

Answer 3

I'll just focus on RecoverPassword.

You have a class whose constructor takes no parameters, and the constructor calls the only method. So, instantiating a RecoverPassword object has the side-effect of sending the password-resetting e-mail for the user named in the email POST parameter. In fact, once you instantiate a RecoverPassword object, would you ever call $recoverPassword->doRecoverPassword(…) on it? The usage pattern is highly unusual, and it suggests that there really isn't much reason to make the it an object at all.

I am suspicious of the four files included in the constructor. Incorporating code like that makes it hard to tell what is going on. The require_once statements should be at the top of the file. Any side effects should be explicit — for example, you should call validateData($_POST), if that's what you want to do.

In recoverPassword(), there is one goal, with many conditions that could cause it to fail prematurely. Therefore, the flow would be more readable if you inverted the conditions. (You failed to check $swift->sendMessage() for errors, so it could fail mysteriously with no indication.)

public static function doRecoverPassword($credentials) {
    if (!ValidateData::validateEmail(…)) {
        echo '<p>Email is invalid</p>';
        return;
    }
    $sqlQueryController = …;
    …
    if (!$emailExist) {
        echo "<p>Email doesn't exist!</p>";
        return;
    }
    …
    if (!$swift->sendMessage($message)) {
        echo '<p>Failed to send message</p>';
        return;
    }
    echo '<p>Check your inbox…</p>';
}