HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Outstanding password keeper w/ password generator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
outstandinggeneratorkeeperpassword

Problem

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

"""
Task List:
Hide key after hitting enter
Create a settings GUI
Let user continuously generate a password untill satisfied
When changing password let user pick a new random password
Use string.punctuation in character list without messing up printing
Add Exclude similar characters option
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: {} '
'- Username: {} '
'- Password: {} '
' -')

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

@property
def CHARACTERS(self):
CHAR = []
if self.LOWER:CHAR.append(s

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#114915, answer score: 2

Revisions (0)

No revisions yet.