Remarks:
- Bug:
random.randint(0,len(numbers))should berandom.randint(0, len(numbers) - 1)since the end is inclusive. If you run your program enough it gives anIndexError. - Avoid indexing--prefer
random.choicesto select random items from a list (random.choiceif you just want to select one item). - Separate user interaction (I/O) and algorithm logic.
- Use a function to make your code reusable 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. You could find your crash mentioned above using a fuzz test--run thousands of random inputs.
Here's a rewrite:
from random import choices, shuffle
from string import ascii_letters
def generate_password(nr_letters: int, nr_numbers: int, nr_symbols: int) -> str:
"""Generates a password with n random letters, numbers and symbols"""
numbers = "0123456789"
symbols = "!#$%&()*+"
password = [
*choices(ascii_letters, k=nr_letters),
*choices(numbers, 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:
...
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.