6
\$\begingroup\$

As a beginner (Learn Python the Hard Way), I've set myself this exercise with classes and it works but looks very inelegant to me. I'm trying to write classes which could be re-used in other code. I've left long variable names and comments so you can see what I thought I was doing. What should I focus on in my study to develop better practice and where can I slim down the code please?

from os.path import exists
from sys import exit
import json
import time


# Create a database of names and birth dates
# Print data to screen with age at birthday of current year
# Edit or delete records
# Save to file

class Filing(object):
# check for file and create if necessary

    def __init__(self, filename):
# filename argument is required
        self.filename = filename

    def check_create(self, filename):
# method checks for existence of file and if not, creates empty file 

        if exists(self.filename) == True:
            print """\nYour data is stored in a file named '%s'\n""" %self.filename
        else:
#create file and open in append mode - pointer at end of file
            data_file = open(self.filename, 'a+')
            print """\nYour data will be stored in a file named '%s'\n""" %self.filename
            data_file.close()

    def open_file_read_to_variable(self):
# method reads file to dict variable and returns - if file empty an empty dict is returned

        try:
            unjson_file = open(self.filename, 'r')
            self.data = json.load(unjson_file)

            unjson_file.close()
            return self.data
        except ValueError:
            self.data = {}
            return self.data    

    def save_data(self, data, filename):
# takes variable dict and target file arguments
        self.data = data
        self.filename = filename
        json_output = open(self.filename, 'w')
        json.dump(self.data, json_output)
        print "Data stored in file: %s" %self.filename
        json_output.close() 

class DataHandling(object):
# do not create instance of DataHandling until 
# data = filing.open_file_read_to_variable() has run

    def __init__(self):
        self.data = data        

    def menu(self, data):

        local_data_work = DataHandling()
        while True:
            x = raw_input("Press Y to add to your database, E to edit, D to display records or any other key to quit: > ")
            if x == "Y":
                self.data = local_data_work.add_data(self.data)
            elif x == "E":
                data_work.edit_data(self.data)
            elif x == "D":
                data_work.print_output_to_screen(data)
            elif x == "Q":
                data_work.print_output_to_screen(data)                
                filing.save_data(data, birth_data_file)
                exit()
            else:
                pass

    def get_input_int(self, prompt, min, max):
        self.prompt, self.min, self.max = prompt, min, max
    #check raw_input is an integer of correct length and return 
    #takes prompt parameter
        while True:
            try:
                self.num = int(raw_input("%s" %self.prompt)) 
                if self.min <= self.num <= self.max:
                    return self.num

            except ValueError:
                print "Error. Please enter a number of the correct length."


    def add_data(self, data):

        self.name = raw_input("Enter the name you wish to add to the database: > ")
        self.day = data_work.get_input_int("Date of birth: dd  > ", 1, 31)
        self.month = data_work.get_input_int("Date of birth: mm > ", 1, 12)
        self.year = data_work.get_input_int("Date of birth: yyyy > ", 1900, 2100)

        self.data[self.name] = [self.day, self.month, self.year]
        return self.data        

    def edit_data(self, data):

        local_data_work = DataHandling()
        local_data_work.print_output_to_screen(self.data)
    #self.name = raw_input("Enter the name you wish to edit or delete: > ")
        while True:
            x = raw_input("Press 1 to change your data, 2 to delete a record >")
            if x == "1":
                print "Re - enter the name and data"
                local_data_work.add_data(data)
                return data

            elif x == "2":           
                del_ = raw_input("Type name to be deleted > ")
                del data[del_]
                return data

            else:
                print "Not understood"
                pass    


    def print_output_to_screen(self, data):
# prints dict keys and data

        self.data = data
        print ""
        for i in sorted(self.data): 
            print "%s" % i,
            print "%d   %d  %d Age at birthday this year is %d" %(self.data[i][0], self.data[i][1], self.data[i][2], (int(time.strftime("%Y")) - data[i][2]))   

#START

birth_data_file = "birth_data"
#for user defined filenames use instead:
#birth_data_file = raw_input("Type an existing storage file or a new file name > ")

filing = Filing(birth_data_file)
filing.check_create(birth_data_file)
data = filing.open_file_read_to_variable()

data_work = DataHandling()
# do not create instance of DataHandling until 
# data = filing.open_file_read_to_variable() has run

data_work.menu(data)

data_work.print_output_to_screen(data)

filing.save_data(data, birth_data_file)
\$\endgroup\$
  • 1
    \$\begingroup\$ Yes, it is the right kind of question. Welcome to Code Review! \$\endgroup\$ – Phrancis Sep 29 '14 at 15:46
4
\$\begingroup\$

Style

Python has a style guide known as PEP8. You'll find various tools to check your code against it : there is a command-line tool called pep8 but you can also find it online. You'll also find a tool to try to fix the issue automatically : autopep8 but it's definitly worth having a look at the PEP 8 document anyway.

Among the different problems in your code : spacing, line length and comparison to True with ==.

Once this is fixed, your code becomes :

from os.path import exists
from sys import exit
import json
import time


# Create a database of names and birth dates
# Print data to screen with age at birthday of current year
# Edit or delete records
# Save to file

class Filing(object):
    # check for file and create if necessary

    def __init__(self, filename):
        # filename argument is required
        self.filename = filename

    def check_create(self, filename):
        # method checks for existence of file and if not, creates empty file

        if exists(self.filename):
            print """\nYour data is stored in a file named '%s'\n""" % self.filename
        else:
            # create file and open in append mode - pointer at end of file
            data_file = open(self.filename, 'a+')
            print """\nYour data will be stored in a file named '%s'\n""" % self.filename
            data_file.close()

    def open_file_read_to_variable(self):
        # method reads file to dict variable and returns - if file empty an
        # empty dict is returned

        try:
            unjson_file = open(self.filename, 'r')
            self.data = json.load(unjson_file)

            unjson_file.close()
            return self.data
        except ValueError:
            self.data = {}
            return self.data

    def save_data(self, data, filename):
        # takes variable dict and target file arguments
        self.data = data
        self.filename = filename
        json_output = open(self.filename, 'w')
        json.dump(self.data, json_output)
        print "Data stored in file: %s" % self.filename
        json_output.close()


class DataHandling(object):
    # do not create instance of DataHandling until
    # data = filing.open_file_read_to_variable() has run

    def __init__(self):
        self.data = data

    def menu(self, data):

        local_data_work = DataHandling()
        while True:
            x = raw_input(
                "Press Y to add to your database, E to edit, D to display records or any other key to quit: > ")
            if x == "Y":
                self.data = local_data_work.add_data(self.data)
            elif x == "E":
                data_work.edit_data(self.data)
            elif x == "D":
                data_work.print_output_to_screen(data)
            elif x == "Q":
                data_work.print_output_to_screen(data)
                filing.save_data(data, birth_data_file)
                exit()
            else:
                pass

    def get_input_int(self, prompt, min, max):
        self.prompt, self.min, self.max = prompt, min, max
    # check raw_input is an integer of correct length and return
    # takes prompt parameter
        while True:
            try:
                self.num = int(raw_input("%s" % self.prompt))
                if self.min <= self.num <= self.max:
                    return self.num

            except ValueError:
                print "Error. Please enter a number of the correct length."

    def add_data(self, data):

        self.name = raw_input(
            "Enter the name you wish to add to the database: > ")
        self.day = data_work.get_input_int("Date of birth: dd  > ", 1, 31)
        self.month = data_work.get_input_int("Date of birth: mm > ", 1, 12)
        self.year = data_work.get_input_int(
            "Date of birth: yyyy > ", 1900, 2100)

        self.data[self.name] = [self.day, self.month, self.year]
        return self.data

    def edit_data(self, data):

        local_data_work = DataHandling()
        local_data_work.print_output_to_screen(self.data)
        while True:
            x = raw_input(
                "Press 1 to change your data, 2 to delete a record >")
            if x == "1":
                print "Re - enter the name and data"
                local_data_work.add_data(data)
                return data

            elif x == "2":
                del_ = raw_input("Type name to be deleted > ")
                del data[del_]
                return data

            else:
                print "Not understood"
                pass

    def print_output_to_screen(self, data):
        # prints dict keys and data

        self.data = data
        print ""
        for i in sorted(self.data):
            print "%s" % i,
            print "%d   %d  %d Age at birthday this year is %d" % (self.data[i][0], self.data[i][5], self.data[i][6], (int(time.strftime("%Y")) - data[i][7]))

# START

birth_data_file = "birth_data"

filing = Filing(birth_data_file)
filing.check_create(birth_data_file)
data = filing.open_file_read_to_variable()

data_work = DataHandling()

data_work.menu(data)

data_work.print_output_to_screen(data)

filing.save_data(data, birth_data_file)

More tools

You'll find other tools to check how your code looks automatically : pylint, pyflakes, pychecker (and a few more I might be missing, comments are welcome).

pylint hilights the fact that your documentation is not quite in the right format : PEP 257 gives the docstring conventions. It also hilights a few things that shows probably bigger problems : unused arguments for instance

Trying to use pychecker will show something else that is wrong with your code : one should be able to import your code just to get your functions/classes without running your code. In order to do so, you should use a main guard :

if __name__ == '__main__':
     # your code

Comments

Slightly redundant with style but a few comments are not really helpful : for instance # create file and open in append mode - pointer at end of file is not really helpful for anyone able to open the open documentation (which should be anyone reading your code really).

Single responsability

I do not quite agree with your general design but in any way, you should try to follow simple rules like the Single Responsability Principle. For instance, no one expects/wants a method called print_output_to_screen to do more than printing. In particular, having side effects like self.data = data is most probably wrong.

User interaction

You are lying to your user : you print "Press Y to add to your database, E to edit, D to display records or any other key to quit: > " and you check if the value is Q to quit. This got me confused.

Using contexts

You open files in different places in your code and then you need to close them. Congratulations for thinking about closing them, this is a usual error. Fortunately, Python provides you a nice keyword so that you don't need to do the closing "manually" : the with keyword. I suggest you read about it as it is really interesting.

There is a lot more to say but this should give enough some food for thought.

\$\endgroup\$

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.