7
\$\begingroup\$

This is a budgeting script I wrote for myself that now works perfectly as planned. I just wanted the Code Review community to help me make any improvements and/or optimizations that they deem necessary.

#!/usr/bin/python
# Accounting.py - A simple command line based budgeting software with the
# ability to have data saved for, virtually, inifite amount of users.
#
# TO-DO:
# 1. Convert data storage to CSV
# 2. Add interface with Tkinter
# 3. Convert data storage to SQL

import os
import os.path
import time
import shelve
import csv
import sys
from pyautogui import alert


def main():

    # Acquire initial login data from user
    name = input('Hi! What is your name?\n  ').lower()
    pwd = input('\nHi ' + name[0].upper() + name[1:] +
                "! May I have you password as well?\n  ")

    # Create shelve object while keeping nomenclature 
    # to the specific user for easier access
    appData = shelve.open(('fad' + name))

    pw("\nInitializing your account...")
    pw("Retrieving any previous data, if any...")

    # Checking username against existing shelve data
    if (name in appData):
        pw("Data found. Checking password...")

        # If username exists, then check password against user-defined existing
        # shelve data
        while (pwd != appData['password']):
            pwd = str(input("Wrong password. Try again!\n "))

        pw("Password is correct! Logging in...")
        pw("Success!")

        # Save the specific shelve object to a variable for permanent data
        # accessibility for the user session
        user = appData[name]

    else:
        pw("No previous record found.")

        if (input("\nWould you like to create a new account?\n ")[0] == "y"):

            # Creating new user object using data provided by user at script
            # initialization
            user = Budget(name, pwd)

            # Saving customized class object to shelve object
            appData[user.user_name] = user

            pw("Creating account...")
            pw("Account created.")

        else:
            pw("I will not create an account. Exiting program...")

            # Deleting data that this script may have unintentionally created while
            # checking for username against shelve data
            try:
                os.unlink(os.path.abspath(os.curdir) + r"\fad" + name + ".dat")
                os.unlink(os.path.abspath(os.curdir) + r"\fad" + name + ".dir")
                os.unlink(os.path.abspath(os.curdir) + r"\fad" + name + ".bak")
            except FileNotFoundError:
                pass

            quit()

    pw("Your current balance is Rs." + str(appData['total']))

    try:
        while True:
            command = str(
                input("\nWhat else would you like to do?\n ")).lower()

            cc = command.split(' ')
            main = cc[0]
            cclen = len(cc)

            if (main == "deposit"):
                user.deposit(int(cc[1]))
            elif (main == "withdraw"):
                user.withdraw(int(cc[1]))

            def mainFunc(main, amount):
                if (main == "create"):
                    user.create(cc[1], amount)
                elif (main == "add"):
                    user.add(cc[1], amount)
                elif (main == "remove"):
                    user.remove(cc[1], amount)

            def askAmount():
                amount = input("Amount?")
                while int(amount) <= 0 and amount.isnum():
                    amount = int(
                        input("Sorry, please enter an integer value greater than 0."))
                return int(amount)

            if (cclen == 2):
                mainFunc(main, askAmount())

            elif (cclen == 3):
                try:
                    amount = int(cc[2])
                    mainFunc(main, amount)
                except:
                    mainFunc(main, askAmount())

            elif (main == "summary"):
                tp = [["TOTAL", ":", str(appData['total'])]]
                types = appData['types'].split('/')

                # Loop for creating a data stucture (list of lists) to provide
                # printTable() fucntion with an argument
                for i in types:
                    ts = []
                    ts.append(i.upper())
                    ts.append(":")
                    ts.append(str(appData[i]))
                    tp.append(ts)

                printTable(list(zip(*tp)))

            elif (main == "exit" or main == "quit"):
                if (sure("Are you sure?\n ") == False):
                    pw("Thank you for using this budgeting software. See you next time, " + name + ". Bye!")
                    lastUser = name
                    time.sleep(1)
                    appData.close()
                    quit()

    except Exception as error:
        pw("Sorry, an error occured: " + str(error) +
           ". I am saving your work and quitting the program...")
        appData.close()
        quit()


# A prompt-and-wait function to reduce repetition of same lines of code
def pw(string):
    print(string)
    time.sleep(0.8)

# Function to double-check user selection
def sure(string):
    yn = input(string).lower()

    if (yn[0][0] == "y"):
        return False

    elif (yn[0][0] == "n"):
        return True

# Function to prompt the user data in a presentable format
def printTable(table):
    colWidths = [0] * len(table)
    mainListLen = len(table)
    listOfListLen = len(table[0])

    for i in range(mainListLen):
        for x in table[i]:
            if (colWidths[i] < len(x)):
                colWidths[i] = len(x)

    for i in range(listOfListLen):
        for x in range(mainListLen):
            print(table[x][i].rjust(colWidths[x]), end=" ")
        print()


class Budget():

    def __init__(self, user_name, password):
        self.user_name = user_name
        self.password = password
        self.types = []
        appData['user_name'] = user_name
        appData['password'] = password
        appData['total'] = 0

        """
        The following line of code is a workaround of the fact that shelve objects can not store dictionaries.
        I have programmed it so that all the types will be in a single string, seperated by the fron-slash(/).
        When the need arises, this program will split the string and use the resulting data accordingly.
        """

        appData['types'] = "/".join(self.types)

    def deposit(self, amount):
        if amount > 0:
            appData['total'] += amount
            pw("Depositing...")
            pw("Deposit Succesful!")
            pw("Your total balance now is Rs." + str(appData['total']))

    def withdraw(self, amount):
        if (amount > appData['total']):
            print("Sorry, you do not have enough money!")

        else:
            appData['total'] -= amount
            pw("Withdrawing...")
            pw("Withdrawal Succesful!")
            pw("Your total balance now is Rs." + str(appData['total']))

    def create(self, name, amount):
        types = appData['types'].split('/')
        types.append(name)
        appData['types'] = "/".join(types)

        if (amount > appData['total']):
            print("Sorry, you do not have enough money to spend on this thing!")

        else:
            appData['total'] -= amount
            appData[name] = amount
            pw("Your total balance now is Rs." + str(appData['total']))

    def add(self, type_, amount):
        appData['total'] -= amount

        if appData[type_] > 0:
            appData[type_] += amount

        else:
            appData[type_] = 0
            appData[type_] += amount

        pw("Your total balance now is Rs." + str(appData['total']))

    def remove(self, type_, amount):
        if (amount > appData[type_]):
            print("Sorry, that's not possible!")

        elif (amount == appData[type_]):
            types.remove(type_)
            appData['types'] = "/".join(types)

        else:
            appData['total'] += amount
            appData[type_] -= amount
            pw("Tranferring Rs." + str(amount) + " back to your Total Balance...")
            pw("Transfer complete!")
            pw("Your total balance now is Rs." + str(appData['total']))

if __name__ == "__main__":
    main()
\$\endgroup\$
1
\$\begingroup\$

While it is good that your program works the way you want it to (after all, that is the goal!), your code leaves a lot to be desired.

Use of global variables

Firstly, appData is essentially a global variable. Even in your Budget class you still use appData even though it does not ever get explicitly passed to the constructor, nor is it a property of the class itself. This leads to a lot of well-documented problems.

Lack of procedural abstraction

Secondly, your main function should get broken up into smaller functions, as (in its current form) it has too many responsibilities. You do this for a lot of your code (i.e. you have pw, printTable, sure, etc.) but you need to do it for all of your code.

Hardcoding

Thirdly, you hardcode quite a few things, especially string literals you are comparing against. What if you wanted to change the desired responses from y or n to something else? You would have to find all instances of those characters in your code (which becomes tedious very quickly).

What if you wanted to change the delay between each statement? This is not as bad because there is only one place you do this (in pw), but it could still be made clearer to others looking to maintain your code.

Scoping

Also, please do not define functions inside loops or anything else - it is poor practice. Simply define your functions mainFunc and askAmount like all of your other functions.

Naming

Although minor, some of your functions and variables are named poorly. Consider the following:

  • sure
  • mainFunc
  • cc
  • tp
  • ts

Without context, it is virtually impossible to infer what they mean. This makes skimming your code very taxing.

What does sure mean? I understand how it is related to its purpose but that is not clear a priori. The same issue exists with mainFunc (except this is arguably worse, as one might confuse that for your program's entry point).

Other

This isn't really about your code directly, but when I tested your program myself, it was unclear to me what input you expected. The prevailing convention is to have something like this

Would you like to create a new account? (y/n)

Rather than simply

Would you like to create a new account?

Conclusion

While your code has a lot of room for improvement, your commenting was very good and helped me when trying to understand different fragments of your code. I also enjoyed playing around with your program and the functionality it provides is quite nice.

\$\endgroup\$
  • \$\begingroup\$ You have my gratitude for the comprehensive help that you have provided. I was truly worried over the response I would get (due to hostile encounters over on StackOverflow) but your detailed analysis put that worry to rest. I have now corrected the mistakes that you have mentioned, is it okay for me to update the code in the question, so you can give it another look to make sure that its all good now? Thanks again! \$\endgroup\$ – Arhum Ishtiaq May 21 '17 at 18:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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