After almost completely redoing the first version I think I finally have it. I still have a few things to add/change, but I think it's pretty good and safe. I've improved security features as well as overall functionality.

I haven't run this program through pycharm yet, so there might be a few PEP 8 errors. It took me a few weeks just trying to learn about cryptography (not an easy subject). For @GarethRees post, I really didn't even start on version 2 until I implemented some sort of encryption that worked perfectly on Version 1. I tried using gpg as he recommended, but could never figure it out. I even tried it again after figuring out how to use pycrypto.

If there are any bugs just point them out you don't have to write a full review just whatever you think should be changed or added post it. I really appreciate feedback and will upvote all. If you want a .exe version just ask and I'll post a link. Hope everyone likes it.

#Programmer: DeliriousSyntax
#Date: 12-9-15
#File: AccountKeeperV2.py

#This program lets you store and create passwords

import CustomFunctions as CF
import pyperclip
import random
import shelve
import EcstaticCryption
import string

#Encrypting and Decrypting is done with pycrypto

"""
Hide key after hitting enter
Create a settings GUI
Let user continuously generate a password untill satisfied
Use string.punctuation in character list without messing up printing
Let user save settings without opening script
"""

class main:
LOWER = True
UPPER = True
NUMBERS = True
SYMBOLS = True
COPY = True

File = "Keeper.dat"

template = ('\n-  Account:  {}  '
' -')

key = input('Key: ')
EC = EcstaticCryption.AESCipher(key)

@property
def CHARACTERS(self):
CHAR = []
if self.LOWER:CHAR.append(string.ascii_lowercase)
if self.UPPER:CHAR.append(string.ascii_uppercase)
if self.SYMBOLS:CHAR.append('!#\$%&()*+,-.:;<=>?@[]^_{|}~')
if self.NUMBERS:CHAR.append(string.digits)
return CHAR

def all_accounts(self):
with shelve.open(self.File) as f:
print('\n')
for account in f:
print(account, end='  ')
print('\n')

def all_users(self, account):
with shelve.open(self.File) as f:
print('\n')
for user in f[account]:
print(user, end='  ')
print('\n')

def check_account(self, account):
list_of_accounts = []
with shelve.open(self.File) as f:
if account in f:
return True
else:
return False

list_of_users = []
with shelve.open(self.File) as f:
try:
return True
else:
pass
except KeyError:
pass
return False

holder = []
for _ in range(digits_in_pass):
temp = random.SystemRandom().choice(self.CHARACTERS)
holder.append(random.SystemRandom().choice(temp))
return ''.join(holder)

def save_account(self, entry):
with shelve.open(self.File) as f:
try:
holder_account = f[entry[0]]
except KeyError:
holder_account = {}
holder_account[entry[1]] = entry
f[entry[0]] = holder_account

def new_account(self, log=True):
print('\n')
account = input("Account: ")
with shelve.open(self.File) as f:
while True:
break
digits_in_pass = CF.valid_int("Length of password: ")
self.save_account(entry)
if self.COPY:
if log:

with shelve.open(self.File) as f:
if self.COPY:

with shelve.open(self.File) as f:
account_holder = f[account]
f[account] = account_holder
if log:

with shelve.open(self.File) as f:
if log:

confirmation = input("\nType 'DELETE' to confirm deletion of this account...\n   ->")
if confirmation.lower() == 'delete':
with shelve.open(self.File) as f:
account_holder = f[account]
try:
f[account] = account_holder
print('\nAccount deleted...')
except KeyError:
print('Error deleting account!')

print("\nAccount Found! What's next?\n"
" 1) Print account\n"
" 4) Delete account\n"
" 5) Cancel")
account_choice = input("   ->")
if account_choice == '1':
elif account_choice == '2':
elif account_choice == '3':
elif account_choice == '4':
else:
pass

def find_account(self):
account = input('\n\nAccount: ')
while account == 'all accounts':
self.all_accounts()
account = input('Account: ')
existing_account = self.check_account(account)
if existing_account:
self.all_users(account)
if existing_user:
else:
print('\nWe could not find your account.')
else:
print('\nCould not find any {} accounts.'.format(account))

def program_start(self):
"""MAIN"""
choice = None
while True:
" 2) Search for an existing account\n"
" 3) Exit")
choice = input("   ->")
if choice == "1":
self.new_account()
elif choice == "2":
self.find_account()
else:
break

if __name__ == '__main__':
print("Welcome to Account Keeper V2")
m = main()
m.program_start()

• Don't forget to accept one of the answers, or comment on why you don't find any of the current answers good enough for acception. Feb 28 '16 at 18:43
• @holroy Oh yeah thanks, just been busy and haven't check in on code review in a while. Between school, work, gym, and trying to create a big project to get noticed by employers it's hard to find time. Feb 28 '16 at 18:50

I've seen your question and would like to give you a full review, but haven't had the time yet. So now I decided to give you some of the recommendations, and then we'll see if I get the time to complete it or add to it later on.

• Move shelve handling to class level – Instead of opening/closing the shelve all the time, I would suggest to let it follow the lifetime of the class, and add a sync method to the class. This would reduce the file operations, and you can keep the shelve open within the class.

This would allow for all of the read access to be almost instant as the file is already read into memory (and the encrypted password should still be encrypted). And when doing changing password methods, you could trigger a sync, writing the shelve back to the file.

• A better class namemain is a terrible name, you should choose a better name like PasswordHandler or AccountHandler. As already suggested I would move the shelve handling to the outer level, and possibly add a few class methods. I.e. a static class method to verify if a user has entered the correct password would be nice to have.
• Allow methods not to use input – To improve the interface, I would change the methods allowing input to accept that input to be predefined as a parameter. I'll show some code for one of the functions which could accomplish this:

def change_password(self, account, username, new_password=None, log=True):

with shelve.open(self.File) as f:
if log:


This still uses the old code for the shelve handling, but you'll get the gist of the idea.

• Let methods return list, not do the actual output – Instead of letting your methods, like all_users, do the print, in general it's better to let the method returning the list of users for extra handling, i.e. printing or sorting before printing or whatever...

• Move class level code into __init__ – Doing stuff like key = input(...) is very strange, and should be moved into a method of it's own, namely the __init__, which is automatically called whenever you create an instance of your class.

There are some other minor aspects still not covered (neither in mine, nor the other answer which just came in), but these matters are some of the main issues I see in your code.

• I thought __init__ was for items that would change and not constants.? Dec 27 '15 at 21:15
• @DeliriousSyntax, __init__ is for class instanstation cases, yes. Having rogue code executed at import time is not good coding strategy. If you want them as class constants, then you should consider using static methods or similar. But having a input() at class level, which could be triggered wherevere, is a real code smell Dec 27 '15 at 21:18

This looks pretty good - I'd say the biggest opportunities for improvement are in style, ease of i18n, and documentation (I have no idea if any of these have been mentioned in a previous post, as I haven't read it).

I'm going to start with documentation - generally, most of your methods and parameter names are fairly straightforward, however if you write useful docstrings (I appreciate the numpydoc standard) you can use a tool like sphinx to create documentation (if you think anyone else might ever want to use your tool as a developer). Even if you don't think anyone else will ever program with this, writing documentation will help you understand it later.

On that note, your module as a whole should probably have a brief docstring describing its overall purpose.

I don't know that I like how hardcoded some things are (for example LOWER, UPPER, File, etc). It would be nice if these could be set somewhere, or passed as CLA, or in a config file, etc.

In check_account you don't ever use list_of_accounts, and you could just return account in f instead of having the if statements. Likewise, in check_username you don't use list_of_users, and you could probably simplify the conditions. I'm not familiar with shelve, but assuming it quacks like a dict you could just do return username in f.get(account, {}) and then avoid the try-except, the if-else, etc. If you don't want to do that, you don't need the else:pass at all.

You could probably turn generate_password into a list comprehension.

def generate_password(self, digits_in_pass):
func = random.SystemRandom().choice
return ''.join(func(func(self.CHARACTERS)) for _ in digits_in_pass)


This should be equivalent.

In save_account you could (presumably) use f.get() instead of the try-except.

In all of the places you use input I'd rather see you take that input as a parameter - this separates concerns in case you want to stop asking on the command line, and lets you build out whatever front-end you want.

In new_account you can use single quotes instead of escaping the double quotes, which is cleaner IMHO. I'd also strip the string in case they have whitespace on the ends, and if they have something close to random (use difflib) maybe emit a warning in case they misspelled it.

You have lots of magic numbers - maybe instead of indexing at 0, 1, and 2, use ACCOUNT_INDEX = 0 (or something). Then if you change the order, or use a new scheme, its easier to change it.

program_start shouldn't be a part of the class - it should be another function that makes use of the class.

Internationalization (i18n) is important - what if someone wanted to use a different language? I'd say that you should create a class (or something) that has all of the string literals you use as properties, and then returns them based on some locale or language that has been set. Then you can add translations easily, if you wanted to expand your userbase.

In terms of style - I don't really like the onelineing in CHARACTERS - you aren't losing points for adding some newlines, or at least spaces.

Your naming could use a little work - a class named main is not even a little bit helpful. I'd recommend something like AccountKeeper or PasswordManager or something like that. CHARACTERS is also not as helpful - maybe VALID_CHARACTERS would be better? Also, File isn't as helpful - either make it a constant as FILE or a variable as file.

all_accounts implies that you should get a list or something - calling it display_all_accounts is more honest - same for all_users.

Constants should be named in UPPER_SNAKE_CASE, classes in PascalCase, method/function/variable names in lower_snake_case`. You can read more about naming and style conventions in PEP8