Can someone comment on my code?

[Chris Liechti]

Chris Burn <chris_burn at NO-SPAM.bigfoot.com> wrote in
news:3CAD0719.6050707 at NO-SPAM.bigfoot.com: 

> Hi, I've been writing perl, shell, c programs for a while but I
> thought I should move on to python.

welcome

> So this is my first real venture in python. Basically I work on an app
> that auto saves to your home dir every 10 mins, but I keep my working 
> files in a dir elsewhere. So this script is intended to be run after a
> crash, it searches for the last autosave and the last normal save in
> the current dir. It then copies the autosave to the current dir with
> _rec stuck on the end.
> 
> Anyway, it works, but I'm new to error handling in python and it would
> be good to know if I'm handling errors correctly and also if there are
> easier ways to do things or conventions I should be keeping to? It
> seems to be a bit long to me.
> 
> Oh it needs to run on python 1.5.2
> 
> Thanks
> Chris
>
>
> #!/usr/bin/python
> 
> import os, sys, re, shutil
> 
> class FileListError(Exception):
>      def __init__(self, message):
>              self.message = message
>      def __str__(self):
>           return self.message
...

no need to redefine __init__ and __str__ here. this will work as expected:

class FileListError(Exception): pass
class NoAutoError(Exception): pass
class NoSavesError(Exception): pass
class DontWriteError(Exception): pass


> def fileList(dir):
>      flist = []
>      try:
>           for file in os.listdir(dir):
>                flist.append(os.path.join(dir, file))
>           return flist
>      except:
>           raise FileListError(dir)

catch specific exceptions. just catchig everything makes debuging and code 
maintenace a _lot_ harder.

> def isSave(fname):
>      return re.search('autoSave\d\.shk', fname)

note that '\' in re are escaped twice. once by the python parser and once 
py the re module. this doesn't hurt as '\d' and '\.' are no valid python 
escapes. you can avoid problems by using raw strings:
 
r'autoSave\d\.shk'

> def isScript(fname):
>      return re.search('\.shk', fname)
> 
> def getLastAuto():
>      files = fileList("/home/users/cburn/nreal/autosave")
>      files = filter(isSave, files)
> 
>      if files == []:
>           raise NoAutoError

no need to test for emptyness explicit. "if not files: ..." will do.

>      filetime = []
>      for fname in files:
>           filetime.append([os.path.getmtime(fname), fname])
> 
>      filetime.sort()
>      filetime.reverse()
>      return filetime[0][1]
> 
> def getLastSave():
>      files = fileList(os.getcwd())
>      files = filter(isScript, files)
> 
>      if files == []:
>           raise NoSavesError
> 
>      filetime = []
>      for fname in files:
>           filetime.append([os.path.getmtime(fname), fname])
> 
>      filetime.sort()
>      filetime.reverse()
>      return filetime[0][1]
> 
> def okWrite(file):
>      if not os.path.exists(file):
>           raise DontWriteError

when you add

if __name__ == '__main__':
    	...

here and indent to following codeblocks, you can use the same file as a 
script and as a module for an otherone. makes code reuse very easy.

> try:
>      lastAuto = getLastAuto()
>      lastSave = getLastSave()
> 
>      newSave = os.path.split(lastSave)
>      newSave = os.path.join(newSave[0], re.match('([^\s]+)\.shk',
>      newSave[1]).group(1) + '_rec.shk') 
> 
>      try:
>           okWrite(newSave)
>           print 'Copying %s to %s' % (lastAuto, newSave)
>           ok = raw_input("OK? ")
>           if (ok == "y"):
>                shutil.copyfile(lastAuto, newSave)
>      except:
>           raise
if you just re-raise the exception you don't need the try: ... except: 
block at all...

> except FileListError, err:
>      print 'Error: Cannot read %s directory' % err
> except NoAutoError:
>      print 'Error: Cannot find any autosaves!'
> except NoSavesError:
>      print 'Error: Cannot find any shake scripts!'
> except DontWriteError:
>      print "Can't copy, file exists: %s" % newSave
> except:
>      print "Unexpected error:", sys.exc_info()[0]

you don't need to handle all errors with execptions...

if okWrite(newSave):
    	...

is very readable and okWrite is shorter too:

def okWrite(file):
    return os.path.exists(file)


HTH, chris


-- 
Chris <cliechti at gmx.net>