Beginner level password generator optimization

Question

I've been learning python for a few days now, and I programmed this password generator after today's lesson (following the 100 days of code on udemy course). This code works like it should and gives me the desired results, but out of pure curiosity, I am interested in optimizing the code even more.

import random

letters = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z']
numbers = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']
symbols = ['!', '#', '$', '%', '&', '(', ')', '*', '+']

print("Welcome to the PyPassword Generator!")
nr_letters = int(input("How many letters would you like in your password?\n"))
nr_symbols = int(input(f"How many symbols would you like?\n"))
nr_numbers = int(input(f"How many numbers would you like?\n"))

password = []

for req in range(0, nr_letters + 1):
    letter_index = random.randint(0, len(letters))
    password.append(letters[letter_index-1])

for sym in range(1, nr_symbols + 1):
    symbol_index = random.randint(0,len(symbols))
    password.append(symbols[symbol_index-1])

for num in range(1, nr_numbers + 1):
    number_index = random.randint(0,len(numbers))
    password.append(numbers[number_index-1])

random.shuffle(password)
print(*password,sep='')

Answer 1

Remarks:

• Bug: you have an extra letter sneaking in with for req in range(0, nr_letters + 1):, making your output longer than it should be. That should be for req in range(0, nr_letters): or for req in range(nr_letters):. The other calls like for sym in range(1, nr_symbols + 1): can be simplified as well. Try to eliminate these little + and - operations. You generally don't need them, and off by one errors are one of the classic subtle programming bugs.
• In the line above, req is unused, so replace it with a _, by convention.
• Avoid indexing--prefer random.choices to select random items from a list (random.choice if you just want to select one item).
  • Tiny bug: random.randint(0,len(numbers)) should be random.randint(0, len(numbers) - 1) since the end is inclusive, and don't subtract 1 once you have this index. You're subtracting 1 later, so your range is -1 to len(numbers) - 1. The -1 is a valid index in Python so your code won't raise an IndexError, but it means there'll be a bit of extra weight added to the last item.
• Separate user interaction (I/O) and algorithm logic.
• Use a function to make your code reusable, testable and abstract away complexity.
• Add type hints to your function and check them with pyright.
• Don't use f-strings unless there's actually interpolation happening.
• Don't assume input is valid; handle errors gracefully.
• There is no performance issue with your code, so "optimization" doesn't matter at this point. Focus on coding style and writing maintainable code.
• Add docstrings.
• Consider adding unit tests. Since your code uses randomness, you can use a regex or logic to make sure the password has the right stuff in it, or seed the random library to be deterministic. But even writing a simple length test would have caught your primary bug mentioned above. Fuzz testing can also be used to catch edge case crashes and make sure your input sanitization and exception handling is tight. A comprehensive test suite means you can refactor with a lower likelihood of causing regressions.

Here's a rewrite:

from random import choices, shuffle
from string import ascii_letters, digits


def generate_password(nr_letters: int, nr_numbers: int, nr_symbols: int) -> str:
    """Generates a password with n random letters, numbers and symbols"""
    symbols = "!#$%&()*+"
    password = [
        *choices(ascii_letters, k=nr_letters),
        *choices(digits, k=nr_numbers),
        *choices(symbols, k=nr_symbols),
    ]
    shuffle(password)
    return "".join(password)


def read_positive_int(
    prompt: str, invalid: str = "Input must be a positive integer"
) -> int:
    """Reads a positive integer input from stdin, repeating until successful"""
    while True:
        try:
            if (n := int(input(prompt))) > 0:
                return n
        except ValueError:
            pass
        print(invalid)


def main():
    """Interacts with the user to generate a password"""
    print("Welcome to the PyPassword Generator!")
    nr_letters = read_positive_int("How many letters would you like in your password? ")
    nr_symbols = read_positive_int("How many symbols would you like? ")
    nr_numbers = read_positive_int("How many numbers would you like? ")
    password = generate_password(nr_letters, nr_numbers, nr_symbols)
    print(password)


if __name__ == "__main__":
    main()

You could generalize this a step further and accept arbitrary iterables and counts instead of hardcoded letters, numbers and symbols, but that seems a bit premature, so I left the function as-is in that respect.

3 positional args is getting to be a bit much, so you might want to make those kwargs as well, also left as a future improvement.

I'm not a security expert, so I can't remark on whether this should be used for actual passwords. Erring on the side of caution, I'd say don't use it in favor of an expert-vetted algorithm. See Typical password generator in Python for our community thread on the topic.

Answer 2

Security

The documentation for the random module gives the following warning:

Warning: The pseudo-random generators of this module should not be used for security purposes. For security or cryptographic uses, see the secrets module.

The secrets module can be used in a similar way to the random module, but uses the best source of randomness available on your system. However, it doesn't provide an equivalent method to shuffle, so you'll have to rework your algorithm a bit. You can take inspiration from the "Recipes and best practices" section of the secrets documentation.

Index into string

Instead of using lists of single-character strings, you can index directly into strings:

>>> foo = 'abc'
>>> foo[1]
'b'

Taking advantage of this would make the definitions at the beginning of the program easier to write and more readable.

Note that strings are immutable, you can't use string indexing to change characters:

>>> foo[1] = 'd'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'str' object does not support item assignment

In your case, changing the character sets would likely be a bug, so string immutability will catch these.

Use built-ins when possible

Even better, the string modules provide these constant strings, so the character set definitions can be simplified to:

from string import ascii_letters, digits, punctuation

This would ensure that no character character is omitted or duplicated, and reduce complexity, whether for writing the code or proofreading it.

You could also prefer to use ascii_lowercase and ascii_uppercase for more fine-grained control.

Factor code into functions

This improves reusability, testability and readability of your code. @ggorlen provides good feedback on how to do that.