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/
--
http://mail.python.org/mailman/listinfo/python-list
