Password generator in Python 3.7

Question

I am learning how to code in Python by myself and have never coded before, I just want to know if there is any way I can upgrade my PasswordGenerator.

#!/usr/bin/env python3
import random


def passwordgenerator():
    mypw = ""
    count = 0
    alphabet_number =    "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"
    symbols = "!?@%#$"
    running1 = True
    while running1:

        if len(symbols) == 1:

            #  Reset symbols
            symbols = "!?@%#$"

        if len(alphabet_number) == 1:

            #  Reset letters/numbers
            alphabet_number = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"

        elif count == pw_length:

            if mypw.isupper() or mypw.islower():  # check if there is only upper     or only lower letter and rerun if True
                passwordgenerator()

            else:
                x = mypw
                y = list(x)  # creates a list and put the pw in to shuffle it
                random.shuffle(y)
                mypw = "".join(y)

                print(mypw)
                input("\nPress <Enter> to close the program")
                running1 = False

        elif count % 4 == 0:

            #  pick a random symbol every 3 loop and add it to the pw
            symbols_index = random.randrange(len(symbols))
            new_symbols = symbols[symbols_index]
            mypw = mypw + new_symbols

            # delete the symbols that is picked so there are no duplicate
            symbols_list_change = symbols.replace(new_symbols, "")
            symbols = symbols_list_change

        else:

            # pick a random number or letter and add it to the pw
            next_index = random.randrange(len(alphabet_number))
            new_alphabetnumber = alphabet_number[next_index]
            mypw = mypw + new_alphabetnumber

            # delete the symbols that is picked so there are no duplicate
            an_list_change = alphabet_number.replace(new_alphabetnumber, "")
            alphabet_number = an_list_change

        count += 1


if __name__ == '__main__':
    print("/!\ 12 Characters is a minimum for good security /!\ ")

    print("=" * 55)  # just to make it pretty

    running = True
    while running:
        pw_length = input("How many Characters do you want?\n")

        if pw_length.isdigit():  # verify if user input a number
            pw_length = int(pw_length)
            passwordgenerator()
            running = False
        else:
            print("A number is needed")

Answer 1

Thanks for posting your code, it's great that you want to improve your style. I created a small list of things you could improve. This list is probably not complete and different programmers may have different opinions but I tried to my best to be constructive and unopinionated.

1. some_var = True
   while some_var:
       # Do something
       some_var = False
   

   Is usually written as

   while True:
       # Do something
       break
   

2. Global variables can make it harder to reason about functions. So instead of globally defining pw_length, just pass it as an argument to passwordgenerator.

3. It is a good rule of thumb that functions should only have one responsibility, so instead of printing the password in passwordgenerator you should just return it and print it in you main function.

4. random.choice selects a random element from a list and random.sample selects k unique elements from a list.

5. Not allowing repetition makes the password more predictable. The same goes for using symbols only at specific locations. So my final version would be:

   def passwordgenerator(pw_length):
       ALPHABET = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ!?@%#$"
       return ''.join([random.choice(ALPHABET) for _ in range(pw_length)]
   
   if __name__ == '__main__':
       print("/!\ 12 Characters is a minimum for good security /!\ ")
   
       print("=" * 55)  # just to make it pretty
   
       while True:
           pw_length = input("How many Characters do you want?\n")
   
           if pw_length.isdigit():  # verify if user input a number
               pw_length = int(pw_length)
               mypw = passwordgenerator(pw_length)
               print(mypw)
               break
           else:
               print("A number is needed")
   

Answer 2

Not bad a all for a first Python program:

• Good use of the line: if __name__ == '__main__': .
• Good use of string methods (replace, isupper, islower etc...).
• Good use of the random module methods.

But some things can be made better:

• The function passwordgenerator could have pw_length as a parameter and return mypw. (The printing of the password moves to the main part in this case)
• calling passwordgenerator from within passwordgenerator itself (a recursive call) can be tricky (what is the password accidentally is always lower...?)
• The string constants "!?@%#$" and "abc...XYZ" occur both twice, they could be made constants on top of the program.
• I think the line: elif count == pw_length should be different
• a docstring could be added to passwordgenerator explaining the properties of the generated password.

Answer 3

One really important and common error this makes is using random.

random is sufficient for applications where security isn't needed, but for cases like this, you really should use the secrets module. This will generate numbers that are less predictable, at the expense of being slightly slower. Unfortunately, this does make some bits of your program harder, as the module does not provide some of the convenience methods of random, but the methods aren't too hard to write yourself.

For example, symbols_index = random.randrange(len(symbols))becomes secrets.randbelow(len(symbols)).

Answer 4

Instead of writing out those long strings of characters, it might be better to use the predefined constants. For example:

alphabet_number = string.ascii_lowercase +
                  string.ascii_digits +
                  string.ascii_uppercase

That way, you're saved from mistyping the list, and it's clearer to readers what's included.