Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

I would like to optimise this code. It seems a bit verbose, particularly with the 'elseif' that doesn't do anything.

This code either:

  • takes a plain text password, generates a salt, and returns them both OR
  • takes a plain text password and a salt with which to encrypt the password, and returns them both (even though the salt has already been supplied).

Thanks for your help!

class Member extends ActiveRecord\Model {

private function process_password($password, $salt = '') {
    /**
     * Process a password to encrypt it for storage or verification.
     * @param string - plain text password for processing.
     * @access private
     * @return array Containing the encrypted password and the salt.
     */

    //Check if the salt has been supplied. If not, generate one.
    if (!$salt) {
        $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
    } elseif ($salt && strlen($salt) == 64)  {
        //Do nothing, the salt has already been supplied here.
    } else {
        log_message('info', 'Supplied password to process_password() was not the correct 64-byte length.');
        return false;
    }

    $hashed_password = hash('sha256', $password.$salt);

    return array('password' => $hashed_password, 'salt' => $salt);

}
}
share|improve this question
If you want to get rid of that empty elseif, just change it to strlen($salt) != 64 and put the else code in the elseif block. – DaveRandom Feb 22 '12 at 18:18
Your code looks like already optimized! Is it working? If so why change? – B4NZ41 Feb 22 '12 at 18:21
@B4NZ41 I've tried my best to optimise it, but I'm trying to learn to code a bit more 'profesionally'. Particularly by getting rid of the extra elseif! :) – Kieran Feb 22 '12 at 18:27
1  
I'm a big fan of removing nesting whenever possible. See this for more information. Bad formatting since I can't post an answer on closed questions, so here's a pastebin. – Paul DelRe Feb 22 '12 at 18:27
Please disregard the pastebin as it has an error. I am still a fan of reducing nesting, but I no longer think it makes a difference here. – Paul DelRe Feb 23 '12 at 16:31

migrated from stackoverflow.com Feb 22 '12 at 18:31

2 Answers

up vote 4 down vote accepted

How about the following:

if (!$salt) {
    $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
} elseif (strlen($salt) !== 64)  {
    log_message('info', 'Supplied password to process_password() was not the correct 64-byte length.');
    return false;
}
// if we're here, we have a valid salt

Improvements

  • Removed $salt from second condition (already known to be TRUE)
  • Flipped length logic, moved else code into second block
  • Removed empty condition block
share|improve this answer
up vote 2 down vote

You could reduce your if/elseif/else to:

if (!$salt) {
    $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
} elseif (strlen($salt) !== 64)  {
    log_message('info', 'Supplied password to process_password() was not the correct 64-byte length.');
    return false;
}

You already know that $salt is truthy since you check in the if part, so all you care about is that it's valid (length 64).

A way to further improve this would be to use a standard password hashing library instead of using PHP's built in hash function. For example, yours has a major potential problem: It's way too fast. With enough hardware, an attacker could just brute force any short passwords.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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