Skip to main content
added 123 characters in body
Source Link
STerliakov
  • 2k
  • 3
  • 13

(and definitely not if not x == y, there is a fancy syntax sugar for that called "not equal" and denoted as x != y)

(and definitely not if not x == y, there is a fancy syntax sugar for that called "not equal" and denoted as x != y)

Source Link
STerliakov
  • 2k
  • 3
  • 13

Please read this as an addition to two existing answers, I agree with almost all points made there.

Whitespace

You may like excessive vertical whitespace, it's your choice. But in this case it's a choice that violates formatting standard (PEP8, link to relevant section):

Use blank lines in functions, sparingly, to indicate logical sections.

Most existing formatters (black, ruff, autopep8) will deny more than one line inside a function. And it's generally good thing to do: when you split all the lines into separate "blocks" 1-2 lines each, it's becoming more difficult to read. And please, definitely no blank lines immediately after for loop - indentation makes the following line sufficiently distinct.

Docstring

A nitpick in addition to answer by @J_H

Prescribe, do not describe. The first sentence of a docstring should be prescriptive, like

Check if a string meets the requirements for a master password.

Booleans are good as booleans!

You have the following:

if not valid_characters_count == string_length or not all([has_digit, has_lower_case, has_upper_case]):
    return False
return True

While it could be just

return (
    valid_characters_count == string_length
    and all([has_digit, has_lower_case, has_upper_case])
)

Entrypoint

You have a shebang on top of your script. It is a good thing to have in scripts, but not at all in library parts. If you're going to allow using this as a script, that's OK - then make it actually usable as a script. Probably you want a script that can read a password and report its quality by exit code (0 for OK, 1 for insufficiently strong). You can use argparse for it to get help and validation for free.

def _read_args(source: list[str]):
    parser = argparse.ArgumentParser(
        prog="check_password.py",
        description="Utility for admin password validation",
    )
    parser.add_argument("password", help="The password to check")
    parser.add_argument(
        "-q",
        "--quiet",
        action="store_false",
        dest="verbose",
        help="Omit output",
    )
    return parser.parse_args(source)


if __name__ == "__main__":
    args = _read_args(sys.argv[1:])
    is_strong = is_strong_master_password(args.password)
    if args.verbose:
        if is_strong:
            print("Validated successfully")
        else:
            print("Validation failed", file=sys.stderr)
    sys.exit(1 - is_strong)

I'm omitting the return type above for brevity, you may use a Protocol to make this fully typed:

from typing import Protocol


class _CommandArguments(Protocol):
    password: str
    verbose: bool


def _read_args(source: list[str]) -> _CommandArguments:
    ...  # Implementation follows

Alternatively, you can remove the shebang if it's not going to be a script.

Reference code

Here's what (roughly) I would have done with these requirements (note that in real life 5-char ASCII password can not be secure):

#!/usr/bin/env python3
"""Utility script for admin password validation."""

import argparse
import string
import sys
from typing import Final, Protocol

MIN_PASSWORD_LENGTH: Final = 5
MAX_PASSWORD_LENGTH: Final = 20
ALLOWED_CHARACTERS: Final = set(
    "".join([string.ascii_lowercase, string.ascii_uppercase, string.digits])
)


def is_strong_master_password(password: str) -> bool:
    """Check if a string meets the requirements for a master password."""

    string_length = len(password)
    if string_length < MIN_PASSWORD_LENGTH:
        return False
    if string_length > MAX_PASSWORD_LENGTH:
        return False

    if set(password) - ALLOWED_CHARACTERS:
        # Contains characters outside of allowed range.
        return False

    has_upper_case = False
    has_lower_case = False
    has_digit = False
    for character in password:
        if character.isdigit():
            has_digit = True
        if character.isupper():
            has_upper_case = True
        if character.islower():
            has_lower_case = True

    return all([has_digit, has_lower_case, has_upper_case])

...  # Entrypoint code goes here

Yes, set building costs more than iteration, but less than you implementation: your original code will check up to (26+26+10) characters on every iteration, while set difference does not have to go this far. We build a set in O(n log n) with n being a password length, but it's still nothing with any real-life length constraints. This code is likely not something performance-critical, and so brevity and clean look should be more important.