3
\$\begingroup\$

I made this random password script. It works, but i think it is kind of bloated. The part where random characters are being added is the area where I think it needs work. I don't want to declare if and else statement for every combination of lowercase list, uppercase list and digits list(I am taking input from user whether they want uppercase,lowercase,digits or any possible combination of these three). Is there any possible way in which I can just check which of these three values are yes and then just use only those.

Also what is more efficient using lower() to turn the list called using string module into lower case or just calling the lowercase list itself.

import random
from  string import ascii_letters ,digits
while True:
    userchoice_size = int(input("Enter the length of the password : ")) # Asks for length of password
    userchoice_syntax_lower = input("Do you want lower case letter?(y or n) ")
    userchoice_syntax_upper = input("Do you want upper case letter?(y or n) ")
    userchoice_syntax_number = input("Do you want numbers?(y or n) ")
    counter=0
    password='' # String which stores the password
    choice_list=[1,2,3]
    while counter <= userchoice_size:
        choice_number = random.choice(choice_list)
        if choice_number == 1:
            if userchoice_syntax_upper == 'y': # Uppercase list
                password = password + str(random.choice(ascii_letters.upper())) 
                counter += 1
        if choice_number == 2:
            if userchoice_syntax_number=='y': # Digits list
                password = password + str(random.choice(digits))
                counter += 1
        if choice_number == 3:
            if userchoice_syntax_lower == 'y': # Lower case list
                password= password + str(random.choice(ascii_letters.lower())) # Converts all letters into lowercase then randomly select and store in list
                counter += 1
    print (password)
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

About your code

Style

Your style is mostly fine, except lacking a bit of white space, especially after the imports, and having mostly useless comments that break the 80 character barrier.

Input validation

You don't check if the user input can be parsed as an integer in the first input. The int() methods validates its input, however a bad input will crash your program. It would be preferable to reject the input, and ask again.

You don't validate the input for the yes/no question either, and it is much worse. Typing anything other than a lowercase y will be treated as a no. At the very least, add a .lower() to handle inputs like Y. It would be high priority to check for n/N, instead of silently treating anything else than y as meaning no.

Since you ask 3 yes/no questions (and possibly more if you'd like to add stuff like special characters), you should delegate the input and its validation to a separate method.

Bug

If the user says "no" (more precisely: not "y") to all 3 questions, the program will hang indefinitely, as counter will never increase and the inner while loop will never end.

Security

random is not a cryptographically secure random number generator. Use secrets instead, which is very similar to use.

Also, ideally you'd want the probability for any character to be chosen to be the same. By separating the character categories as you did, digits are over-represented if they appear alongside letters.

Outer while loop

You put the entire code in an infinite loop. How many passwords do you think a user would want to generate?

My take on the problem

def generate_password():
    password_length = ask_for_int('Enter the length of the password')
    
    choices = ''
    if ask_for_bool('Use lowercase letters? [y/n]'):
        choices += string.ascii_lowercase
    if ask_for_bool('Use uppercase letters? [y/n]'):
        choices += string.ascii_uppercase
    if ask_for_bool('Use digits? [y/n]'):
        choices += string.digits
    
    password = ''
    for _ in range(password_length):
        password += secrets.choice(choices)
        
    return password


def ask_for_int(msg):
    while True:
        print(msg)
        try:
            return int(input('> '))
        except ValueError:
            print('Unable to parse number. Please try again.')


def ask_for_bool(msg):
    while True:
        print(msg)
        user_in = input('> ').lower()
        if user_in =='y':
            return True
        if user_in =='n':
            return False
        print('Unable to parse answer. Please try again.')
\$\endgroup\$
4
  • \$\begingroup\$ password += should be replaced with ''.join - secrets even shows an example doing so. \$\endgroup\$
    – Reinderien
    Jun 19, 2020 at 16:23
  • \$\begingroup\$ @Reinderien Is there a reason - besides performance - for doing so? I don't like list comprehensions, as they are rather hard to read. I assume that arsh is quite new to python, and I feel that code readability is more valuable than a clever one-liner, especially at a beginner level. \$\endgroup\$
    – gazoh
    Jun 19, 2020 at 16:31
  • 1
    \$\begingroup\$ The use of ''.join in this case is already well-understood, and is more "standard" than "clever". Terseness and performance are both factors, and for some it's more legible than a loop. For the scale of password generation performance will not matter, but in other applications your loop will be O(n²) where join is O(n). \$\endgroup\$
    – Reinderien
    Jun 19, 2020 at 16:33
  • \$\begingroup\$ More generally: I try to give submissions on this site the best code possible and not necessarily code that they're guaranteed to understand, since presumably they came here to learn. People that come here are aspiring. \$\endgroup\$
    – Reinderien
    Jun 19, 2020 at 16:36
3
\$\begingroup\$

Character distribution

You randomly choose between uppercase, lowercase and digit characters with an equal probability, but there are more letters than digit characters. That means characters will be under-represented in your password's probability distribution. Easier and more accurate is to make one string of all eligible characters and choose from it randomly.

Redundant str

No need for str here:

str(random.choice(ascii_letters.upper())) 

Move your increment

counter += 1

should not be repeated three times in if statements. You can do it once at the end of the loop. Better yet,

for _ in range(userchoice_size):

Choice list

This:

choice_list=[1,2,3]
choice_number = random.choice(choice_list)

should not use a list, and should be replaced with a call to randrange.

Secure entropy

For password generation it's crucially important to use a secure source of entropy. Read this quote carefully:

Python uses the Mersenne Twister as the core generator. It produces 53-bit precision floats and has a period of 2**19937-1. The underlying implementation in C is both fast and threadsafe. The Mersenne Twister is one of the most extensively tested random number generators in existence. However, being completely deterministic, it is not suitable for all purposes, and is completely unsuitable for cryptographic purposes.

Emphasis mine. A better built-in option is secrets.

\$\endgroup\$
1

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.