7
\$\begingroup\$

This exercise comes from Automate the Boring Stuff Ch 7. The assignment is to use regular expressions to test the strength of a password. Password must be >= 8 characters, contain at least one uppercase letter, one lowercase letter, and one number.

Any and all criticisms/suggestions for improving the code are welcome.

# function to test the strength of a password

import re

# pw = password
# function receives user input (password) as argument
def pw_strength_test(pw: str) -> bool:
    is_strong = True # final return value, change to False if test fails
    
    # lowercase, uppercase, and number regexes
    lower_regex = re.compile(r"[a-z]")
    upper_regex = re.compile(r"[A-Z]")
    num_regex = re.compile(r"\d")

    # store matches into lists
    lower_groups = lower_regex.findall(pw)
    upper_groups = upper_regex.findall(pw)
    num_groups = num_regex.findall(pw)

    # def a function to iterate over the regex lists
    # pass list and type into function
    # return number of items of that type
    def count_matches(match_list, match_type):
        count = 0
        try:
            if match_type == str and match_list[0].islower() == True:
                count = len(match_list)
            elif match_type == str and match_list[0].isupper() == True:
                count = len(match_list)
            elif match_type == int:
                count = len(match_list)
        # if no items in list, IndexError will be thrown
        except IndexError:
            print('IndexError thrown. One or more match lists are empty.')

        return count

    # store count into variables
    lower_count = count_matches(lower_groups, str)
    upper_count = count_matches(upper_groups, str)
    num_count = count_matches(num_groups, int)
    
    # if any list contains 0 items, pw test fails
    if lower_count == 0 or upper_count == 0 or num_count == 0:
        is_strong = False
        print("Error: password strength test failed.")
    # if less than 8 characters, pw test fails
    elif (lower_count + upper_count + num_count) < 8:
        is_strong = False
        print("Error: password strength test failed.")
    else:
        print("Password strength test passed.")

    return is_strong

print('Please enter your password.')
pw_strength_test(str(input()))
```
\$\endgroup\$
2

2 Answers 2

13
\$\begingroup\$

The current implementation is too complex. As soon as you get the results from the three findall() calls, you have essentially everything you need, but the function continues with over a dozen lines of unnecessary stuff. Remember that an empty collection evaluates to false in boolean terms, so if the goal is just to write a function that returns true or false, you could wrap up the logic as simply as this:

return lower_groups and upper_groups and num_groups and len(pw) >= 8

A password checker should not print. A password testing function should be a purely data-oriented function: it should take a password and return data; it should not have other side effects, such as printing. The returned data could be a bool (the simplest implementation) or it could be some other type of object: for example, a (bool, error_message) tuple; or just an error_message or None (the code example below takes the latter approach, reframing the function as a password-error-finder). In any case, you want to keep the in-depth checking logic in an easily-tested, data-oriented function. Leave the printing to a different part of the program.

Collections are often more powerful than separate variables. If you do want the function to provide a helpful error message (the current messages are not helpful, because they do not tell the user specifically what's wrong), you should consider bundling the checks into a collection so that any failed check can be linked to its corresponding message. The code below illustrates one way to do that kind of thing. It also provides a compact illustration of how data-centric thinking can radically simplify code. The function focuses nearly all of its attention on building a convenient data structure. And once we have that data, the rest of the algorithm/logic becomes almost trivial.

import re
from typing import Tuple

def check_password(pw: str) -> Tuple[str]:
    checks = {
        'must be at least 8 characters': r'.{8}',
        'must contain lowercase letter': r'[a-z]',
        'must contain uppercase letter': r'[A-Z]',
        'must contain digit': r'\d',
    }
    return tuple(
        err_msg
        for err_msg, pattern in checks.items()
        if not re.search(pattern, pw)
    )
\$\endgroup\$
18
  • 6
    \$\begingroup\$ For actual use, I would strongly recommend return [err for (err, ok) in checks.items() if not ok]. The point being not the use of a "comprehension" (which are good!), but rather that reporting every error detected is a better UX. \$\endgroup\$ Mar 27, 2022 at 3:10
  • 2
    \$\begingroup\$ minor remark on this answer: there's not really any need to make checks a dictionary, since you never want to search it by key. You could just make it a list of tuples, which IMO reflects better how it is used. \$\endgroup\$ Mar 27, 2022 at 16:19
  • 2
    \$\begingroup\$ I agree that a dict is not appropriate here. It's not about cost; it's about intent: you're not doing any lookups. A sequence of tuples fits the purpose better. Also: don't findall; just search - you don't care about "all matches"; you care about "at least one match". Finally: returning presentation strings from a business logic function is not a good idea. \$\endgroup\$
    – Reinderien
    Mar 27, 2022 at 18:07
  • 2
    \$\begingroup\$ A more uniform implementation of your strategy would be for the second element of each tuple to be a regular expression only, the first being .{8,}. Then your bool cast and search call would be written once and not four times. \$\endgroup\$
    – Reinderien
    Mar 27, 2022 at 18:09
  • 3
    \$\begingroup\$ @Reinderien Not convinced that the dict-vs-tuples question matters in this case. But I agree in principle with your other points, especially the simplifications regarding the regexes, which have the added benefit of reinforcing the regex-learning goal of the exercise. Edited the answer to incorporate your good idea and also to implement the earlier suggestion to return all errors rather than just the first. \$\endgroup\$
    – FMc
    Mar 27, 2022 at 18:57
2
\$\begingroup\$

The original problem description is rather thin. It doesn't require that you print anything, or even indicate why the password was weak. So your print() statements can go away. If you did want to add this feature - indicating why a password failed - then you'd either return an Enum instance (not a printable string), or raise an exception.

You can condense your regular expressions down to one:

import re


PASS_PATTERN = re.compile(
    r'''
    ^            # start of password
    (?=.*?[A-Z]) # lookahead: any characters, then an uppercase letter
    (?=.*?[a-z]) # lookahead: any characters, then a lowercase letter
    (?=.*?[0-9]) # lookahead: any characters, then a digit
    .{8,}        # at least eight characters
    $            # end of password
    ''',
    re.VERBOSE,
)


def is_strong_password(password: str) -> bool:
    return PASS_PATTERN.match(password) is not None
\$\endgroup\$
5
  • \$\begingroup\$ would you mind clarifying the return statement, specifically the part with is not None? Perhaps I'm not understanding it correctly, but it seems that PASS_PATTERN.match(password) would return None instead of a Boolean value if the regex does not find a match. \$\endgroup\$
    – Ramza
    Mar 27, 2022 at 21:39
  • \$\begingroup\$ You're exactly right - and to convert from a None to a False, we write is not None. The is replaces == since None is a singleton in Python. \$\endgroup\$
    – Reinderien
    Mar 27, 2022 at 23:22
  • \$\begingroup\$ Does that mean that != None has the same effect? This is new to me, because up until now I've been used to very simple return statements. \$\endgroup\$
    – Ramza
    Mar 27, 2022 at 23:43
  • 1
    \$\begingroup\$ See stackoverflow.com/questions/14247373/… \$\endgroup\$
    – Reinderien
    Mar 27, 2022 at 23:45
  • \$\begingroup\$ @Maska Your latest edit first of all would not have worked with match(), only with search(); and secondly gets into performance topics that I have not bothered to cover. You should post a separate answer with your performance analyses detailed. \$\endgroup\$
    – Reinderien
    Mar 28, 2022 at 13:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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