3
\$\begingroup\$

After coding this, I was wondering if there are any ways in which I can improve upon it. It generates a random string of characters and symbols the same length as what is entered by the user. It uses a separate list for each char list but I am sure it is using a very long-winded method!

import random, sys
letters = ["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"]
lettersnum = ["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","0","1","2","3","4","5","6","7","8","9"]
letterssym = ["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","#","*", "£", "$", "+", "-", "."]
lettersnumsym = ["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","0","1","2","3","4","5","6","7","8","9","#", "*", "£", "$", "+", "-", "."]


def generator(length, num, sym, caps):
    count = 0
    password = ""
    try:
        test = int(length)
    except ValueError:
        error("LENGTH NOT VALID NUMBER")
    if num.lower() == "yes" and sym.lower() == "yes":
        merge = lettersnumsym
    elif num.lower() == "yes":
        merge = lettersnum
    elif sym.lower() == "yes":
        merge = letterssym
    while count <= int(length):
        password = password + merge[random.randint(0, len(merge) - 1)]
        count = count + 1
    if caps.lower() == "uppercase":
        password = password.upper()
    elif caps.lower() == "lowercase":
        password = password.lower()
    print("PASSWORD:",password)

def error(error):
    print(error)
    sys.exit(0)

running = True
while running == True:
    length = input("How long do you want the password?")
    numbers = input("Do You Want It to Include Numbers?")
    symbols = input("Do You Want It To Include Symbols?")
    capital = input("Do You Want It To be uppercase or lowercase??")
    generator(length, numbers, symbols, capital)
    restart = input("Do You Want Restart?")
    restart = restart.lower()
    if restart in ("yes", "y", "ok", "sure", ""):
        print("Restarting\n----------------------------------")
    else:
        print("closing Down")
        running = False
\$\endgroup\$
6
\$\begingroup\$

Python already defines a number of strings of possible characters. See string.ascii_lowercase and string.digits Source


I would use True and False instead of "yes"/"uppercase" as arguments to generator(). This function might be used by code that does directly interact with a user, so passing a string would not make sense. Additionally, the restart prompt supports a number of positive responses that are not supported by this function. You should have one layer that controls interaction with the user and one that generates a password. This will make the function cleaner as well as an easier API to work with.


Two more point about separation of concerns with generator():

  • It should return the password instead of printing it. Again, this allows the function to be used when not directly interacting with a command prompt.

  • It should throw an exception instance of calling sys.exit(). exit() will stop the python process and not allow any more execution of code. Your code is written so that multiple passwords can be generated one after another. However, if the user accidentally enters an invalid character to the first question, the application stops running instead asking the user to input a correct value. Throwing an exception would have the same result if you don't change the rest of your code, but allows you to change the code that interacts with the user to handle this case without restarting the application.


You convert length to an integer repeatedly instead of storing the value. The first validation completely ignores the result and the while loop does the conversion every time it tests if it should continue looping. This can all be solved by having length be passed in as an integer and letting the user layer handle the conversion and error cases.


Generator is already a well defined term with in Python that do something very different. The function should be renamed to generate_password() or something similar.

\$\endgroup\$
3
\$\begingroup\$

You are still not following the advice offered in previous answers to your questions.
Quoting from Edward's great answer on "Are there any bugs or ways to make my divisor code better?":

Separate I/O from calculation

The main purpose of the program is to factor numbers into divisors which is something that is potentially reusable. Both to make it more clear as to what the program is doing and to allow for future re-use, I'd suggest extracting the factoring part into a separate function and the have the input from the user be done within the main routine or some other input-only function.

Your password generating function, sadly, asks the user for input and thus can never be re-used outside the context of this application. Instead, make it take exactly two arguments as the only inputs: a string containing the characters to base the password on, and the length of the password:

def make_password(length, alphabet):
    return ''.join(random.choice(alphabet) for _ in range(length))

Here, random.choice(alphabet) replaces merge[random.randint(0, len(merge) - 1)]
in a more readable way.

Another repeat problem, quoting from the same answer:

Think of your user

Right now, the user has to enter "yes" or the equivalent and then enter another number to be factored. However, the prompt doesn't suggest that "y" is a valid answer. Adding that to the prompt would help the user understand what the computer is expecting. Better still, would be t eliminate that question and to simply ask for the next number with a number '0' being the user's way to specify no more numbers. Of course the prompt should be changed to tell the user of this fact.

Your user interface is basically the same: it would drive anyone who used it regularly up the wall. This is a command-line program; throw away all the input prompting and give it a command-line argument interface instead:

usage: makepwd.py [-h] [-d] [-s] [-l | -u] length

Generates passwords of the specified length, optionally including digits
and/or symbols.

positional arguments:
  length         a positive integer denoting the password length

optional arguments:
  -h, --help     show this help message and exit
  -d, --digits   include digits in the generated password
  -s, --symbols  include symbols in the generated password
  -l, --lower    use only lowercase letters
  -u, --upper    use only uppercase letters

The argparse module can take care of this for you, but you need to familiarize yourself with its semantics to understand what is going on. Let's take a look at every part individually:

def parse_args():
    parser = argparse.ArgumentParser(description=__doc__, argument_default='')
    parser.set_defaults(letters=string.ascii_letters)

We've created an argument parser and provided it the docstring of our module to use a description. In your original code, lower-case characters were used as a default. This is a bad default, instead use both lower and upper case if nothing else is specified.

    parser.add_argument('length', type=int,
                        help='a positive integer denoting the password length')

The first argument we need is the length of the password. argparse will convert the value to an int and take care of the error handling for us.

    add_const_arg = arg_adding_function_for(parser)
    add_const_arg('-d', '--digits', const=string.digits,
                  help='include digits in the generated password')
    add_const_arg('-s', '--symbols', const='#*£$+-.',
                  help='include symbols in the generated password')

Our next two arguments determine the non-alphabetical characters to include in the password.

    group = parser.add_mutually_exclusive_group()
    store_letters = arg_adding_function_for(group, dest='letters')
    store_letters('-l', '--lower', const=string.ascii_lowercase,
                  help='use only lowercase letters')
    store_letters('-u', '--upper', const=string.ascii_uppercase,
                  help='use only uppercase letters')

And the final arguments are for overriding the mixed-case default, so it's possible to generate a password containing only lower-case or only upper-case letters (in addition to the digits and symbols). These arguments are mutually exclusive: a password can not be upper-cased and lower-cased at the same time.

    return parser.parse_args()

And that's it. Almost. You may have noticed I didn't explain the arg_adding_function_for function yet. I defined it as the following higher-order helper function to simplify the above code by using functools.partial to pre-set some of the options that are common for each argument. (For flexibility, the *args parameter is included, though not technically necessary - find out more about *args and **kwargs).

def arg_adding_function_for(parser, *args, action='store_const', **kwargs):
    return functools.partial(parser.add_argument, action=action, *args, **kwargs)

The whole thing in once piece:

"""
Generates passwords of the specified length, optionally including digits and/or symbols.
"""

import argparse
import functools
import random
import string


def main():
    args = parse_args()
    password = make_password(args.length, args.letters + args.digits + args.symbols)
    print(password)


def make_password(length, alphabet):
    return ''.join(random.choice(alphabet) for _ in range(length))


def parse_args():
    parser = argparse.ArgumentParser(description=__doc__, argument_default='')
    parser.set_defaults(letters=string.ascii_letters)

    parser.add_argument('length', type=int,
                        help='a positive integer denoting the password length')

    add_const_arg = arg_adding_function_for(parser)
    add_const_arg('-d', '--digits', const=string.digits,
                  help='include digits in the generated password')
    add_const_arg('-s', '--symbols', const='#*£$+-.',
                  help='include symbols in the generated password')

    group = parser.add_mutually_exclusive_group()
    store_letters = arg_adding_function_for(group, dest='letters')
    store_letters('-l', '--lower', const=string.ascii_lowercase,
                  help='use only lowercase letters')
    store_letters('-u', '--upper', const=string.ascii_uppercase,
                  help='use only uppercase letters')
    return parser.parse_args()


def arg_adding_function_for(parser, *args, action='store_const', **kwargs):
    return functools.partial(parser.add_argument, action=action, *args, **kwargs)


if __name__ == '__main__':
    main()
\$\endgroup\$
1
\$\begingroup\$

You are trying to generate a complex password right? Why can't you just use string.py modules built in constants? Something like below. You can take in length or any other parameter as input from user.

import random, string

def gen_passwrd(length):
  count = 0
  password = ""
  while count <= length:
    password = password + random.choice(string.letters + string.digits + string.punctuation)
    count = count +1
print password, "Length = ", length

gen_passwrd(12) 
\$\endgroup\$

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.