3
\$\begingroup\$
#!/usr/bin/env python

"""
Safely copies .mpg files from one volume to another using
temporary filenames and hash verification
"""

import os
from datetime import datetime
from hashlib import sha256
from shutil import copy2

#ask user for origin (where to get files)
def choose_drive(drives, label):
    while True:
        for d in drives:
            print '%s - %s' % (drives.index(d)+1,d)
        drive = raw_input('Select %s: ' % label)
        if not drive in str(range(0, len(drives)+1)):
            continue
        else:
            return os.path.join('/Volumes', drives[int(drive)-1])

# safe copy
def safe_copy(src, dst):
    dst_temp_name = dst+'_INCOMPLETE'
    try:
        copy2(src,dst_temp_name)
        if compare_hashes(src, dst_temp_name):
            try:
                os.rename(dst_temp_name, dst)
            except Exception as e:
                print 'Error trying to rename the file: ', e
        else:
            safe_copy(src, dst)
    except Exception as e:
        print 'Error trying to copy the file: ', e

# hash a file and return the hash string
def get_local_hash(filename, block_size=2**20):
    """
    Hashes a file using SHA1
    """
    # print 'Starting hash of %s at %s' % (filename, datetime.now().time())
    print 'Hashing', filename
    try:
        local_file = open(filename,'rb')
    except Exception as e:
        print 'Error while opening file for hashing: ', e
    hasher = sha256()
    while True:
        data = local_file.read(block_size)
        if not data:
            break
        hasher.update(data)
    # print 'Finished hash at %s' % datetime.now().time()
    result = hasher.hexdigest()
    local_file.close()
    return unicode(result)

def compare_hashes(file1, file2):
    if get_local_hash(file1) == get_local_hash(file2):
        return True

#main
def main():
    # get list of volumes
    drives = [d for d in os.listdir('/Volumes')]
    # ask user for origin drive
    origin = choose_drive(drives, 'Origin')
    origin_files = os.listdir(origin)
    # ask user for destination drive
    destination = choose_drive(drives, 'Destination')
    destination_files = os.listdir(destination)
    # for each file on origin, check if it exists on destination
    for of in origin_files:
        if of.endswith('.mpg'):
            if of in destination_files:
                if compare_hashes(os.path.join(origin, of), os.path.join(destination, of)):
                    print 'already exists and is identical... skipping', of
                else:
                    print 'already exists but is different... copying', of
                    safe_copy(os.path.join(origin, of), os.path.join(destination, of))
            else:
                print 'does not exists... copying', of
                safe_copy(os.path.join(origin, of), os.path.join(destination, of))




if __name__ == "__main__":

    main()
\$\endgroup\$
6
  • 1
    \$\begingroup\$ No explanation, no nothing, we don't work for you. We are here for free and for fun, if you want our help you'll have to ask better. \$\endgroup\$
    – Rik Poggi
    Apr 3 '12 at 14:26
  • \$\begingroup\$ Hmm not sure what more explanation I can give.. and I did say please. Something about the name "Code Review" led me to believe I could put my code up for review. Sheesh.. I can be so dumb :-| \$\endgroup\$
    – MFB
    Apr 3 '12 at 15:08
  • 3
    \$\begingroup\$ @RikPoggi, I see nothing wrong with this question. The docstring provides a good explanation of what the code is trying to do. \$\endgroup\$ Apr 3 '12 at 15:16
  • 2
    \$\begingroup\$ @RikPoggi Strictly adhering to the faq, I think you are mistaken. His question contains code, and he has invited review of all facets of his code. It wasn't an impolite question, and is entirely appropriate for CodeReview. \$\endgroup\$
    – kojiro
    Apr 3 '12 at 20:44
  • 1
    \$\begingroup\$ @kojiro: I'm mistaken for what? I never said that it's not a good fit for codereview or something like that; otherwise I'd have cast a closing vote. I was stating that it's a poor question. I wont go on repeating my point, since until the question stays this way I wont change it: a question with a title "Please review Python code" and then it dumps all his code without adding nothing more is not a good question, hence my downvote. I thought that this was somehow a shared thought, but I guess it's not. \$\endgroup\$
    – Rik Poggi
    Apr 3 '12 at 22:55
3
\$\begingroup\$

Kojiro has made many of the points I was going to. But a few more are worth making

          drive = raw_input('Select %s: ' % label)
        if not drive in str(range(0, len(drives)+1)):
            continue
        else:
            return os.path.join('/Volumes', drives[int(drive)-1])

Rather then using continue, invert the logic in the if.

        if compare_hashes(src, dst_temp_name):
            try:
                os.rename(dst_temp_name, dst)
            except Exception as e:
                print 'Error trying to rename the file: ', e

In most situations you shouldn't catch Exception. It'll catch just about anything and make it harder to see bugs. Also, do you actually want to print errors and continue on? Maybe you should shut down the script instead. At the very least catch a more specific error.

        else:
            safe_copy(src, dst)

Write this with a while loop rather then recursion. It'll be easier to read.

# hash a file and return the hash string

    try:
        local_file = open(filename,'rb')
    except Exception as e:
        print 'Error while opening file for hashing: ', e

If this error actually happens, the code below will fail because the script will still try to read from the file. If you are going to catch exceptions you need to actually do something sane about it. Printing the error is not something sane.

    local_file.close()

Use the with construct instead

    return unicode(result)

Why?

#main
def main():
    # get list of volumes
    drives = [d for d in os.listdir('/Volumes')]

This is the same as drives = os.listdir('/Volumes')

    # ask user for origin drive
    origin = choose_drive(drives, 'Origin')
    origin_files = os.listdir(origin)
    # ask user for destination drive
    destination = choose_drive(drives, 'Destination')
    destination_files = os.listdir(destination)

You should almost always be getting this information from the command line arguments not user input.

\$\endgroup\$
0
3
\$\begingroup\$

Responses in comments starting with koj -.

#!/usr/bin/env python

"""
Safely copies .mpg files from one volume to another using
temporary filenames and hash verification
"""

import os
from datetime import datetime
from hashlib import sha256
from shutil import copy2

#ask user for origin (where to get files)
def choose_drive(drives, label):
    # koj - needs documentation
    while True:
        for d in drives: # koj - use for d, drive in enumerate(drives)
            print '%s - %s' % (drives.index(d)+1,d) # koj - print '%d - %s' % (d+1,drive)
        drive = raw_input('Select %s: ' % label)
        # koj - cast to int here inside a try/except. (except ValueError, continue).
        if not drive in str(range(0, len(drives)+1)): # koj - Then, no cast to string
            continue
        else:
            return os.path.join('/Volumes', drives[int(drive)-1]) # koj - no need to cast to int anymore

# safe copy
def safe_copy(src, dst):
    # koj - needs documentation
    dst_temp_name = dst+'_INCOMPLETE'
    try:
        copy2(src,dst_temp_name)
        if compare_hashes(src, dst_temp_name):
            try:
                os.rename(dst_temp_name, dst)
            except Exception as e:
                print 'Error trying to rename the file: ', e
        else:
            safe_copy(src, dst)
    except Exception as e:
        print 'Error trying to copy the file: ', e

# hash a file and return the hash string
def get_local_hash(filename, block_size=2**20):
    """
    Hashes a file using SHA1
    """
    # print 'Starting hash of %s at %s' % (filename, datetime.now().time())
    print 'Hashing', filename
    try: # koj - use a with statement to manage the file handle
        local_file = open(filename,'rb')
    except Exception as e:
        print 'Error while opening file for hashing: ', e
    hasher = sha256()
    while True:
        data = local_file.read(block_size)
        if not data:
            break
        hasher.update(data)
    # print 'Finished hash at %s' % datetime.now().time()
    result = hasher.hexdigest()
    local_file.close()
    return unicode(result)

def compare_hashes(file1, file2):
    # koj - needs documentation
    if get_local_hash(file1) == get_local_hash(file2):
        return True # koj - sure, but why not just return get_local_hash(file1) == get_local_hash(file2) ?

#main
# koj - did you really need that label?
def main():
    # get list of volumes
    drives = [d for d in os.listdir('/Volumes')]
    # ask user for origin drive
    origin = choose_drive(drives, 'Origin')
    origin_files = os.listdir(origin) # koj - filter here: (orig for orig in os.listdir(origin) if orig.endswith('.mpg'))
    # ask user for destination drive
    destination = choose_drive(drives, 'Destination')
    destination_files = os.listdir(destination)
    # for each file on origin, check if it exists on destination
    for of in origin_files:
        if of.endswith('.mpg'): # koj - see filter on line 76 above, don't need this.
            if of in destination_files:
                if compare_hashes(os.path.join(origin, of), os.path.join(destination, of)):
                    print 'already exists and is identical... skipping', of
                else:
                    print 'already exists but is different... copying', of
                    safe_copy(os.path.join(origin, of), os.path.join(destination, of))
            else:
                print 'does not exists... copying', of
                safe_copy(os.path.join(origin, of), os.path.join(destination, of))




if __name__ == "__main__":

    main()
\$\endgroup\$
2
  • \$\begingroup\$ +1. Good stuff. My only suggestion is that putting your discussion in comments isn't the most readable. See how I've put mine for a possible alternative. \$\endgroup\$ Apr 3 '12 at 21:33
  • 3
    \$\begingroup\$ Wait… if you're reviewing my code review, shouldn't that go on meta.cr.se? (j/k) \$\endgroup\$
    – kojiro
    Apr 3 '12 at 21:40

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.