4
\$\begingroup\$

I have the following code here

It's a password generator that lets you choose a completely random password with a certain length of a custom password with different amounts of character types.

I want feedback of the general readability, style, and security (as it's a password generator), and if you would do anything differently.

The code:

# Imports libraries
import secrets
import string

# Creates list of every possible character for the "general" option
fulllist = string.ascii_lowercase + string.ascii_uppercase + string.digits + string.punctuation
# Asks user what option they want
generalorcustom = input("General (g) or custom (c) password: ")
# Randomly grabs a character from the 'fulllist' as many times as specified
def general():
    return ''.join(secrets.choice(fulllist) for i in range(int(length))) 

def custom():
    # Inputs how many of each character user wants
    lowernum = input("Lowwer case character count: ")
    uppernum = input("Upper case character count: ")
    digitnum = input("Digit count: ")
    specnum = input("Special character count: ")
    # Creates list, then randomly adds each characters of each type to list for specified amount
    temp_list = []
    temp_list.extend(list(secrets.choice(string.ascii_lowercase) for i in range(int(lowernum))))
    temp_list.extend(list(secrets.choice(string.ascii_uppercase) for i in range(int(uppernum))))
    temp_list.extend(list(secrets.choice(string.digits) for i in range(int(digitnum))))
    temp_list.extend(list(secrets.choice(string.punctuation) for i in range(int(specnum))))
    # Creates final string, and randomly adds contents of temp_list to final string
    password = ''
    rangevar = list(range(len(temp_list)))
    while len(rangevar) > 0:
        picked_item = secrets.choice(rangevar)
        password += temp_list[picked_item]
        rangevar.remove(picked_item)

    return password
# Checks and executes function as user requested
if generalorcustom.lower() == "general" or generalorcustom.lower() == "g":
    length = input("Password length: ")
    print(general())
elif generalorcustom.lower() == "custom" or generalorcustom.lower() == "c":
    print(custom())
else:
    print('Error, invalid input')
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Regarding security, longer passwords of only lowercase letters are stronger than shorter passwords containing mix. A 13-character lowercase-only password is harder to crack than a 10-character password that could have uppercase letters, lowercase letters, or digits. (26**13 > 62**10). Forcing users to use any specific mix of characters encourages shorter passwords, because they're easier to remember or type. \$\endgroup\$
    – chepner
    Jan 20, 2023 at 21:40

1 Answer 1

3
\$\begingroup\$

A few minor inconsistencies and maybe two improvements.

PEP 8 styling or your own in other places uses lower_case_with_underscores but some variables like fulllist don't and that's alot of ls. For readability update variables accordingly (also generalorcustom).

Move .lower() to input so its only called once not 4 times.

In custom you get the password options then generate a list of indices to pick from. A more optimal solution would be to just shuffle temp_list (Fisher-Yates shuffle) as you have all the characters for the password you just want to scramble them now. After the shuffle you can use ''.join(temp_list)

Full shuffle:

for i in range(len(temp_list)-1, 1, -1):
    j=secrets.randbelow(i+1)
    temp = temp_list[i]
    temp_list[i] = temp_list[j]
    temp_list[j] = temp

return ''.join(temp_list)

You don't have any handlers for if the user enters invalid input for a number (like a letter or negative). I'd make a function that takes a prompt and returns a number if valid otherwise continues to prompt for a number, like so:

def get_number_input(prompt):
    num=0
    while True:
        try:
            num = int(input(prompt))
            if num < 0:
                print('Please enter a positive number')
                continue
            break
        except ValueError:
            print('Please enter a number')
    return num

Instead of a single run, I would wrap the input with a while loop so a user could generate multiple passwords, with some sort of break/exit option added.

I would avoid the global length and instead pass length as a parameter to general

Everything together:

import secrets
import string

# Creates list of every possible character for the "general" option
full_list = string.ascii_lowercase + string.ascii_uppercase + string.digits + string.punctuation

def get_number_input(prompt):
    num = 0
    while True:
        try:
            num = int(input(prompt))
            if num < 0:
                print('Please enter a positive number')
                continue
            break
        except ValueError:
            print('Please enter a number')
    return num

# Randomly grabs a character from the 'full_list' as many times as specified
def general(length):
    return ''.join(secrets.choice(full_list) for i in range(length)) 

def custom():
    # Inputs how many of each character user wants
    lower_num = get_number_input("Lower case character count: ")
    upper_num = get_number_input("Upper case character count: ")
    digit_num = get_number_input("Digit count: ")
    spec_num = get_number_input("Special character count: ")
    # Creates list, then randomly adds each characters of each type to list for specified amount
    temp_list = []
    temp_list.extend(list(secrets.choice(string.ascii_lowercase) for i in range(lower_num)))
    temp_list.extend(list(secrets.choice(string.ascii_uppercase) for i in range(upper_num)))
    temp_list.extend(list(secrets.choice(string.digits) for i in range(digit_num)))
    temp_list.extend(list(secrets.choice(string.punctuation) for i in range(spec_num)))
    # shuffle choices
    for i in range(len(temp_list)-1,1,-1):
        j=secrets.randbelow(i+1)
        temp=temp_list[i]
        temp_list[i]=temp_list[j]
        temp_list[j]=temp
    return ''.join(temp_list)

while True:
    # Asks user what option they want
    general_or_custom = input("General (g) or custom (c) password: ").lower()
    # Checks and executes function as user requested
    if general_or_custom == "general" or general_or_custom == "g":
        length = get_number_input("Password length: ")
        print(general(length))
    elif general_or_custom == "custom" or general_or_custom == "c":
        print(custom())
    else:
        print('Error, invalid input')
        break
\$\endgroup\$

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.