tags:

views:

184

answers:

5

This is my first python script, be ye warned.

I pieced this together from Dive Into Python, and it works great. However since it is my first Python script I would appreciate any tips on how it can be made better or approaches that may better embrace the Python way of programming.

import os
import shutil

def getSourceDirectory():
    """Get the starting source path of folders/files to backup"""
    return "/Users/robert/Music/iTunes/iTunes Media/"

def getDestinationDirectory():
    """Get the starting destination path for backup"""
    return "/Users/robert/Desktop/Backup/"

def walkDirectory(source, destination):
    """Walk the path and iterate directories and files"""

    sourceList = [os.path.normcase(f)
        for f in os.listdir(source)]

    destinationList = [os.path.normcase(f)
        for f in os.listdir(destination)]

    for f in sourceList:
        sourceItem = os.path.join(source, f)
        destinationItem = os.path.join(destination, f)  

        if os.path.isfile(sourceItem):
            """ignore system files"""
            if f.startswith("."):
                continue

            if not f in destinationList:
                "Copying file: " + f
                shutil.copyfile(sourceItem, destinationItem)

        elif os.path.isdir(sourceItem):
            if not f in destinationList:
                print "Creating dir: " + f
                os.makedirs(destinationItem)

            walkDirectory(sourceItem, destinationItem)

"""Make sure starting destination path exists"""
source = getSourceDirectory()
destination = getDestinationDirectory()

if not os.path.exists(destination):
    os.makedirs(destination)

walkDirectory(source, destination)
+3  A: 

Use os.path.walk. It does most all the bookkeeping for you; you then just feed a visitor function to it to do what you need.

Or, oh damn, looks like os.path.walk has been deprecated. Use os.walk then, and you get

for r, d, f in os.walk('/root/path')
    for file in f:
       # do something good.
Charlie Martin
Isn't os.path.walk deprecated in favor of os.walk? >>>Note This function is deprecated and has been removed in 3.0 in favor of os.walk(). (from the bottom of http://docs.python.org/library/os.path.html)
JoshD
Looks cool, I will check it out, thanks.
blu
yeah, caught that myself. I can't keep up with the deprecations. Thanks.
Charlie Martin
+2  A: 

I recommend using os.walk. It does what it looks like you're doing. It offers a nice interface that's easy to utilize to do whatever you need.

JoshD
+6  A: 

As others mentioned, you probably want to use walk from the built-in os module. Also, consider using PEP 8 compatible style (no camel-case but this_stye_of_function_naming()). Wrapping directly executable code (i.e. no library/module) into a if __name__ == '__main__': ... block is also a good practice.

paprika
I started to look at the style guide on python.org, but I used Dive Into Python as my starting point for functionality. Is Dive Into Python totally jacked in terms of syntax?
blu
Also if I can, why does the os package not use "_"? Ex. "isfile", "makedirs", etc.
blu
Never mind, "The naming conventions of Python's library are a bit of a mess, so we'll never get this completely consistent", oh man...
blu
+4  A: 

The code

  • has no docstring describing what it does
  • re-invents the "battery" of shutil.copytree
  • has a function called walkDirectory which doesn't do what its name implies
  • contains get* functions that provide no utility
    • those get functions embed high-level arguments deeper than they ought
  • is obligatorily chatty (print whether you want it or not)
msw
You mixed in general complaints about the coding style, but did so in a non-verbose, non-disturbing way. This is exactly what I expect from quality answers on SO. (+1)
paprika
Does shutil.copytree really do the same thing? "The destination directory, named by dst, must not already exist". The script I wrote doesn't dumb copy the entire source every time, it checks for new files that haven't already been copied. Am I missing something?
blu
Your other points are great feedback, thanks.
blu
The whole thing with "walk" is totally valid. I was going to pass a function to the function to do the actual logic, good catch.
blu
No, it isn't an exact replacement for shutil.copytree in all aspects, true. However, when creating a feature with similar operation to an existing standard facility, it is good to follow the general approach of the other for if you know `copytree(...)` then your new `update_tree(...)` will be easier to remember and use.
msw
+1  A: 

The main thing to make things more Pythonic is to adopt Python's PEP8, the style guide. It uses underscore for functions.1

If you're returning a fixed string, e.g. your get* functions, a variable is probably a better approach. By this, I mean replace your getSourceDirectory with something like this:

source_directory = "/Users/robert/Music/iTunes/iTunes Media/"

Adding the following conditional will mean that code that is specific for running the module as a program does not get called when the module is imported.

if __name__ == '__main__':
    source = getSourceDirectory()
    destination = getDestinationDirectory()

    if not os.path.exists(destination):
        os.makedirs(destination)

    walkDirectory(source, destination)

I would use a try & except block, rather than a conditional to test if walkDirectory can operate successfully. Weird things can happen with multiple processes & filesystems:

try:
    walkDirectory(source, destination)
except IOError:
    os.makedirs(destination)
    walkDirectory(source, destination)

1 I've left out discussion about whether to use the standard library. At this stage of your Python journey, I think you're just after a feel how the language should be used in general terms. I don't think knowing the details of os.walk is really that important right now.

Tim McNamara