7
\$\begingroup\$

I'm pretty new to Python and I made a password generator. I would like you to check it out and give me tips on how I could do it better.

import random


def small():  # Prints a generated string of 6 chars

    list = ("A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "L", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U",
    "V", "W", "X", "Y", "Z", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "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")

    generatepassword = random.choice(list) + random.choice(list) + random.choice(list) + random.choice(list) +\
               random.choice(list) + random.choice(list)

    print(generatepassword)
    print("The passwords consists of: " + str(len(generatepassword))+" Characters")
    print("\n")


def med():  # Prints a generated string of 10 chars

   list2 = ("A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "L", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U",
     "V", "W", "X", "Y", "Z", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "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")

   generatepassword = random.choice(list2) + random.choice(list2) + random.choice(list2) + random.choice(list2) +\
               random.choice(list2) + random.choice(list2) + random.choice(list2) + random.choice(list2) +\
               random.choice(list2) + random.choice(list2)

   print(generatepassword)
   print("The passwords consists of: " + str(len(generatepassword))+" Characters")
   print("\n")


def big():  # Prints a generated string of 32 chars

    list3 = ("A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "L", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U",
     "V", "W", "X", "Y", "Z", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "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")

    generatepassword = random.choice(list3) + random.choice(list3) + random.choice(list3) + random.choice(list3) +\
               random.choice(list3) + random.choice(list3) + random.choice(list3) + random.choice(list3) +\
               random.choice(list3) + random.choice(list3) + random.choice(list3) + random.choice(list3) +\
               random.choice(list3) + random.choice(list3) + random.choice(list3) + random.choice(list3) +\
               random.choice(list3) + random.choice(list3) + random.choice(list3) + random.choice(list3) +\
               random.choice(list3) + random.choice(list3) + random.choice(list3) + random.choice(list3) +\
               random.choice(list3) + random.choice(list3) + random.choice(list3) + random.choice(list3) +\
               random.choice(list3) + random.choice(list3) + random.choice(list3) + random.choice(list3)

    print(generatepassword)
    print("The passwords consists of: " + str(len(generatepassword))+" Characters")
    print("\n")

def generate():  # This askes how long the password should be

    print("How big do you want your password? choices >> [6], [10], [32]")
    choice = input("please input the lenght >> ")

    while choice != '6' and choice != '10' and choice != '32':
         choice = input("please choose: [6], [10] or [32] >> ")

        if choice == '6':
          break

        elif choice == '10':
          break

        elif choice == '32':
          break

   if choice == '6':
       small()

   if choice == '10':
       med()

   if choice == '32':
       big()

again = 'yes'
while again == 'yes' or again == 'y':

  # From here on the user can choose to generate another password
  # If the person typed yes or y then it will run the def function generate
  # And it will restart
  # If the user types anythin else then yes or y then the program quits

  generate()
  print("\n")
  print("Do you want to generate another password? [yes] or [no] >> ")
  again = input()
\$\endgroup\$
2
  • 6
    \$\begingroup\$ Please, please, please! Use the secrets module in Python 3.6.0. The random module is NOT securely encrypted enough for password generation. \$\endgroup\$
    – boboquack
    Commented Mar 12, 2017 at 6:57
  • \$\begingroup\$ There are some cases when you need the literal password. This is my case. \$\endgroup\$
    – mirapole
    Commented Jan 9, 2018 at 18:19

2 Answers 2

12
\$\begingroup\$

Summary:

  • If you have multiple function that does similar tasks, make it as one that passes an arg.
  • If you have to repetitively type the something over and over again, there's probably a better way to do it
  • Include some error checking in spots such as input()
  • Take advantage of the string module. For example: string.ascii_letters is the same as 'abcdef...ABCDEF...'

Note to the OP:

Here's a more concise version of your code. I also made it be able to take any amount of length. Read the comments cause they explains every change I made

import random
import string

def gen_pass(length):  # Prints a generated string of any length of chars

    # don't name anything list or other type name. Use a name like word_list instead
    word_list = list(string.ascii_letters+string.digits) # use string module to make it shorter, so you dont need to type everything


    generatepassword = "".join([random.choice(word_list) for i in range(length)]) # typing the same thing over and over again is redundent
                                                                        # use a variable for the length instead of making multiple functions 

    print(generatepassword)
    print("The passwords consists of: {} Characters\n".format(length)) # use str.format instead of + to make it look neater.
                                                            # isn't length the same as len(generatepassword)?                                                                                        
                                                    # add the \n on to the previous print statement

# don't need the other functions now


def generate():  # This askes how long the password should be

    print("How long do you want your password? ") # now it supports any length of password
    while 1: # do some checking, while 1 is an infinite loop until it's breaked
        try: # do some error checking as well
            choice = int(input("please input the length >> ")) # make it an integer
            if choice < 1: # should not be less than 1
                raise TypeError()

        except TypeError: # if choice < 1:
            print("Length should not be less than 1") # show an error message

        except ValueError: # if choice is not a number:
            print("Please input a valid integer") # show an error message

        except:
            print("Some other error occured... :(") # show an error message if some other error occured, though I don't think it's possible

        else:
            break # if no error occured

    gen_pass(choice) # generates a password with any length
    # everything else isn't needed 

again = 'yes'
while again.lower() in ['yes','y']: # use lower in case they capped it, use this kind of checking method
  generate()
  again = input("\nDo you want to generate another password? [yes] or [no] >> ") # put the print statements inside the input()
\$\endgroup\$
7
  • \$\begingroup\$ pep8 is always appreciated \$\endgroup\$ Commented Mar 12, 2017 at 0:12
  • \$\begingroup\$ Does pep8 involves comments? \$\endgroup\$
    – abccd
    Commented Mar 12, 2017 at 0:24
  • \$\begingroup\$ Thank you for your help en making te code much better! And as far as I know comments have to be done like this: # p \$\endgroup\$ Commented Mar 12, 2017 at 0:31
  • \$\begingroup\$ Oops I'm on my phone but comments should be done like this # sample or else i get a pep8 error thing in pycharm editor \$\endgroup\$ Commented Mar 12, 2017 at 0:32
  • 3
    \$\begingroup\$ Good stuff but there are some further improvements I would highly recommend: (1) use a generator, not a list, in "".join(...)(2) make word_list a module-level variable since it never changes (3) raise ValueError rather than TypeError (4) use while True for the loop (5) as mentioned in the other answer, use a main() function and an import guard. \$\endgroup\$
    – David Z
    Commented Mar 12, 2017 at 10:44
12
\$\begingroup\$
  1. Please read documentation, random has a usage warning at the top of the documentation:

    Warning: The pseudo-random generators of this module should not be used for security purposes. Use os.urandom() or SystemRandom if you require a cryptographically secure pseudo-random number generator.

  2. Use string formatting, this simplifys the creation of strings.

  3. Have a standard for your indentation. I'm honestly surprised this code runs with your tabbing.
  4. You can simplify your equality checks, rather than doing a == b or a == c, you can use a in {b, c}. I use a set due to the peephole optimization.
  5. Use loops, if you use for _ in range(amount) you can reduce your code significantly.
  6. Move duplicate code into one function, in this case move your prints of the password out of; small, med, and big. And put it in generate.
  7. Make a single function to be able to make any sized input.
  8. Change generate to account for this.
  9. Use a main. You want to keep things out of global scope, so it's as small as possible. This can lead to performance improvements.
  10. Protect your main function with a if __name__ == '__main__': guard.
  11. You can simplify the for loop to a list comprehension, this documentation may be easier to understand.
  12. You can define your letters as a global constant, so that if you do need them again later, you can use the one we've defined. And so if at a later date you want to add or remove letters, then you can without having to change multiple strings.
import random
r = random.SystemRandom()
r.choice()

LETTERS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890abcdefghijklmnopqrstuvwxyz"

def choice_over(letters, amount):
    choice = r.choice
    return ''.join(choice(letters) for _ in range(amount))


def generate():
    print("How big do you want your password?")
    while True:
        try:
            choice = int(input("please input the length >> "))
            break
        except ValueError:
            print('Please enter a number.')
    generatepassword = choice_over(LETTERS, choice)
    print(generatepassword)
    print("The passwords consists of: {} Characters".format(len(generatepassword)))
    print("\n")

def main():
    again = 'yes'
    while again in {'yes', 'y'}:
        generate()
        print("\n")
        print("Do you want to generate another password? [yes] or [no] >> ")
        again = input()

if __name__ == '__main__':
    main()
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Note that the only way the warning about not using random() is applicable in this situation is if the password generator is used in the same session to generate over 624 passwords which are all known to the attacker. From the 625th one the attacker can guess the next one (or ones generated previously). It also implies that the attacker can already read all the passwords in plaintext, at which point the reason for actually hacking the PNRG is a bit lost. On the other hand I agree that it's a good idea to use the secure PNRG anyhow, even if it wouldn't be strictly necessary. \$\endgroup\$
    – Voo
    Commented Mar 12, 2017 at 11:14
  • \$\begingroup\$ @Voo Would you be willing to source the 624 cycle, only as I'd be interested in reading more around the area. I don't know how it'll be used, but IMO it's better to be safe than sorry. \$\endgroup\$
    – Peilonrayz
    Commented Mar 12, 2017 at 14:41
  • 2
    \$\begingroup\$ That's just how Mersenne Twister PRNGs work. But a quick google search found this. Or to think differently about: The PRNG has a state of 2496 bytes. You can get at most 4 bytes information per call. If there is no redundant information in the state (which would make for a rather stupid algorithm), you require at least 2496 bytes of information to recreate the internal state. By the same logic you can easily figure out how many calls you need for linear congruential PRNGs \$\endgroup\$
    – Voo
    Commented Mar 12, 2017 at 14:48
  • \$\begingroup\$ @Voo In any case, you still need to seed the RNG properly. \$\endgroup\$ Commented Mar 12, 2017 at 18:03

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.