2
\$\begingroup\$

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")
\$\endgroup\$
  • \$\begingroup\$ Why does "reset letters/numbers" assign to symbols? Is that a bug? \$\endgroup\$ – Toby Speight Nov 13 '18 at 16:37
  • \$\begingroup\$ That's an error it should be alphabet_number(I have edited the code so people are not copying the error) \$\endgroup\$ – Thewizy Nov 19 '18 at 9:23
  • \$\begingroup\$ @Thewizy, have you considered passlib passlib.readthedocs.io/en/stable/lib/passlib.pwd.html \$\endgroup\$ – mooga Feb 21 at 8:56
4
\$\begingroup\$

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")
    
\$\endgroup\$
  • \$\begingroup\$ I fixed that formatting up a bit. Code blocks in lists are a bit weird, you need to indent them another additional level, otherwise they mess everything up. \$\endgroup\$ – Graipher Nov 13 '18 at 21:18
  • \$\begingroup\$ Thanks a lot for all those advices It really helps me to now how I can get better! \$\endgroup\$ – Thewizy Nov 16 '18 at 8:25
5
\$\begingroup\$

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.
\$\endgroup\$
3
\$\begingroup\$

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)).

\$\endgroup\$
  • \$\begingroup\$ no problem, security is hard. (I'm in a crypto class now) \$\endgroup\$ – Oscar Smith Nov 16 '18 at 8:36
  • \$\begingroup\$ I want to be a pen tester so it's a super important advice you gave me \$\endgroup\$ – Thewizy Nov 16 '18 at 9:25
-1
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Please read How do I write a good answer?: "Every answer must make at least one insightful observation about the code in the question.Answers that merely provide an alternate solution with no explanation or justification do not constitute valid Code Review answers and may be deleted." \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Feb 20 at 17:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.