Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote this script to backup some important files. It backs up the files to a local folder as well as to an external hard drive. It creates a new subdirectory which has its name constructed with the current date and time. Works perfectly, but I'm just wondering if there are any Python best-practices I'm missing or if there's any way I could've made it better!

import datetime
import os
import shutil

GOOGLE_DRIVE_DIRECTORY = 'C:\\Users\\Jeff\\Google Drive\\Manifest_Destiny'
MAIN_BACKUP_DIRECTORY = 'C:\\Users\\Jeff\\Desktop\\Manifest_Destiny_Backups\\md_backup_{0}'
EXTERNAL_DRIVE_DIRECTORY = 'F:\\My Files\\Manifest_Destiny_Backups\\md_backup_{0}'

def get_backup_directory(base_directory):
    date = str([:16]
    date = date.replace(' ', '_').replace(':', '')
    return base_directory.format(date)

def copy_files(directory):
    for file in os.listdir(GOOGLE_DRIVE_DIRECTORY):
        file_path = os.path.join(GOOGLE_DRIVE_DIRECTORY, file)
        if os.path.isfile(file_path):
            shutil.copy(file_path, directory)

def perform_backup(base_directory):
    backup_directory = get_backup_directory(base_directory)

def main():

if __name__ == '__main__':
share|improve this question

2 Answers 2

up vote 13 down vote accepted

You can simplify paths using forward slashes:

GOOGLE_DRIVE_DIRECTORY = 'C:/Users/Jeff/Google Drive/Manifest_Destiny'
MAIN_BACKUP_DIRECTORY = 'C:/Users/Jeff/Desktop/Manifest_Destiny_Backups/md_backup_{0}'
EXTERNAL_DRIVE_DIRECTORY = 'F:/My Files/Manifest_Destiny_Backups/md_backup_{0}'

Instead of converting a date to a string, which may be locale dependent too, and then replacing characters in it, better to use strftime to generate exactly the format that you want:

def get_backup_directory(base_directory):
    date ='%Y-%m-%d_%H%M')
    return base_directory.format(date)

If a variable contains a format, I would suffix it with _fmt or _format to clarify.

I don't really like global constants inside methods, so I would split copy_files like this:

def copy_files_to(srcdir, dstdir):
    for file in os.listdir(srcdir):
        file_path = os.path.join(srcdir, file)
        if os.path.isfile(file_path):
            shutil.copy(file_path, dstdir)

def copy_files(dstdir):
    copy_files_to(GOOGLE_DRIVE_DIRECTORY, dstdir)

The advantage of this is that copy_files_to is easier to unit test. I think it's also good to use more specific names for the directory variables to clarify which one is a source and destination.

share|improve this answer

I typically create wrapper methods around anything that is specific to the target platform [aka, Windows™, Mac™, Linux™, Android™, etc]. I might use method names that look like setGoogleDriveDir( ), getExternDriveDir( ), learnBackupDir( ), etc. These methods may simply return the platform specific constants, but they may also do something complex to supply the needed value using platform specific components. For example, there might discover the specific platform using system calls and then use switch-case logic to compute appropriate return values.

All of this presumes that you anticipate the requirement to run your scripts on multiple platforms in future. Remember, "... No one will ever use more than 640 Kb of RAM ..."

share|improve this answer
Welcome to CodeReview. Would you add more context to this answer? Right now it's somewhat unclear what you're actually suggesting. –  Legato Apr 6 at 23:54
Those method names don't follow PEP8 –  JaDogg Apr 7 at 3:09

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

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