# Code review for python password generator

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. • I don't think _case is a particularly good name here. Consider alphabet instead. – hammar Oct 16 '12 at 17:44 ## 3 Answers 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. • Thanks! Excellent breakdown of where I went wrong. Will keep in mind your suggestions for future codes. – Ayrx Oct 17 '12 at 1:42 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.

• Ahhh! Good point. – Ayrx Oct 16 '12 at 12:54

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)