First practical Python code, comments appreciated

[Steve Holden]

planetthoughtful wrote:
> Hi All,
> 
> I've written my first piece of practical Python code (included below),
> and would appreciate some comments. My situation was that I had a
> directory with a number of subdirectories that contained one or more
> zip files in each. Many of the zipfiles had the same filename (which is
> why they had previously been stored in separate directories). I wanted
> to bring all of the zip files (several hundrd in total) down to the
> common parent directory and obviously rename them in the process by
> appending a unique string to each filename.
> 
> The code below did the job admirably, but I don't imagine it is
> anywhere near as efficiently written as it could and should be. Note:
> I'm aware there was no need, for example, to wrap the "os.walk(path)"
> statement in a def -- in a couple of places I added extra steps just to
> help familiarize myself with Python syntax.
> 
> I'd very much like to see how experienced Python coders would have
> achieved the same task, if any of you can spare a couple of minutes to
> share some knowledge with me.
> 
> Many thanks and much warmth,
> 
> planetthoughtful

Brave of you. Please note I haven't actually tested the exact format of 
these suggestions, so I made have made stupid typos or syntax errors.
> 
> import os
> fcount = 0
> path = 'X:\zipfiles'
> def listFiles(path):
>     mylist = os.walk(path)
>     return mylist
> 
> filelist = listFiles(path)
> for s in filelist:
>     if len(s[2]) > 0:

You don't really need this "if" - with an empty s the "for" loop on the 
next line will simply execute its body zero times, giving the effect you 
appear to want without the extra level of logic.

>         for f in s[2]:
>             pad = str(fcount)
>             if len(pad) == 1:
>                 pad = "00" + pad
>             elif len(pad) == 2:
>                 pad = "0" + pad
> 
Here you could take advantage of the string formatting "%" operator and 
instead of the "if" statement just say

               pad = "%03d" % fcount

  >>> ["%03d" % x for x in (1, 3, 10, 30, 100, 300)]
['001', '003', '010', '030', '100', '300']

>             (fname, fext) = os.path.splitext(f)
>             oldname = f

There isn't really any advantage to this assignment, though I admit it 
does show what you are doing a little better. So why not just use

            for oldname in s[2]:

to control the loop, and then replace the two statements above with

               (fname, fext) = os.path.splitext(oldname)

Note that assignments of one plain name to another are always fast 
operations in Pythin, though, so this isn't criminal behaviour - it just 
clutters your logic a little having essentially two names for the same 
thing.

>             newname = fname + "_" + pad + fext
>             os.rename(s[0] + "\\" + oldname, path + "\\" + newname)

That form is non-portable. You might argue "I'm never going to run this 
program on anything other than Windows", and indeed for throwaway 
programs it's often easier to write something non-portable. It's 
surprising, though, how often you end up *not* throwing away such 
programs, so it can help to write portably from the start. I'd have used

               newname = os.path.join(path,
                              "%s_%s.%s" % (fname, pad, fext))
               os.rename(os.path.join(s[0], oldname), newname)

>             fcount = fcount + 1
> 

Just a few pointers to make the script simpler, but your program is a 
very creditable first effort. Let us know what mistakes I made!

regards
  Steve
-- 
Steve Holden       +44 150 684 7255  +1 800 494 3119
Holden Web LLC                     www.holdenweb.com
PyCon TX 2006                  www.python.org/pycon/