5
\$\begingroup\$

I have a Python script I have written to copy files to a mounted Windows SMB share on a Mac.

import os
import distutils.core
# Create a local site for the mount to reside
directory = "/Users/username/share"
if not os.path.exists(directory): os.makedirs(directory)
# Mount the Windows smb share
os.system("mount_smbfs //service.account:password@server.domain.com/share ~/share")
# set source and destination
fromDirectory1 = "/Volumes/Macintosh HD/folder1/folder2/folder3"
fromDirectory2 = "/Volumes/Macintosh HD/folder4/folder5/folder6"
fromDirectory3 = "/Volumes/Macintosh HD2/folder1/folder2"
fromDirectory4 = "/Volumes/Macintosh HD2/folder1/folder3/folder4"
toDirectory1 = "/Users/username/share/folder3"
toDirectory2 = "/Users/username/share/folder6"
toDirectory3 = "/Users/username/share/folder2"
toDirectory4 = "/Users/username/share/folder4"
#Do the copying
distutils.dir_util.copy_tree(fromDirectory1, toDirectory1)
distutils.dir_util.copy_tree(fromDirectory2, toDirectory2)
distutils.dir_util.copy_tree(fromDirectory3, toDirectory3)
distutils.dir_util.copy_tree(fromDirectory4, toDirectory4)
#Unmount the share
os.system("umount ~/share")

I understand the script is verbose but I wrote it this way to # out lines to problem solve. Can you suggest a cleaner way to write it?

Any insights gratefully received.

Version 2:

import os
import distutils.core


# Create a local site for the mount to reside
directory = "/Users/username/share"
if not os.path.exists(directory): os.makedirs(directory)

# Mount the Windows smb share
os.system("mount_smbfs //service.account:password@server.domain.com/share ~/share")

# set source and destination
copy_jobs = []
copy_jobs.append({"from": "/Volumes/Macintosh HD/folder1/folder2/folder3", "to": "/Users/username/share/folder3"})
copy_jobs.append({"from": "/Volumes/Macintosh HD/folder4/folder5/folder6", "to": "/Users/username/share/folder6"})
copy_jobs.append({"from": "/Volumes/Macintosh HD2/folder1/folder2", "to": "/Users/username/share/folder2"})
copy_jobs.append({"from": "/Volumes/Macintosh HD2/folder1/folder3/folder4", "to": "/Users/username/share/folder4"})

#Do the copying
for job in copy_jobs:
    distutils.dir_util.copy_tree(job["from"], job["to"])

# Unmount the Windows smb share
os.system("umount ~/share")
\$\endgroup\$
  • 1
    \$\begingroup\$ You do not use toDirectory1. Is it normal ? \$\endgroup\$ – SylvainD Feb 11 '14 at 14:40
  • \$\begingroup\$ @Josay typo! Good eyes. Have corrected it. Thanks \$\endgroup\$ – John Harris Feb 11 '14 at 15:14
  • 1
    \$\begingroup\$ It would be better to use shutil.copytree from the standard library instead of the version from distutils. \$\endgroup\$ – Gareth Rees Feb 11 '14 at 15:28
  • \$\begingroup\$ @GarethRees Thanks for your reply. Never had much luck with shutil - if I import shutil and replace distutils.dir_util.copy_tree with shutil.copytree I get a page of [Errno 22] invalid argument errors. \$\endgroup\$ – John Harris Feb 11 '14 at 16:10
  • 1
    \$\begingroup\$ That probably indicates a bug in shutil.copytree which you should report. \$\endgroup\$ – Gareth Rees Feb 11 '14 at 17:18
4
\$\begingroup\$

The code looks OK. A few specific improvements can be made.

Readability

import os
import distutils.core

Learn about PEP8, you should have two empty lines between imports and the rest of the code. More generally, you should give more structure to your code to make it more readable. Empty lines are one way to achieve that. For example, one empty line before each comment could be nice.

# Create a local site for the mount to reside
directory = "/Users/username/share"
if not os.path.exists(directory): os.makedirs(directory)

PEP8 comment: prefer indenting ifs to make clear they are ifs.

"Security"

# Mount the Windows smb share
os.system("mount_smbfs //service.account:password@server.domain.com/share ~/share")

Don't store account and password in your source file. You could move them to a configuration file (that would not be tracked if you used git or svn) or retrieve them from the environment (but then they would show up in your shell logs).

Don't Repeat Yourself

# set source and destination
fromDirectory1 = "/Volumes/Macintosh HD/folder1/folder2/folder3"
fromDirectory2 = "/Volumes/Macintosh HD/folder4/folder5/folder6"
fromDirectory3 = "/Volumes/Macintosh HD2/folder1/folder2"
fromDirectory4 = "/Volumes/Macintosh HD2/folder1/folder3/folder4"
toDirectory1 = "/Users/username/share/folder3"
toDirectory2 = "/Users/username/share/folder6"
toDirectory3 = "/Users/username/share/folder2"
toDirectory4 = "/Users/username/share/folder4"

Variables name var1, var2 and so on indicate that you should use a list of pairs or a dictionary!

directories = [
    ("/Volumes/Macintosh HD/folder1/folder2/folder3", "/Users/username/share/folder3"),
    ("/Volumes/Macintosh HD/folder4/folder5/folder6", "/Users/username/share/folder6"),
]

#Do the copying
for from, to in directories:
    distutils.dir_util.copy_tree(from, to)

#Unmount the share
os.system("umount ~/share")

A note about the 80-char limit: in this case, the directories list is more readable if you don't enforce the rule.

Recovering from failures

What happens when mounting works but not copying? You should still try to unmount your folder.

try:
    # mount
    # copy
finally:
    # umount

You can also decide to catch exceptions if you know what to do about them.

\$\endgroup\$
  • 2
    \$\begingroup\$ Excellent answer. As an additional comment, depending on how your requirements change in the future, you might want directories to become a list of pairs <from, to> if the same folder is to be copied in different places. The change should be pretty straight forward to perform and the idea is still to use the right data structure instead of duplicating logic. \$\endgroup\$ – SylvainD Feb 12 '14 at 10:26
  • \$\begingroup\$ Nice spot. I didn't really know which one to choose, I'll switch to a list of pairs. Thanks! \$\endgroup\$ – Quentin Pradet Feb 12 '14 at 10:29
  • \$\begingroup\$ @QuentinPradet Thanks for your insights. I have used a dictionary and revised my code (see version 2 above). Am aware of the security issues but it is on a secure Mac server. Never-the-less I will investigate a config file. I need to put the recovering from failure and perhaps a logging step into the code. Thanks again \$\endgroup\$ – John Harris Feb 12 '14 at 10:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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