5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I don't think _case is a particularly good name here. Consider alphabet instead. \$\endgroup\$ – hammar Oct 16 '12 at 17:44
6
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ Thanks! Excellent breakdown of where I went wrong. Will keep in mind your suggestions for future codes. \$\endgroup\$ – Ayrx Oct 17 '12 at 1:42
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Ahhh! Good point. \$\endgroup\$ – Ayrx Oct 16 '12 at 12:54
5
\$\begingroup\$

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