Password generator script

Question

What I was trying to do here was create a Python script that would generate passwords with 16 characters with uppercase and lowercase letters, and numbers. The generated password would then write itself to a file.

Let me know what you think because I know they say the point of Python is to be able to write powerful scripts that can do what other languages can do in a few lines. I feel like this is probably way more then it should be for what it's doing.

import random
import string
print "Welcome to PassBook!"
def genPasswd():
 global route
 global password
 global username
 global account
 global entry
 account = raw_input("Account: ")
 username = raw_input("Username: ")
 key1 = random.randrange(10,100)
 key2 = random.randrange(10,100)
 key3 = random.randrange(10,100)
 key4 = random.randrange(10,100)
 keya = str(random.choice(string.letters) + random.choice(string.letters))
 keyb = str(random.choice(string.letters) + random.choice(string.letters))
 keyc = str(random.choice(string.letters) + random.choice(string.letters))
 keyd = str(random.choice(string.letters) + random.choice(string.letters))
 key1a = str(key1) + str(keya)
 key2b = str(key2) + str(keyb)
 key3c = str(key3) + str(keyc)
 key4d = str(key4) + str(keyd)
 password = str(key1a + key2b + key3c + key4d)
 entry = "Account: " + account + " - Username: " + username + " - Password: " + password
 file = open("/home/meta/Desktop/Python2/passbook/passbook.txt", "a")
 file.write("\n" + entry + "\n")
 print entry
 return key1
 return key2
 return key3
 return key4
 return keya
 return keyb
 return keyc
 return keyd
 return key1a
 return key2b
 return key3c
 return key4d
 return password
 return username
 return account
 return entry
def queryPb():
 search = raw_input("For which account are you searching: ")
 read = open("/home/meta/Desktop/Python2/passbook/passbook.txt", "r")
 for line in read:
 if search in line:
 print line
 return line
 else:
 print "I'm sorry we could not find any account related to " + search
def takeRoute():
 global route
 route = raw_input("Press 1 to generate a password, Press 2 to search for an existing account: ")
 if int(route) ==1:
 genPasswd()
 elif int(route) == 2:
 queryPb()
 else:
 print "I'm sorry that is not an option!"
 exit()
takeRoute()

Answer 1

Your sentiment is indeed correct, this code is much longer than it needs to be and can be shortened 10x while retaining the same functionality.

---

You are doing 4 things in a single function!

1. Asking for input
2. Generating a password
3. Printing
4. Writing to a file.

A function should do 1 thing only.

Let's say we want to generate a password:

def generate_password():
 numbers = (str(random.randint(10, 99)) for _ in range(4))
 letters = (random.choice(string.letters) for _ in range(4))
 return ''.join(numbers) + ''.join(letters)

This function is shorter than yours not only because it does not care about Input/Output, but also because it uses loops (Generator expressions here) to do the same thing many times.

You may call this function in a user-interface that handles input and output like:

print(raw_input("Username ") + " " + raw_input("Surname ") + ": " + generate_password())

Of course you may want to make the interface nicer or make it handle files, but the backbone is here.

Answer 2

You have some bad habits here, so even though Caridorc has a better function for you, I'll highlight some general ideas to improve on. For a start, you have key1, key2, key3 and key4. All the same name and method of getting a value. Instead you should make a list of 4 values and call the list keys. You also call str on every value in the list, so you should just create them as strings from the get go. You can use a list comprehension to make your list of random strings. A list comprehension is like a for loop that gets condensed into an expression, here's how it might look:

keys = [str(random.randrange(10,100)) for _ in range(4)]

for _ in range(4) is a very simple for loop that basically just runs 4 times. range(4) creates a list of 4 values, so iterating over it gives you the 4 times you need. Using _ just signals that the value isn't used.

You could do the same for keya through keyd, but you've also misunderstood the str function. str is called to change other types to strings, like ints or floats. But string.letters already just is a string, and a random.choice from it will still be a string, so you don't need the value here and you definitely don't need it when creating key1a and the rest of your joined values.

Speaking of string.letters, that's a good use of the string module. Saves you manually declaring a list of characters and will help localisation since it includes other characters from a user's locale (eg. áéíóú), though be careful that some password fields might be limited to ascii characters. In that case, you could use ascii_letters instead. But also when importing, there's a couple tricks you can use to save some typing. If you instead use from string import letters (or from string import ascii_letters) then you don't need to repeatedly type string.letters, you could just access it with letters. Like this:

from string import letters
 ...
 keya = random.choice(letters) + random.choice(letters)

Next, you're using global and really shouldn't. global is only there for rare cases, most of the time you can pass values in other ways, using either arguments or returning values. However, in your particular case you don't need them at all. route is never used inside genPasswd, and all your other values are entirely just read and defined within the program. If you remove all these globals it will have no effect on your code whatsoever.

Likewise you use return to return values that you don't need. Only the first return call will actually return anything, because as soon as you call return then the program ends. If you did want to return all the values, you'd need to have them on one line. But you really don't need all the variables to be returned. You can definitely skip the keys you used to make up the password, and the entry value is easily recreated with password, username and account. So at most you might return this:

return password, username, account

Though since it's a password generating function it should only return password really, the username and account should be handled in another function.

You're also opening files in functions that you never close, this is a very bad idea. You should always close files when you're done with them. You could use file.close(), but there's a syntax that will automatically close files for you:

with open("/home/meta/Desktop/Python2/passbook/passbook.txt", "a") as f:
 f.write("\n" + entry + "\n")
print entry

This is the with open() as file_name syntax. It automatically closes your file no matter what happens. Everything that's indented after it will have the file open, but once the indented block ends it will close the file. You notice I changed the name to f too, as file will replace the name of a builtin function. I also think you should store the filepath as a constant near the top, named something like FILEPATH so you can just call open(FILEPATH).

One last note, you print your not found message for every line that search isn't found. Just move it outside the for loop. As I explained above, as soon as return is reached the function ends, so it will only leave the for loop if search wasn't found in any of the lines.

for line in read:
 if search in line:
 print line
 return line
print "I'm sorry we could not find any account related to " + search

• mkrieger1: "A list comprehension is like a for loop that gets condensed into one line" - it's not a matter of the number of lines, but it's a single expression. You can very well spread a list comprehension over multiple lines.
• SuperBiasedMan: @mkrieger1 Good point, I'll make sure to focus on it being an expression in future. Thanks!