Simple Random Password Generator in Python

Question

I've made a password generator that works fine. It takes alphabetic, numeric, and punctuation characters and randomly generates a 8-16 character password.

import sys
import string
import random
def PasswordGenerator():
 Name = input("What is your name? ")
 if Name.isalpha():
 GeneratePassword = input(Name + " would you like to generate a random password? ")
 YesOptions = ["Yes", "yes", "Y", "y"]
 NoOptions = ["No", "no", "N", "n"]
 PasswordCharacters = string.ascii_letters + string.digits + string.punctuation
 while GeneratePassword in YesOptions:
 Password = "".join(random.choice(PasswordCharacters) for i in range(random.randint(8, 16)))
 print(Password)
 GeneratePasswordAgain = input("Would you like to generate another random password? ")
 while GeneratePasswordAgain in YesOptions:
 Password = "".join(random.choice(PasswordCharacters) for i in range(random.randint(8, 16)))
 print(Password)
 GeneratePasswordAgain = input("Would you like to generate another random password? ")
 break
 while GeneratePasswordAgain in NoOptions:
 print("Good bye!")
 sys.exit()
 break
 while GeneratePasswordAgain not in YesOptions or NoOptions:
 print("Not a valid response! Try again.")
 GeneratePasswordAgain = input("Would you like to generate another random password? ")
 break
 while GeneratePassword in NoOptions:
 print("Good bye!")
 sys.exit()
 while GeneratePassword not in YesOptions or NoOptions:
 print("Not a valid response! Try again.")
 PasswordGenerator()
 while GeneratePassword in YesOptions:
 Password = "".join(random.choice(PasswordCharacters) for i in range(random.randint(8, 16)))
 print(Password)
 GeneratePasswordAgain = input("Would you like to generate another random password? ")
 while GeneratePasswordAgain in YesOptions:
 Password = "".join(random.choice(PasswordCharacters) for i in range(random.randint(8, 16)))
 print(Password)
 break
 GeneratePasswordAgain = input("Would you like to generate another random password? ")
 while GeneratePasswordAgain in NoOptions:
 print("Good bye!")
 sys.exit()
 break
 while GeneratePasswordAgain not in YesOptions or NoOptions:
 print("Not a valid response! Try again.")
 GeneratePasswordAgain = input("Would you like to generate another random password? ")
 break
 while GeneratePassword in NoOptions:
 print("Good bye!")
 sys.exit()
 while Name is not Name.isalpha:
 print("Not a valid response! Try again.")
 PasswordGenerator()
PasswordGenerator()

• Martin R: Why does the program for the user name at all? Why does it matter if the user name is "Martin", "Joe123" or "💩"?
• Simon Forsberg: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

I found a bug. If the initial response to "Do you want to..." is neither yes-like or no-like, it prompts for the username again. Also, you have large sections of dead code that will never execute.

1. Avoid duplicating code.

   Several prompts are exactly the same as each other, even with the same logic after them. Refactor the code so that each possibility only occurs once.

2. Avoid duplicating variables

   The GeneratePassword and GeneratePasswordAgain variables serve the same purpose: to get user input and determine if it should continue. and should be unified.

3. Keep the code reusable.

   Avoid calls like sys.exit() that prevent this snippet of code from being used elsewhere.

4. Use the language features properly.

   You know how to use while loops -- so why are you making multiple unnecessary recursive calls, each of which performs the same task as a while loop?

5. Separate logic and user-interface. Separate the code into modules.

   Write the function that generates the password and does nothing else. Something like

    def RandomPassword(passwordCharacters, minLetters, maxLetters):
    return "".join(random.choice(PasswordCharacters) for i in range(random.randint(minLetters, maxLetters)))
    def DefaultRandomPassword():
    passwordCharacters = string.ascii_letters + string.digits + string.punctuation
    return RandomPassword(passwordCharacters, 8, 16)
   

   Write another function that takes a prompt, displays it, waits for the user to type something, and then does it again if it was not a valid yes-no answer. Then return either yes or no. Call this function multiple times.

• Throwaway25663579348: When I don't use sys.exit() in while GeneratePassword in NoOptions: the while GeneratePassword not in YesOptions and NoOptions: snippet is executed though.
• Snowbody: Then that's another bug. Hint: What do you intend to accomplish with GeneratePassword not in YesOptions and NoOptions clause? What is the order of operations? What is the effect of using and between two lists?

Answer 2

In addition to @Snowbody's answer, I'd like to point out that random.choice and random.randint are not fit for generating passwords. The random module makes use of the Mersenne Twister, which is not cryptographically secure. The Python 3.6 release added secrets to the standard library, which retrieves random data from /dev/urandom. If you're not running Python 3.6 or upwards, you can still use random.SystemRandom (or os.urandom directly).

---

Also, have a close look at this xkcd post :)

Answer 3

1. PEP 8: Top-level functions should be surrounded by two blank lines. CamelCase names are usually for classes. Functions and variables should be in lower_case_with_underscores. Limit all lines to a maximum of 79 characters.

2. Instead of PasswordGenerator() in the end of your script write this:

   if __name__ == '__main__':
    PasswordGenerator()
   

   You can read about it for example here.

3. If you have python3.6 then you can use f-strings:

   GeneratePassword = input(f"{Name} would you like to generate a random password? ")
   

4. You have some magic numbers in your code. How about taking them to function's signature? We also could take there YesOptions and NoOptions and use type hints. After all, explicit is better than implicit.
5. Instead of using i variables in your loops use _. More information here.
6. sys.exit() is too extreme. Use return instead.
7. Some lines of your code that go after sys.exit() and break are unreachable.
8. All this spaghetti code with recursions can be rewritten in a much clearer way. Check out this answer about asking user for a valid response.

In the end it can be something like this:

import random
import string
from typing import Set
def generate_passwords(*,
 min_length: int = 8,
 max_length: int = 16,
 password_characters: str = (string.ascii_letters
 + string.digits
 + string.punctuation)
 ) -> None:
 username = ask_name()
 question = f'{username} would you like to generate a random password? '
 while True:
 if user_wants_password(question):
 password_length = random.randint(min_length, max_length)
 password = ''.join(random.choice(password_characters)
 for _ in range(password_length))
 print(password)
 question = 'Would you like to generate another random password? '
 continue
 print('Good bye!')
 return
def ask_name() -> str:
 while True:
 username = input('What is your name? ')
 if username.isalpha():
 return username
 print('Not a valid response! Try again.')
def user_wants_password(question: str,
 *,
 yes_options: Set[str] = {'Yes', 'yes', 'Y', 'y'},
 no_options: Set[str] = {'No', 'no', 'N', 'n'}) -> bool:
 while True:
 response = input(question)
 if response in yes_options:
 return True
 elif response in no_options:
 return False
 else:
 print('Not a valid response! Try again.')
if __name__ == '__main__':
 generate_passwords()

• greybeard: (Welcome to CR!) (I'd change the loop-statement in ask_name() to just if username.isalpha(): return username print('Not a valid response! Try again.').)