Code review for python password generator

Question

The following code is a simple random password generator that spits out a randomized password based on some input flags.

import string
import random
import argparse

def gen_char(lower, upper, digit, special):

    _lower_letters = string.ascii_lowercase
    _upper_letters = string.ascii_uppercase
    _digits = string.digits
    _special = "!@#$%^&*()"

    _rand = random.SystemRandom()

    _case = ""

    if lower:
        _case += _lower_letters

    if upper:
        _case += _upper_letters

    if digit:
        _case += _digits

    if special:
        _case += _special


    if _case:
        return _rand.choice(_case)

    else:
        return _rand.choice(_lower_letters+_upper_letters+_digits+_special)

def main():

    """

    The following lines of code setup the command line flags that the
    program can accept.

    """

    parser = argparse.ArgumentParser()

    parser.add_argument("length", type=int, help="length of password")
    parser.add_argument("--lower", "-l", 
                       help="Generates passwords with lower case letters",
                       action="store_true")
    parser.add_argument("--upper", "-u",
                       help="Generates passwords with upper case letters",
                       action="store_true")
    parser.add_argument("--digit", "-d",
                       help="Generates passwords with digits",
                       action="store_true")
    parser.add_argument("--special", "-s",
                       help="Generates passwords with special characters",
                       action="store_true")

    args = parser.parse_args()


    """

    The following lines of code calls upon the gen_char() function to
    generate the password.

    """
    _pass = ""

    for x in range(args.length):

        _pass += gen_char(args.lower, args.upper, args.digit, args.special)

    print _pass

main()

The code is in working condition. I'm looking for tips in terms of coding styles, readability and perhaps optimizations if any.

If anyone is interested, the code can be found on github.

Answer 1

import string
import random
import argparse

def gen_char(lower, upper, digit, special):

    _lower_letters = string.ascii_lowercase
    _upper_letters = string.ascii_uppercase
    _digits = string.digits
    _special = "!@#$%^&*()"

Its against python convention to prefix local variables with _. Just don't do it. All of these categories would be better in a dictionary. That is create a global dict like:

CHARACTER_CATEGORIES = {
   'lower' : string.ascii_lowercase,
   'upper' : string.ascii_uppercase,
   'digits' : string.digits,
   'special' : "!@#$%^&*()"
}

Then take a list of categories as your parameters to this function instead.

    _rand = random.SystemRandom()

You should create a random number generator once in your program, not once per random choice.

    _case = ""

case? why case?

    if lower:
        _case += _lower_letters

    if upper:
        _case += _upper_letters

    if digit:
        _case += _digits

    if special:
        _case += _special

    if _case:
        return _rand.choice(_case)

    else:
        return _rand.choice(_lower_letters+_upper_letters+_digits+_special)

this is really the wrong place to do this. the decision to include all categories if non are specifically selected is part of interpretating the user's input. So you should do it when you are parsing that, not here.

def main():

    """

    The following lines of code setup the command line flags that the
    program can accept.

    """

Not a very useful comment, because its pretty obvious that's whats happening here.

    parser = argparse.ArgumentParser()

    parser.add_argument("length", type=int, help="length of password")
    parser.add_argument("--lower", "-l", 
                       help="Generates passwords with lower case letters",
                       action="store_true")
    parser.add_argument("--upper", "-u",
                       help="Generates passwords with upper case letters",
                       action="store_true")
    parser.add_argument("--digit", "-d",
                       help="Generates passwords with digits",
                       action="store_true")
    parser.add_argument("--special", "-s",
                       help="Generates passwords with special characters",
                       action="store_true")

    args = parser.parse_args()


    """

    The following lines of code calls upon the gen_char() function to
    generate the password.

    """
    _pass = ""

    for x in range(args.length):

        _pass += gen_char(args.lower, args.upper, args.digit, args.special)

Its not very efficient to do it this way as other have pointed out. But its also bad because adding strings in python is expensive, better to put all the letters in a list and join them.

    print _pass

main()

Better to do

if __name__ == '__main__':
   main()

That way you could import this module elsewhere, and use its features.

Answer 2

Re-assembling the _case character list for each character is rather wasteful. I would get rid of the gen_char function.

import string
import random
import argparse

def main():

    """

    The following lines of code setup the command line flags that the
    program can accept.

    """

    parser = argparse.ArgumentParser()

    parser.add_argument("length", type=int, help="length of password")
    parser.add_argument("--lower", "-l", 
                       help="Generates passwords with lower case letters",
                       action="store_true")
    parser.add_argument("--upper", "-u",
                       help="Generates passwords with upper case letters",
                       action="store_true")
    parser.add_argument("--digit", "-d",
                       help="Generates passwords with digits",
                       action="store_true")
    parser.add_argument("--special", "-s",
                       help="Generates passwords with special characters",
                       action="store_true")

    args = parser.parse_args()


    """

    The following lines of code generate the password.

    """

    _lower_letters = string.ascii_lowercase
    _upper_letters = string.ascii_uppercase
    _digits = string.digits
    _special = "!@#$%^&*()"

    _case = ""

    if lower:
        _case += _lower_letters

    if upper:
        _case += _upper_letters

    if digit:
        _case += _digits

    if special:
        _case += _special

    if !_case:
                _lower_letters+_upper_letters+_digits+_special

    _rand = random.SystemRandom()

    _pass = ""

    for x in range(args.length):

        _pass += _rand.choice(_case)

    print _pass

main()

You could, perhaps, use a function to make the list the one time.

Answer 3

In addition to what has been said about not calling gen_char() every iteration, you are prefixing your variable names with an underscore, which is the python standard for private variables - the issue here is that you are doing this inside a function definition - as these are local variables that will go out of scope and get garbage collected when the function ends, there really isn't much point. I would recommend just using normal variable names.

I would also argue gen_char() could be improved by using default values:

def gen_char(lower=string.ascii_lowercase, upper=string.ascii_upercase,
             digit=string.digits, special="!@#$%^&*()"):
    rand = random.SystemRandom()
    return rand.choice("".join(
               group for group in (lower, upper, digit, special) if group)

This makes it clear what the default is, makes it possible to change the character range used, and makes it easy to disable certain parts.

It's noted that str.join() is preferable over repeated concatenation, and using it allows us to stop repeating ourself and say that if we get a value, use it.

This would require changing your use of argparse, prehaps like so:

kwargs = {}
if not args.lower:
    kwargs["lower"] = False
...
gen_char(**kwargs)