5
\$\begingroup\$

This is my first project using Python. I made a simple password generator that checks user input. How can I improve it?

import random
def password_generator():
    password = []
    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"]
    password_length = 0
    password_numbers = []
    password_letters = []
    # Input the length of the password
    while True:
        password_length_input = input("Choose the length of your password with numbers between 6 and 15:\n")
        if not password_length_input.isnumeric():
            print(f"{password_length_input} is not a number, try again:")
            continue
        else:
            password_length = int(password_length_input)
            print(f"Password length: {password_length}")
        if 6 <= password_length <= 15:
            break
        else:
            print("The password must be between 6 and 15 characters, try again:")
            continue
    # Input the amount of numbers in password
    while True:
        password_numbers_input = \
            input(f"Choose the amount of numbers you want in your password, max {password_length}\n")
        if not password_numbers_input.isnumeric():
            print(f"{password_numbers_input} is not a number try again")
            continue
        elif int(password_numbers_input) > password_length:
            password_numbers = 0
            print(f"The value is too high, choose maximum {password_length} numbers")
            continue
        else:
            password_numbers = int(password_numbers_input)
            print(f"Password numbers: {password_numbers}")
            for number in range(0,password_numbers):
                password.append(random.randrange(0,9))
            break
    # Check for numbers and letters in password
    while True:
        if password_numbers == password_length:
            print(f"The password will be only {password_numbers} numbers, no letters.")
            break
        else:
            password_letters = password_length - password_numbers
            print(f"""Your password will be {password_length} characters with {password_numbers} numbers and {password_letters} letters.""")
            for letter in range(0,password_letters):
                password.append(random.choice(letters))
            break
    random.shuffle(password)
    password_string = ''.join([str(item) for item in password])
    print(f"Your password is:\n{password_string}")

password_generator()

Usage example:

Choose the length of your password with numbers between 6 and 15:
Password length: 8
Choose the amount of numbers you want in your password, max 8
Password numbers: 2
Your password will be 8 characters with 2 numbers and 6 letters.
Your password is:
pzc11bmf
\$\endgroup\$
  • 1
    \$\begingroup\$ Are you required to ask the user interactively for the 2 pieces of information? I ask because a more typical design for a password creation script would just get the required parameters directly on the command line and then just print the password, with no interactivity at all: python create-password 8 2 \$\endgroup\$ – FMc Sep 11 at 8:02
  • 1
    \$\begingroup\$ Note: random should not be used for security purposes, but you can use secrets instead; you probably should replace random with secrets.SystemRandom() if generating real passwords or any other cryptographically secure value. \$\endgroup\$ – 0x2b3bfa0 Sep 11 at 12:27
9
\$\begingroup\$
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"]

This method of writing the alphabet is very error prone. I would import string and use string.ascii_lowercase in place of letters. If you want to generate your own range of letters for whatever reason, I would write

letters = [chr(n) for n in range(ord('a'), ord('z') + 1)]

since there's then no danger of omitting or duplicating a letter.


password_length = 0
password_numbers = []
password_letters = []

These default values are never used. The defaults for password_numbers and password_letters don't make sense since those variables hold numbers. I would delete all three lines.


if not password_length_input.isnumeric():
    print(f"{password_length_input} is not a number, try again:")
    continue
else:
    password_length = int(password_length_input)
    print(f"Password length: {password_length}")

I would instead write

try:
    password_length = int(password_length_input)
except ValueError:
    print(f"{password_length_input} is not a number, try again:")
    continue
print(f"Password length: {password_length}")

while True:
    if password_numbers == password_length:
        ...
        break
    else:
        ...
        break

There is no sense in having a while loop here since you always break out of it on the first iteration.


range(0,password_numbers)

You can just write range(password_numbers).


password.append(random.randrange(0,9))

This will append a digit from 0 to 8 inclusive, never 9. If you want all ten digits you should write random.randrange(10). Or, perhaps better, use random.choice(string.digits).


password_string = ''.join([str(item) for item in password])

If you use string.digits then every element of password will be a character so you can simplify this to password_string = ''.join(password).

| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

A more straightforward way of generating a random string:

import random
import string

    def get_random_string(length):
        letters = string.ascii_lowercase
        result_str = ''.join(random.choice(letters) for i in range(length))
        print("Random string of length", length, "is:", result_str)
    
    get_random_string(8)
    get_random_string(8)
    get_random_string(6)

borrowed from here, and there are more examples.

Now if you have specific requirements like a minimum number of digits, you can either tweak the formula, or generate two lists and merge them while shuffling the values.

There is an example in the link I quoted above: "Generate a random alphanumeric string with a fixed count of letters and digits" => merging two list comprehensions.

The way you are doing it is procedural but not Pythonic. It is kinda reinventing the wheel.

At the very least, your list of allowed characters should look like this:

letters = "abcdefghijklmnopqrstuvwxyz"

Then you pick out a random letter eg letters[3] will return 'd' since the list is 0-based and Python treats strings as sequences of characters. Using shuffle like you are already doing, you can write more concise code.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

First

I suggest creating separate methods for each while loops.

Second

"while True" loop is not a good practice, I think. Instead of that, use condition.

Third

I suggest creating PasswordGenerator class which will contain your code. It will help you to expand your code in the future.

Base structure for your project

 class PasswordGenerator():
     
    check_declared_password_length():
        ...
        
    check_amount_of_password_numbers():
        ...
    *
    *
    *

For the end remember to create functions with one responsibility. After that, you can write unit tests for each of that and it will be easier.

| improve this answer | |
\$\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.