current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

After reading some texts regarding creation of files under Python, I've decided to create this class which creates a new file on a directory, and creating a backup on the other directory if the file already exists (and if it's older than x hours).

The main reason I opened this question is to know if this is a correct way to write a class using try/except correctly, because actually I'm getting a little confused about the preference of using try/except instead if/elses.

The working example:

import os
import datetime


class CreateXML():
    def __init__(self, path, filename):
        self.path = path
        self.bkp_path = "%s\\backup" % path
        self.filename = filename
        self.bkp_file = "%s.previous" % filename
        self.create_check = datetime.datetime.now()-datetime.timedelta(hours=-8)

    @staticmethod
    def create_dir(path):
        try:
            os.makedirs(path)
            return True
        except:
            return False

    @staticmethod
    def file_check(file):
        try:
            open(file)
            return True
        except:
            return False

    def create_file(self, target_dir, target_file):
        try:
            target = "%s\\%s" % (target_dir, target_file)
            open(target, 'w')
        except:
            return False

    def start_creation(self):
        try:
            # Check if file exists
            if self.file_check("%s\\%s" % (self.path, self.filename)):
                self.create_dir(self.bkp_path)
                creation = os.path.getmtime("%s\\%s" % (self.path, self.filename))
                fcdata = datetime.datetime.fromtimestamp(creation)
                # File exists and its older than 8 hours
                if fcdata < self.create_check:
                    bkp_file_path = "%s\\%s " % (self.bkp_path, self.bkp_file)
                    new_file_path = "%s\\%s " % (self.path, self.filename)
                    # If backup file exists, erase current backup file
                    # Move existing file to backup and create new file.
                    if self.file_check("%s\\%s" % (self.bkp_path, self.bkp_file)):
                        os.remove(bkp_file_path)
                        os.rename(new_file_path, bkp_file_path)
                        self.create_file(self.bkp_path, self.bkp_file)
                    #No backup file, create new one.
                    else:
                        self.create_file(self.bkp_path, self.bkp_file)
            else:
                # Fresh creation
                self.create_dir(self.path)
                self.create_file(self.path, self.filename)
        except OSError, e:
            print e
if __name__ == '__main__':
    path = 'c:\\tempdata'
    filename = 'somefile.txt'
    cx = CreateXML(path, filename)
    cx.start_creation()

So, basically the real questions here are:

  • With the example above, is the usage of try/except correct?

  • Is it correct to perform the validations using try/except to check if file or directory already exists? Instead of using a simplified version like this one:

import os
# Simple method of doing it
path = 'c:\\tempdata'
filename = 'somefile.txt'
bkp_path = 'c:\\tempdata\\backup'
bkp_file = 'somefile.txt.bkp'
new_file_path = "%s\\%s" % (path, filename)
bkp_file_path = "%s\\%s" % (bkp_path, bkp_file)

if not os.path.exists(path):
    print "create path"
    os.makedirs(bkp_path)
if not os.path.isfile(new_file_path):
    print "create new file"
    open(new_file_path, 'w')
else:
    print"file exists, moving to backup folder"
    #check if backup file exists
    if not os.path.isfile(bkp_file_path):
        print "New backup file created"
        open(bkp_file_path, 'w')
    else:
        print "backup exists, removing backup, backup the current, and creating newfile"
        os.remove(bkp_file_path)
        os.rename(new_file_path, bkp_file_path)
        open(bkp_file_path, 'w')
  • If the usage of try/except is correct, is it recommended to write a big class to create a file if it's possible to write a short version of it?

I'm really confused about the "most correct pythonic way to do it".

share|improve this question
    
Close your question? We're not StackOverflow. We do put things on hold every now and then, but this question? Wouldn't dream of it. –  Simon André Forsberg Jan 15 at 17:30
    
@SimonAndréForsberg Hi Simon, sorry if this is the wrong place, I was thinking this code could be reviewed by someone. Should i close the question then? –  Thales Jan 15 at 17:33
    
What I tried to say is that you have come to the right place. Welcome :) –  Simon André Forsberg Jan 15 at 17:34
    
I'm relieved then :) –  Thales Jan 15 at 17:36
add comment

1 Answer

up vote 2 down vote

In your specific case, the second snippet shows the best way to go as your code is simplified and makes more sense. Make sure to seperate two if blocks when they're not the same, otherwise they might look like if/else and confuse anyone reading that code (including you in a few days!).

if not os.path.exists(path):
    print "create path"
    os.makedirs(bkp_path)

if not os.path.isfile(new_file_path):
    print "create new file"
    open(new_file_path, 'w')

Make sure to use os.path.join: your code will be more easily ported to another platform and you won't have to use those nasty double slashes.

Lastly, you seem to be confused about exceptions, so let me explain a little bit.

How exceptions work

Think of try/except as another way to express your error-handling logic. It is generally more useful when the error is considered as "exceptional" and won't happen often. In Python we prefer to go a bit further than that: it's often easier to ask for forgiveness than permission (EAFP).

So, to answer your question: no, that's not how exceptions work.

@staticmethod
def create_dir(path):
    try:
        os.makedirs(path)
        return True
    except:
        return False

The point of exceptions is that you can catch them anywhere in the code. You can catch this one in create_dir as you did, but also in start_creation and even when calling start_creation. That is, you could write:

if __name__ == '__main__':
    path = 'c:\\tempdata'
    filename = 'somefile.txt'
    try:
        cx = CreateXML(path, filename)
        cx.start_creation()
    except:
        pass  # handle my exception

But that probably wouldn't be very useful because you couldn't do anything about that exception. This is why you need to find a middle ground:

  • don't catch the exception too early (as you did) because you don't have enough context to know how to fix the error,
  • and don't catch it too late because you won't be able to do anything about it.

The choice depends on your application. There are exceptions that you really don't want to catch because your program really cannot continue to work. For example, if your main task is backing up, what can you do if the folder is not writable? You probably want to say so to your user: just fail is it's a command-line app, and explain the error to your user if it's a GUI app.

To be able to do that, you must be able to distinguish between various exceptions. The best way to do that is to only catch the exceptions you expect. For example, os.makedirs can only throw an OSError, so that's what you need to catch, as you did in start_creation. However, printing the exception is not engouh: if you catch it, that means you can do something about it.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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