12
\$\begingroup\$

Its primary function is to retrieve+copy a PW to your clipboard. It'll also add, update, delete key/value(accountName/password) pairs from a JSON formatted dictionary(separate .txt file). It's obviously insecure (passwords written in plain text). But I am less interested in security/hashing mechanisms, and more interested in making sure I am following basic best practices for scripting, command line programs, and making sure the code is pythonic in nature, as it's an exercise to learn Python and working with files.

It requires that a file named info.txt be initialized with an empty dictionary--"{}"--within the same directory as the script.

# A simple password retriever
import os
import json
import sys
import pyperclip


def addNew():

    with open(os.path.join(os.path.abspath(os.path.dirname(__file__)),
              "info.txt")) as accFile:
        ACCOUNT_DATA = json.load(accFile)

    newAcc = sys.argv[1]
    newPw = sys.argv[2]
    confirmNew = input("Add \"{0}\" with \"{1}\" to the dictionary?"
                       "\ny or n\n".format(newAcc, newPw))

    if confirmNew == "y":
        ACCOUNT_DATA[newAcc] = newPw
        print("You have added {} to your dictionary".format(newAcc))
    else:
        print("You have not added a new account")

    accString = json.dumps(ACCOUNT_DATA)

    with open(os.path.join(os.path.abspath(os.path.dirname(__file__)),
              "info.txt"), "r+") as accFile:
        accFile.write(accString)


def retrieve():

    with open(os.path.join(os.path.abspath(os.path.dirname(__file__)),
              "info.txt")) as accFile:
        ACCOUNT_DATA = json.load(accFile)

    account = sys.argv[1]

    if account in ACCOUNT_DATA:
        pyperclip.copy(ACCOUNT_DATA[account])
        print("Password for '{}' copied to clipboard.".format(account))
    else:
        print("There is no account named '{}'".format(account))


def update():

    print("An account with this name already exists.")

    accFile = open(os.path.join(os.path.abspath(os.path.dirname(__file__)),
                   "info.txt"), "r+")

    ACCOUNT_DATA = json.load(accFile, strict=False)
    confirmUpdate = input("Update '{0}' with '{1}'?\n"
                          .format(sys.argv[1], sys.argv[2]))
    if confirmUpdate == "y":
        ACCOUNT_DATA.update({str(sys.argv[1]): sys.argv[2]})
    else:
        print("Not updated")

    accString = json.dumps(ACCOUNT_DATA)
    accString.replace('“', '"')
    accFile.truncate(0)
    accFile.seek(0)
    accFile.write(accString)
    accFile.close()
    print("{} has been updated.".format(sys.argv[1]))


def delete():
    accFile = open(os.path.join(os.path.abspath(os.path.dirname(__file__)),
                   "info.txt"), "r+")

    ACCOUNT_DATA = json.load(accFile, strict=False)

    if sys.argv[1] in ACCOUNT_DATA:
        confirmDelete = input("Delete {}?\n".format(sys.argv[1]))
        if confirmDelete == "y":
            del ACCOUNT_DATA[sys.argv[1]]
            accString = json.dumps(ACCOUNT_DATA)
            accFile.truncate(0)
            accFile.seek(0)
            accFile.write(accString)
            accFile.close()
            print("{} has been removed from the dictionary.".format(sys.argv[1]))
    else:
        print("Account does not exist. Did not delete.")
        accFile.close()


def main():

    accFile = open(os.path.join(os.path.abspath(os.path.dirname(__file__)),
                   "info.txt"))
    ACCOUNT_DATA = json.load(accFile, strict=False)
    accFile.close()

    if len(sys.argv) < 2:
        print('usage: python3 {} account - copy account '
              'password\naccount: name of account whose pw to '
              'retrieve'.format(sys.argv[0]))
        sys.exit()

    elif len(sys.argv) == 3 and sys.argv[2] == "del":
        delete()
        sys.exit()

    elif len(sys.argv) == 2:
        retrieve()
        sys.exit()

    elif len(sys.argv) == 3 and not sys.argv[1] in ACCOUNT_DATA:
        addNew()
        sys.exit()

    elif len(sys.argv) == 3 and sys.argv[1] in ACCOUNT_DATA:
        update()
        sys.exit()

    elif len(sys.argv) > 3:
        print('Too many arguments passed. Try again.')

    elif sys.argv[1] == "ls":
        print("Usernames:")
        for key in ACCOUNT_DATA.keys():
            print("-", key)


if __name__ == "__main__":
    main()

Any suggestions for improvements are welcome. I'm especially interested in ways to make the code more DRY, and efficient when performing operations on a separate text file. I'm also looking for a solution to creating the info.txt through code--I think a particular weakness is the varied use of file = open() together with with open(), but this was due to an inability to properly configure w+,r+,a+ settings while attempting to read/write to info.txt.

edit: Usage from the CLI:
python3 filename.py accountName -- to copy PW to clipboard.
python3 filename.py accountName passWord -- to add a new pair.
python3 filename.py accountName del -- to remove a pair.
python3 filename.py accountName ls -- to list the keys.

edit2: I'm also curious about how to efficiently use the try/except block. It seems most programs use them, but I'm not sure how to figure out the right moments for their use.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ I know you're not looking to add one, but you wouldn't use a hash function here. Hash functions are one-way; you wouldn't be able to retrieve the password. You'd use encryption, which requires a secret key to encrypt and decrypt. \$\endgroup\$
    – jpmc26
    Aug 15, 2017 at 2:14

2 Answers 2

11
\$\begingroup\$

I would pinpoint the major problems in the code which I would address first:

  • do not manually handle provided command-line arguments - there is a great built-in module for it - argparse. It would seriously simplify the code and improve the overall readability and scalability (adding more arguments would be easy). And, it also can generate help automatically based on defined arguments.
  • document your code. Consider adding descriptive documentation strings to your functions explaining what is their purpose, what arguments they accept and what are they supposed to return.
  • fix variable and function naming. In Python, lower_case_with_underscores naming style is recommended (PEP8 reference)

Some other notes:

  • if you need to iterate over the dictionary keys, just do for key in ACCOUNT_DATA instead of for key in ACCOUNT_DATA.keys()
  • you can use json.dump() instead of json.dumps() to dump into a opened file handler directly
  • organize imports per PEP8
  • consider switching to f-strings if on Python 3.6+
\$\endgroup\$
4
\$\begingroup\$

Not many things to improve (beside of using context managers (with) instead of open() / close() as you already noted) so only some not very important suggestions:

Too many blank lines. See PEP 8 Blank Lines


newAcc = sys.argv[1]
newPw = sys.argv[2]

may become

newAcc, newPw = sys.argv[1], sys.argv[2]

confirmNew = input("Add \"{0}\" with \"{1}\" to the dictionary?"
                   "\ny or n\n".format(newAcc, newPw))

may be more clear

confirmNew = input('Add "{newAcc}" with "{newPw}" to the dictionary?'
                   '\ny or n\n'.format(newAcc=newAcc, newPw=newPw))

(using apostrophes ' to avoid escaped quotes \", and using named replacement fields)


Your main() function may be improved as this:

num_args = len(sys.argv)

if num_args not in (2, 3):
    ...
elif num_args == 2:
    ...
elif num_args == 3:
    if ...:
        ...
    elif ...:
        ...
    else
        ...
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.