7
\$\begingroup\$

I've been following a Python programming book and reached the Regex chapter where I encountered this challenge:

Write a password strength checker that checks if a password is:

  • At least 8 character long
  • Contains at least one uppercase and lowercase letter.
  • And contains at least one digit.

I managed to do so, but frankly my solution seems both long and ugly. I was hoping someone could shed some light as to how I could maybe condense my program and improve readability if possible.

import re
def is_password_strong():
    password = input('Enter a password to test: ')

    length_regex = re.compile(r'.{8,}')
    length = True if length_regex.search(password) != None else False

    uppercase_regex = re.compile(r'[A-Z]')
    lowercase_regex = re.compile(r'[a-z]')
    uppercase = True if uppercase_regex.search(password) != None else False
    lowercase = True if lowercase_regex.search(password) != None else False
    case = True if uppercase and lowercase == True else False

    digit_regex = re.compile(r'[0-9]')
    digit = True if digit_regex.search(password) != None else False

    return(length and case and digit == True)
print(is_password_strong())

And here is my non-regex solution if you can't read the above and want to see what I am trying to do with regex:

def is_password_strong():
    password = input('Enter a password to test: ')
    length = len(password) >= 8
    case = password != password.upper() and password != password.lower()
    digit = any(c.isdigit() for c in password)

    return(length and case and digit == True)

I know this isn't a good test of password strength, but it was the book's challenge. I'm not implementing it for any real use.

\$\endgroup\$
  • 3
    \$\begingroup\$ You can reduce the entire function to a single pattern: ^(?=.*[a-z])(?=.*\d)(?=.*[A-Z])([a-zA-Z\d]{8})$ \$\endgroup\$ – hjpotter92 Dec 1 '15 at 5:49
8
\$\begingroup\$

Here are the major comments:

  • Ideally functions should do one thing at once. Your function is doing two: (1) asking the user for a password and (2) evaluating the strength of that password.

    You should break this code into two functions: (1) ask_user_for_password() and (2) is_password_strong(password).

  • Regexes are not always the right tool for the job. (I appreciate this is a chapter on regexes, but the point has to be made.)

    Python has fairly powerful string manipulation tools. You should reach for those before opening your regex toolbox – I think the Python constructs are usually much easier to read and debug. Brownie points for writing a version that doesn’t use regexes – I would almost always prefer this version in a “real” codebase.

  • Be lazy about evaluation. This function is fairly fast, because it doesn’t have many checks. Even so, you can save yourself a little work – as soon as a check fails, you can return False.

    (It’s fine to bail out early for a password strength evaluation. Try to avoid doing this when doing real authentication – if your function takes noticeably different time depending on what’s wrong with the function, you’re vulnerable to a timing attack.)

  • Your function should have a docstring. Docstrings are the preferred way to document functions in Python. For this function, it should be fairly simple -- it evaluates a password, and returns True/False depending on whether it meets certain criteria -- but you should still write it.

And a few nitpicks:

  • Compare to None with “is”, not “==” and “!=”.

    It’s always preferred to check using “is”, which checks identity, not equality – this makes sure the object you’re dealing with really is None, and not a class with a strange equality operator. It’s also faster.

  • You can tidy up your booleans.

    Quite a few of your lines boil down to:

    result = True if (check != None) else False
    

    This line reduces to:

    result = (check is not None)
    

    That’s more readable and direct.

    Then your final boolean can be reduced to:

    return (length and case and digit)
    

    which is equivalent.

  • Put your main code in an if __name__ == '__main__' block. This means that it only runs when the file is called directly – not, for example, when it’s imported as a module.

    This makes your code easier to reuse.

\$\endgroup\$
6
\$\begingroup\$

In your regex solution, rather than using two temporary values, I'd find it more readable to just evaluate case directly:

case = (uppercase_regex.search(password) is not None and
        lowercase_regex.search(password) is not None)

This is both readable and makes use of short circuiting. This means that if uppercase_regex.search(password) is not None evaluates as False then Python already knows the value of case is False and it doesn't bother searching the other regex.

If you wanted, you could even employ short circuiting with all four tests in your return statement, instead of searching after each compile:

def is_password_strong():
    password = input('Enter a password to test: ')

    length_regex = re.compile(r'.{8,}')
    uppercase_regex = re.compile(r'[A-Z]')
    lowercase_regex = re.compile(r'[a-z]')
    digit_regex = re.compile(r'[0-9]')

    return (length_regex.search(password) is not None
            and uppercase_regex.search(password) is not None
            and lowercase_regex.search(password) is not None
            and digit_regex.search(password) is not None)

This now organises all your compiles to be neatly together, and then all the tests are in the return itself. This also means you can make use of short circuiting. This is a simple example, but in a more complex one you could arrange the order of tests so that the quickest ones and the ones that fail most often come first, and expensive or rarely failed ones are at the end so that you reduce the number of redundant tests.

You could do the same with your non regex example. Your mileage may vary about readability, but I think not having read regex strings makes it easier to follow:

def is_password_strong():
    password = input('Enter a password to test: ')

    return (len(password) >= 8
            and password != password.upper()
            and password != password.lower()
            and any(c.isdigit() for c in password))

Also you should have a docstring clarifying what the criteria are:

def is_password_strong():
    """Returns a boolean indicating if the password is strong

    Strong passwords are 8 characters or longer,
    contain at least one uppercase letter,
    lowercase letter and digit."""
\$\endgroup\$
  • \$\begingroup\$ Sorry, I realize this is late, but in your reorganization of the non-regex version you have an extra and between the first two conditions \$\endgroup\$ – Dannnno Jan 15 '16 at 17:44
  • \$\begingroup\$ @Dannnno Good catch! I fixed it now, thanks. \$\endgroup\$ – SuperBiasedMan Jan 15 '16 at 17:46
5
\$\begingroup\$

When seeing this question I felt like answering, but before I got around to it there were some good answers commenting upon style and efficiency already. Therefore I decided to do a performance review instead.

I made a simple function, password_tester, to run through some STRONG_PASSWORDS and WEAK_PASSWORDS, and timed it using %timeit password_tester(test_function) within IPython. I'll comment each of them whilst presenting the result, but first the specific test code:

import re

STRONG_PASSWORDS = [
    "ABCabc12",
    "abc123AB",
    "123ABCab",
    "Aa1bB2Cc",
    "abcABC123",
    "A TRUELY STRONG password in between the 6 so-called strong passwords!!!!",
    ]

WEAK_PASSWORDS = [
    "aaaa",
    "!()!=!()",
    "aaaaaaaa",
    "AAAAAAAA",
    "11121312",
    "ABCDabcd",
    "abcd1234",
    "1234ABCD",
    ]


def get_non_strong_passwords(passwords, test_function):
    """Return all passwords which are not strong."""

    return  [password for password in passwords
                      if not test_function(password)]


def get_non_weak_passwords(passwords, test_function):
    """Return all passwords which are not strong."""

    return  [password for password in passwords
                      if test_function(password)]


def password_tester(test_function, with_output=False):
    if with_output:
        print('Testing {}: '.format(test_function.__name__), end='')

    non_strong_passwords = get_non_strong_passwords(STRONG_PASSWORDS, test_function)
    non_weak_passwords = get_non_weak_passwords(WEAK_PASSWORDS, test_function)

    if not with_output:
        return False

    if not non_strong_passwords and not non_weak_passwords:
        print('All OK');
        return True

    if non_strong_passwords:
        print('  Failed on strong passwords: {}'.format(', '.join(non_strong_passwords)))

    if non_weak_passwords:
        print('  Failed on weak passwords: {}'.format(', '.join(non_strong_passwords)))

    return False

To be able to verify the correctness of various alternatives the test functions has optional printing which was turned of when doing the timing test runs. The code contains two utility functions which returns those passwords not being strong or weak when they were supposed to be the opposite. These might be kind of counter-intuitive, but they served the purpose well when testing for correctness.

Test results by version

All the different password tester are presented in the code block at the bottom, but here is the various result, with a comment:

  • is_strong_org – 40.2 µs
    Original non-regex version by original poster
  • is_strong_org_v227.8 µs, 3rd fastest
    Removed intermediate variables from original version by original poster, this removed 12 µs... This version is similar to the version presented by SuperBiasedMan
  • is_strong_regex – 103 µs
    Original regex version by original poster
  • is_strong_regex_v2 – 33.3 µs
    OP's regex version, but moved the re.compile(...) out of the loop. This reduced the running time to a third!!
  • is_strong_regex_v3 – 28.7 µs per loop
    Continued with removing the anti-pattern of True if True else False. This would be similar to the implemented version of alexwlchan's version.
  • is_strong_regex_v421.3 µs, 2nd fastest!
    And finally removed intermediate variables, and we're at a fifth of the original time used for OP's regex version
  • is_strong_holroy – 46.8 µs
    This is my version, where I'm using one regex with named capture groups, and can check for existing of each group. Not the fastest, alas.

    ALL_CASES = re.compile(r'^(?:(?P<number>\d)|(?P<lower>[a-z])|(?P<upper>[A-Z])|(?P<other>[^a-zA-Z0-9])){8,}$')
    
    def is_strong_holroy(password):
       """Version by holroy, with named capturing groups."""
    
        m = ALL_CASES.search(password)
    
        return m is not None and m.group('number') and m.group('upper') and m.group('lower')
    
  • is_strong_hjpotter9215.7 µs, Fastest!!
    The version by hjpotter92, with the modification of allowing longer than 8, and other characters rather than only digit, lower- and uppercase characters. That is the following regex:

    re.compile(r'^(?=.*[a-z])(?=.*\d)(?=.*[A-Z])(?:.{8,})$')
    
  • is_strong_caridorc – 53.2 µs
    A cleaner more pythonic version by Caridorc, where I believe the focus was on clarity and simplicity, not speed.

Performance conclusion

Using the precompiled regex by hjpotter92 using positive lookahead is the fastest version, and most compact version as well. But the OP's version with a little trimming is not lacking that much when it comes to speed (for my testcases).

And then it is up to the reader, which version they think is easiest on the eye, and is most understandable, readable or whatever metric they choose to use!

Here is the complete script used for testing:

from __future__ import print_function
import re


def is_strong_org(password):
    """OP non-regexp version."""
    length = len(password) >= 8
    case = password != password.upper() and password != password.lower()
    digit = any(c.isdigit() for c in password)

    return(length and case and digit == True)


def is_strong_org_v2(password):
    """OP non-regex version, with immediate return."""

    return (len(password) >= 8 
            and password != password.upper()
            and password != password.lower()
            and any(c.isdigit() for c in password))


def is_strong_regex(password):
    """OP's original version with regex."""

    length_regex = re.compile(r'.{8,}')
    length = True if length_regex.search(password) != None else False

    uppercase_regex = re.compile(r'[A-Z]')
    lowercase_regex = re.compile(r'[a-z]')
    uppercase = True if uppercase_regex.search(password) != None else False
    lowercase = True if lowercase_regex.search(password) != None else False
    case = True if uppercase and lowercase == True else False

    digit_regex = re.compile(r'[0-9]')
    digit = True if digit_regex.search(password) != None else False

    return(length and case and digit == True)

LENGTH = re.compile(r'.{8,}')  
UPPERCASE = re.compile(r'[A-Z]')
LOWERCASE = re.compile(r'[a-z]')
DIGIT = re.compile(r'[0-9]')

### Extracted regex

def is_strong_regex_v2(password):
    """OP regex version, with extracted regex compilation."""
    length = True if LENGTH.search(password) != None else False

    uppercase = True if UPPERCASE.search(password) != None else False
    lowercase = True if LOWERCASE.search(password) != None else False
    case = True if uppercase and lowercase == True else False

    digit = True if DIGIT.search(password) != None else False

    return(length and case and digit == True)


def is_strong_regex_v3(password):
    """OP regex version, with extracted regex, and simpler boolean statements."""

    length = LENGTH.search(password) is not None
    uppercase = UPPERCASE.search(password) is not None
    lowercase = LOWERCASE.search(password) is not None
    digit = DIGIT.search(password) is not None

    return length and uppercase and lowercase and digit


def is_strong_regex_v4(password):
    """OP regex version, with extracted regex, and immediate return."""

    return (LENGTH.search(password)
            and UPPERCASE.search(password)
            and LOWERCASE.search(password)
            and DIGIT.search(password))


HJPOTTER92 = re.compile(r'^(?=.*[a-z])(?=.*\d)(?=.*[A-Z])(?:.{8,})$')

def is_strong_hjpotter92(password):
    """hjpotter92 version from comment."""

    return HJPOTTER92.search(password)



from string import ascii_lowercase, ascii_uppercase, digits

def is_strong_caridorc(password):
    """Version by caridorc with 'import string'."""

    return len(password) >= 8 and \
           any(upper in password for upper in ascii_uppercase) and \
           any(lower in password for lower in ascii_lowercase) and \
           any(digit in password for digit in digits)  


ALL_CASES = re.compile(r'^(?:(?P<number>\d)|(?P<lower>[a-z])|(?P<upper>[A-Z])|(?P<other>[^a-zA-Z0-9])){8,}$')

def is_strong_holroy(password):
    """Version by holroy, with named capturing groups."""

    m = ALL_CASES.search(password)

    return m is not None and m.group('number') and m.group('upper') and m.group('lower')



STRONG_PASSWORDS = [
    "ABCabc12",
    "abc123AB",
    "123ABCab",
    "Aa1bB2Cc",
    "abcABC123",
    "A TRUELY STRONG password in between the 6 so-called strong passwords!!!!",
    ]

WEAK_PASSWORDS = [
    "aaaa",
    "!()!=!()",
    "aaaaaaaa",
    "AAAAAAAA",
    "11121312",
    "ABCDabcd",
    "abcd1234",
    "1234ABCD",
    ]


def get_non_strong_passwords(passwords, test_function):
    """Return all passwords which are not strong."""

    return  [password for password in passwords
                      if not test_function(password)]


def get_non_weak_passwords(passwords, test_function):
    """Return all passwords which are not strong."""

    return  [password for password in passwords
                      if test_function(password)]


def password_tester(test_function, with_output=False):
    if with_output:
        print('Testing {}: '.format(test_function.__name__), end='')

    non_strong_passwords = get_non_strong_passwords(STRONG_PASSWORDS, test_function)
    non_weak_passwords = get_non_weak_passwords(WEAK_PASSWORDS, test_function)

    if not with_output:
        return False

    if not non_strong_passwords and not non_weak_passwords:
        print('All OK');
        return True

    if non_strong_passwords:
        print('  Failed on strong passwords: {}'.format(', '.join(non_strong_passwords)))

    if non_weak_passwords:
        print('  Failed on weak passwords: {}'.format(', '.join(non_strong_passwords)))

    return False


def main():  

    # Code used to verify correctness
    for test_function in (is_strong_regex,
                          is_strong_regex_v2,
                          is_strong_regex_v3,
                          is_strong_regex_v4,
                          is_strong_org,
                          is_strong_org_v2,
                          is_strong_holroy,
                          is_strong_hjpotter92,
                          is_strong_caridorc,
                          ):

        password_tester(test_function, True)       


if __name__ == '__main__':
    main()

''' Code run within the IPython shell to get actual timings:

In [300]: for test_function in (is_strong_org,
     ...:                       is_strong_org_v2,
     ...:                       is_strong_regex,
     ...:                       is_strong_regex_v2,
     ...:                       is_strong_regex_v3,
     ...:                       is_strong_regex_v4,
     ...:                       is_strong_holroy,
     ...:                       is_strong_hjpotter92,
     ...:                       is_strong_caridorc,
     ...:                       ):
     ...:     print("\nTesting {}:".format(test_function.__name__))
     ...:     %timeit password_tester(test_function)
     ...:     
'''
\$\endgroup\$
3
\$\begingroup\$

Just write the function as similar as possible to the requirements:

from string import ascii_lowercase, ascii_uppercase, digits

def is_strong(password):
    return len(password) >= 8 and \
           any(upper in password for upper in ascii_uppercase) and \
           any(lower in password for lower in ascii_lowercase) and \
           any(digit in password for digit in digits)

This reads so easily you may think it is pseudocode.

\$\endgroup\$
2
\$\begingroup\$

While the existing answers are all good, one nice approach is yet to be suggested: looping through the regexes.

LENGTH = re.compile(r'.{8,}')  
UPPERCASE = re.compile(r'[A-Z]')
LOWERCASE = re.compile(r'[a-z]')
DIGIT = re.compile(r'[0-9]')

ALL_PATTERNS = (LENGTH, UPPERCASE, LOWERCASE, DIGIT)

def is_password_strong(password):
    return all(pattern.search(password) for pattern in ALL_PATTERNS)
\$\endgroup\$

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.