The following method is designed to returntrue 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);}
- \$\begingroup\$a1111111 will pass, is this the intention?\$\endgroup\$Andrew T Finnell– Andrew T Finnell2012-01-26 20:09:58 +00:00CommentedJan 26, 2012 at 20:09
- 1\$\begingroup\$Password rules like this (apart from min length) are stupid and counter productive:xkcd.com/936\$\endgroup\$Loki Astari– Loki Astari2012-01-26 20:55:28 +00:00CommentedJan 26, 2012 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– Levi Morrison2012-01-26 21:04:07 +00:00CommentedJan 26, 2012 at 21:04
3 Answers3
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;
- 2\$\begingroup\$This is the kind of feedback I'd like to see. Thanks, NikiC.\$\endgroup\$Levi Morrison– Levi Morrison2012-01-26 21:20:57 +00:00CommentedJan 26, 2012 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– Bohemian2018-02-13 05:47:43 +00:00CommentedFeb 13, 2018 at 5:47
Should this:
else if (ctype_digit($char)) { $foundLetter = true;Not be:
else if (ctype_digit($char)) { $foundNumber = true;
- 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– seand2012-01-26 20:01:18 +00:00CommentedJan 26, 2012 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– sepp2k2012-01-26 20:24:20 +00:00CommentedJan 26, 2012 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 replacedX. 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– seand2012-01-26 20:40:16 +00:00CommentedJan 26, 2012 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– sepp2k2012-01-26 20:49:26 +00:00CommentedJan 26, 2012 at 20:49
- 1\$\begingroup\$It was merely a copy error. I do appreciate your time, though.\$\endgroup\$Levi Morrison– Levi Morrison2012-01-26 21:04:51 +00:00CommentedJan 26, 2012 at 21:04
Does performance really matter? It seemspremature 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.
- \$\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– Levi Morrison2012-01-26 21:03:44 +00:00CommentedJan 26, 2012 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– Levi Morrison2012-01-26 21:10:54 +00:00CommentedJan 26, 2012 at 21:10
You mustlog in to answer this question.
Explore related questions
See similar questions with these tags.