5
\$\begingroup\$

The following method is designed to return true if it passes all of the rules for a password. Does anyone see a way to improve it? Performance improvements are welcome for sake of education.

I already know password rules beside length are counter-productive. It's not my choice. Also, I know that performance is absolutely trivial here. I only care about performance by way of educating myself. The stuff I learn on non-critical optimization often helps me when I do have a bottleneck. I will not be implementing something just because it is faster.

/**
 * Returns true if and only if a password passes the rules:
 *  - Must be at least 8 characters.
 *  - Must contain two of the following types:
 *    - Letters 
 *    - Numbers
 *    - Symbols (includes whitespace)
 * 
 * @param string $password The raw, unencrypted password.
 * @return bool 
 */
public function isValidPassword($password) {
    $length = strlen($password);

    if ($length < 8) {
        return false;
    }

    $foundLetter = false;
    $foundNumber = false;
    $foundSymbol = false;

    for ($i = 0; $i < $length; $i++) {
        $char = $length[$i];

        if (ctype_alpha($char)) {
            $foundLetter = true;
        }
        else if (ctype_digit($char)) {
            $foundNumber = true;
        }
        else if (ctype_punct($char) || ctype_space($char)) {
            $foundSymbol = true;
        }
    }

    return ($foundLetter && $foundNumber)
        || ($foundLetter && $foundSymbol)
        || ($foundNumber && $foundSymbol);
}
\$\endgroup\$
  • \$\begingroup\$ a1111111 will pass, is this the intention? \$\endgroup\$ – Andrew T Finnell Jan 26 '12 at 20:09
  • 1
    \$\begingroup\$ Password rules like this (apart from min length) are stupid and counter productive: xkcd.com/936 \$\endgroup\$ – Martin York Jan 26 '12 at 20:55
  • \$\begingroup\$ @LokiAstari I knew I should have mentioned that I didn't create the rules. In fact I was able to get rid of two of them. \$\endgroup\$ – Levi Morrison Jan 26 '12 at 21:04
4
\$\begingroup\$

Your solution does not honor multibyte characters, i.e. it will probably break for non-English languages.

Instead you could utilize PCRE (regular expressions) and Unicode character grounps:

$foundLetter = preg_match('(\pL)u', $password);
$foundNumber = preg_match('(\pN)u', $password);
$foundSymbol = preg_match('(???)u', $password);

Not sure what you define as a symbol, [\pP\p{Xps}] (Punctuation + Whitespace) would reflect your current version (I think). Maybe a better alternative would be [^\pL\pN] though, which just ensures one non alphanumeric letter.

In theory you could change your script into a one liner now:

return strlen($password) >= 8 && (
    preg_match('(\pL)u', $password)
  + preg_match('(\pN)u', $password)
  + preg_match('([^\pL\pN])u', $password)
) >= 2;
\$\endgroup\$
  • 2
    \$\begingroup\$ This is the kind of feedback I'd like to see. Thanks, NikiC. \$\endgroup\$ – Levi Morrison Jan 26 '12 at 21:20
  • \$\begingroup\$ Or an even shorter single-call 1-liner: return preg_match('^(?.*(\pL)u)(?=.*(\pN)u)(?=.*([^\pL\pN])u).{8,}', $password) \$\endgroup\$ – Bohemian Feb 13 '18 at 5:47
2
\$\begingroup\$

Should this:

    else if (ctype_digit($char)) {
        $foundLetter = true;

Not be:

    else if (ctype_digit($char)) {
        $foundNumber = true;
\$\endgroup\$
  • 1
    \$\begingroup\$ I believe that's merely a copy-paste error, which is why I tried to edit it; this isn't relevant to his question. \$\endgroup\$ – seand Jan 26 '12 at 20:01
  • \$\begingroup\$ @seand What do you base that believe on? I can't think of a likely scenario where such an error would be introduced by copy and pasting (though that might just be my lack of imagination, of course). I see no reason to believe that this mistake isn't present in the original code and the OP just didn't test the function extensively. \$\endgroup\$ – sepp2k Jan 26 '12 at 20:24
  • \$\begingroup\$ @sepp2k: If I were writing the same code, I would have copied those statements over because they follow a pattern of $foundX = true; and just replaced X. It probably is an error in the original code but that doesn't change the fact that it's not what he's asking, so it shouldn't be an answer, but rather an edit + comment on the question. \$\endgroup\$ – seand Jan 26 '12 at 20:40
  • 1
    \$\begingroup\$ @seand No, it's not. "Code correctness" is actually the first item on the list of on-topic items in the FAQ. What's off topic is posting code you know is wrong and ask people to find your bug. If someone posts code that he believes to be correct and an answerer points out a mistake the OP overlooked, that's perfectly on topic. \$\endgroup\$ – sepp2k Jan 26 '12 at 20:49
  • 1
    \$\begingroup\$ It was merely a copy error. I do appreciate your time, though. \$\endgroup\$ – Levi Morrison Jan 26 '12 at 21:04
2
\$\begingroup\$

Does performance really matter? It seems premature optimization.

Actually, I'd rewrite the last part to a more readable form:

$classCount = 0;
if ($foundLetter) {
    $classCount++;
}
if ($foundNumber) {
    $classCount++;
}
if ($foundSymbol) {
    $classCount++;
}

if ($classCount >= 2) {
    return true;
}
return false;

It would be more important and less error-prone if you have more than three character classes.

\$\endgroup\$
  • \$\begingroup\$ No, performance does not matter. For the love of everything good and sane, I said, "Performance improvements are welcome for sake of education." \$\endgroup\$ – Levi Morrison Jan 26 '12 at 21:03
  • \$\begingroup\$ I am not an idiot. This is in NO way a bottleneck. That doesn't mean its performance cannot be improved. I ask for performance improvements for my education only. That doesn't mean I'm going to implement a faster performing code just because its faster. \$\endgroup\$ – Levi Morrison Jan 26 '12 at 21:10

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.