7
\$\begingroup\$

I am trying to open two CSV files, one for reading and one for writing. The script should continue only if both files were successfully opened. My code seems to accomplish that but it seems like it could be improved further.

My questions are:

  1. Should I consider a different approach?
  2. My code feels a bit bloated inside the try statement (imagine 100 additional lines of code). Is there a "cleaner" approach?
  3. If the files were opened successfully, but a different error occurs later on that is not an IOError, this code will not catch it, correct?

Code:

input_file = 'in_file.csv'
output_file = 'out_file.csv'


def main():

    try:
        with open(input_file, 'rb') as in_csv, open(output_file , 'wb') as out_csv:
            writer = csv.writer(out_csv, delimiter='\t')
            reader = csv.reader(in_csv, delimiter='\t')
            for row in reader:
                # many lines of code...
    except IOError as e:
        print "Error: cannot open file"
        if e.errno == errno.EACCES:
            print "\tPermission denied."
            print "\tError message: {0}".format(e)
            sys.exit()
        # Not a permission error.
        print "\tDoes file exist?"
        print "\tError message: {0}".format(e)
        sys.exit()


if __name__ == '__main__':
    main()
\$\endgroup\$
4
\$\begingroup\$

To answer your questions…

  1. Your approach is fine.
  2. Move your hundreds of lines into a function, then break down that function into reasonable-sized chunks. If it's still huge and ugly, then pose that as another code review question.
  3. If an error other an IOError occurred, then the error would not get caught. However, when the with block is exited for any reason, input_file and output_file will get properly closed.

In addition to what @JimDennis said, I would like to point out that it it customary to print error messages to sys.stderr, and exit with a non-zero status when an error occurs.

def process(csv_reader, csv_writer):
    for row in csv_reader:
        # many lines of code...

def main(input_filename, output_filename):
    try:
        with open(input_filename, 'rb') as in_csv, open(output_filename , 'wb') as out_csv:
            writer = csv.writer(out_csv, delimiter='\t')
            reader = csv.reader(in_csv, delimiter='\t')
            process(reader, writer)
    except IOError as e:
        print >> sys.stderr, "Error: cannot open file"
        if e.errno == errno.EACCES:
            print >> sys.stderr, "\tPermission denied."
            print >> sys.stderr, "\tError message: {0}".format(e)
            sys.exit(1)
        # Not a permission error.
        print >> sys.stderr, "\tError message: {0}".format(e)
        sys.exit(1)
    except Exception as other_exception:
        print >> sys.stderr, "Error: " + str(other_exception)
        sys.exit(2)

if __name__ == '__main__':
    main('in_file.csv', 'out_file.csv')
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for answering my questions and providing an example! \$\endgroup\$ – fire_water Sep 24 '13 at 20:16
  • \$\begingroup\$ You're welcome. I've added some minor touches. In particular, you shouldn't speculate on the cause of an error, because you could mislead the user. Let the error message speak for itself. \$\endgroup\$ – 200_success Sep 24 '13 at 20:45
1
\$\begingroup\$

You might want to pass the filenames to main() as parameters, to promote code re-use and testing. Then in __main__: you could import sys and check for len(sys.argv) > 1 ... setting the input, and output filenames thereby.

(You can preserve the default behavior in the case where your program is called as it, but these changes would also mean that your CSV re-writer could be used as a module for some other program).

In fact it would also be good go rename your current main() to something more reflective of it's function and wrap the argument handling into a newly written main() wrapper around the function you've shown here.

Generalizing further you could add a delimiter=None as a defaulted argument to your function and thus facilitate over-ride of the input and output delimiters inside the function.

All of that depends on how generalized and re-useable you want your code to be. Obviously there's a trade off between the flexible functionality and the additional complexity being introduced there.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Much appreciated. Great explanation! \$\endgroup\$ – fire_water Sep 24 '13 at 20:15

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.