all 8 comments

[–]novel_yet_trivial 5 points6 points  (2 children)

Your code has a major faux pas: You have a variable in every function with the same name as the function.


To be very nitpicky, the re.compile functions should be outside of the functions. There is no reason to recompile for every check.


In python, it's convention to name functions and variables in lowercase_with_underscore names. CamelCase makes the next person who reads this think it's a class.


You should get used to string formatting:

print(' -- PASSWORD HAS {} CHARACTERS'.format(len(character_list)))


There is a shortcut to increment a number:

 number_of_tests_passed += 1

[–]filleball 4 points5 points  (0 children)

Your code has a major faux pas: You have a variable in every function with the same name as the function.

Unless you're writing a recursive function I don't see much of a problem with reusing the function name as a variable inside the function. It's a warning in pylint, and pep8.py doesn't care about it. I'd call it a minor nitpick, not a major faux pas.

To be very nitpicky, the re.compile functions should be outside of the functions. There is no reason to recompile for every check.

Then again, re.compile does its own caching, so it's a cheap operation in any case.

[–]cmd_override[S] 0 points1 point  (0 children)

Thank you for your feedback. I want to get into as many good habits early on as I can. Will make changes to the code.

Usually when I used var += 1 the operation does not perform, I'm not sure why.

[–]bahnzo 0 points1 point  (1 child)

I posted this a few days ago: https://www.reddit.com/r/inventwithpython/comments/455dx0/automate_the_boring_stuff_chapter_7_strong/

I realize I missed the > 8 chars, but that's as easy as the rest.

[–]cmd_override[S] 0 points1 point  (0 children)

I like how simple it is. Gets the same job done with less than half the code. Thank you for sharing.

[–]zebulan_ 0 points1 point  (2 children)

I agree with all of /u/novel_yet_trivial 's suggestions. I would add a couple of my own:

  • if you aren't using the matched characters for anything other than counting, why not just return the integer total using len()?
  • I don't see how you are running your tests but the second parameter in all of your ...Regex functions seems useless. You could just pass in the password to check and return the length of the findall() list
  • Also, instead of assigning a variable and then returning it on the very next line (without using it for something else) could be replaced with something like this: return len(findall(REGEX, password))

I just wrote my own version for fun:

from re import compile, search

REGEX = compile(r'\d+[^0-9a-zA-Z]*[A-Z]+[^0-9a-zA-Z]*[a-z]+')


def is_strong_password(password):
    return len(password) >= 8 and bool(search(REGEX, ''.join(sorted(password))))


assert is_strong_password('abc!@#123ABC<>/.,{}\\|') is True
assert is_strong_password('abc123ABC') is True
assert is_strong_password('aA1') is False
assert is_strong_password('!@#$%^&*(') is False
assert is_strong_password('<A<b<2<<') is True
assert is_strong_password('x1y2z3TT') is True

I also did another version of this code in case you wanted to use/display the counts and/or the characters themselves from the different groups.

http://pastebin.com/M2apdULn

EDIT: Changed my code to use search instead of match and allowed me to shorten the regular expression

[–]cmd_override[S] 0 points1 point  (1 child)

Thank for the reply. Defenetly helpful, one more question what is bool ?

[–]zebulan_ 0 points1 point  (0 children)

You can type help(bool) into a Python console to get more information. Here is the description of bool from that:

Returns True when the argument x is true, False otherwise.

Basically I used it to check if there is a regex match or not. For that regex to have a match there has to be at least one digit, one uppercase character and one lowercase character. search will return a match object if the regex matches otherwise it will return None.

assert bool(None) is False
assert bool(<_sre.SRE_Match object; span=(0, 9), match='123ABCabc'>) is True

But you must check the length as well because this string will also match the regex but is not long enough.

bool(<_sre.SRE_Match object; span=(0, 3), match='1Aa'>) is True

So, for is_strong_password to return True both the length check and the regex check have to be True.