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

Comments in PHP authentication system

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

Problem

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:

  • Is my code properly commented? Should I add more comments?



  • Is it understandable as it stands?



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



  • 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[] = 'Some fields are empty';

} else {

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

if($credentials['passwordNew'] == $credentials['passwordCurrent']

Solution

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.

Code Snippets

/**
* @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)){

Context

StackExchange Code Review Q#63701, answer score: 11

Revisions (0)

No revisions yet.