5
\$\begingroup\$

I needed this to clean up a couple of similar music libraries, which also contained some empty files and duplicate files with different names.

# dupFinder.py
import os, sys, shutil, datetime
import hashlib

def findDup(parentFolder):
    # Dups in format {hash:[names]}
    dups = {}
    for dirName, subdirs, fileList in os.walk(parentFolder):
        print('Scanning %s...' % dirName)
        for filename in fileList:
            # Get the path to the file
            path = os.path.join(dirName, filename)
            print(path)
            # Calculate hash
            file_hash = hashfile(path)
            # Add or append the file path
            if file_hash in dups:
                dups[file_hash].append(path)
            else:
                dups[file_hash] = [path]
    return dups


# Joins two dictionaries
def joinDicts(dict1, dict2):
    for key in dict2.keys():
        if key in dict1:
            dict1[key] = dict1[key] + dict2[key]
        else:
            dict1[key] = dict2[key]


def hashfile(path, blocksize = 65536):
    print(path)
    afile = open(path, 'rb')
    hasher = hashlib.md5()
    buf = afile.read(blocksize)
    while len(buf) > 0:
        hasher.update(buf)
        buf = afile.read(blocksize)
    afile.close()
    return hasher.hexdigest()


def handleResults(dict1):
    results = list(filter(lambda x: len(x) > 1, dict1.values()))
    if not 'testrun' in globals():
        backupdir = 'removed_' + str(datetime.datetime.now())
        if not os.path.exists(backupdir):
            os.makedirs(backupdir)
    else:
        backupdir = 'backup_dir_date_now'
    if len(results) > 0:
        print('Duplicates Found:')
        print('The following files are identical. The name could differ, but the content is identical')
        print('^^^^^^^^^^^^^^^^^')
        count = 0
        for result in results:
            for subresult in result:
                if count == 0:
                    print('KEEPING ONE COPY: %s.' % subresult)
                    count += 1
            elif 'testrun' in globals():
                print('\t\t %s WILL BE REMOVED TO: %s' % (subresult, backupdir))
            else:
                print('\t\tREMOVING %s TO %s' % (subresult, backupdir))
                try:
                    shutil.move(subresult, backupdir)
                except shutil.Error:
                    print ('\t\t EXISTS. DELETING.')
                    os.remove(subresult)
                print('___________________')

    else:
        print('No duplicate files found.')


if __name__ == '__main__':
    if len(sys.argv) > 1:
        dups = {}
        if sys.argv[1] == '-t':
            testrun = 1
            folders = sys.argv[2:]
        else: folders = sys.argv[1:]
        for i in folders:
            # Iterate the folders given
            if os.path.exists(i):
                # Find the duplicated files and append them to the dups
                joinDicts(dups, findDup(i))
            else:
                print('%s is not a valid path, please verify' % i)
                sys.exit()
        handleResults(dups)
    else:
        print('Usage: python dupFinder.py [-t] folder or python [-t] dupFinder.py folder1 folder2 folder3')

It can be run in test mode with -t flag.

Wow - awesome advice here sure transformed the code: GitHub Repo

\$\endgroup\$
3
\$\begingroup\$
  • Imports should be separated onto multiple lines, not imported on the same line, as seen in the below example.

import import 
import os
import sys
import shutil
import datetime

  • If you're also going to be using only a couple function/class/variables from a module, you can import them using from ... import ....
  • Functions and variables should be in underscore_case, and classes should be in PascalCase.
  • You have a lot of code underneath if __name__ == "__main__":. Consider finding a way to separate this code into a main method. if __name__ == "__main__": should usually be about as simple as this.

if __name__ == "__main__":
    main( ... )

  • You have some inconsistent indentation. E.g, some places are indendented by four spaces/a tab, and others, well, more spaces/tabs.
  • Rather than putting comments over functions, use docstrings to describe the purpose of a function, like this.

def my_function(args):
    """
    Describe the purpose of your function
    here and it's arguments.
    """
    ...

  • My final tip would be to read PEP8, Python's official style guide, and see how you can properly format your code.

If there's anything else that you want me to cover, just mention it in the comments, and I'll see what I can do.

\$\endgroup\$
  • \$\begingroup\$ That's great criticism. I've read PEP8 a couple of times, but obviously could use a review. Why is it that imports are supposed to be on their own line, again? \$\endgroup\$ – MikeiLL Jun 14 '15 at 2:19
  • 1
    \$\begingroup\$ @MikeiLL If there's an error associated with a module it makes debugging easier. \$\endgroup\$ – Ethan Bierlein Jun 14 '15 at 3:56
  • \$\begingroup\$ Posting updated version based on above. \$\endgroup\$ – MikeiLL Jun 14 '15 at 5:27
  • 1
    \$\begingroup\$ @MikeiLL Do not post your refactored code in your post. It is against site rules. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Ethan Bierlein Jun 14 '15 at 5:29
  • 1
    \$\begingroup\$ @MikeiLL The only explanation I can really provide is that it makes your code cleaner and clearer, \$\endgroup\$ – Ethan Bierlein Jun 14 '15 at 5:55
4
\$\begingroup\$

If I had to pick a least-favourite line in the program, it would be this one:

if 'testrun' in globals():

… because

  • Global variables are bad.
  • You didn't even declare testrun as a global variable using global testrun, but rather you used introspection.
  • You didn't test the flag to see whether it was True or False; you just check for its existence.
  • The whole problem could have been avoided by calling handleResults() with a parameter.

print('%s is not a valid path, please verify' % i)
sys.exit()

When you exit the program due to an error, exit with a non-zero status.

i is an unconventional iteration variable, since it has the connotation of being an integer. The code would be more readable if you had written for folder in folders: ….


In hashfile(), you should call open() using a with block.

Since hashfile() seems like it could be a basic reusable operation, I would avoid having it print anything. The findDup() function, which calls hashfile(), prints status updates, and in fact it prints the same path just before calling hashfile().

\$\endgroup\$
  • \$\begingroup\$ Nice. Based on @EthanBierlein's advice had already moved the work into a function to which sys.argv is sent as a parameter. That (i) in the loop bothered me a bit too so thanks for that. Also had wondered about using with in the open block, but thought that maybe with the block/chunksize being declared it wasn't necessary. But the 'controlled_execution' idea makes sense: effbot.org/zone/python-with-statement.htm. Here's current: github.com/MikeiLL/dupFinder in case it's of use to anyone. \$\endgroup\$ – MikeiLL Jun 14 '15 at 16:24
2
\$\begingroup\$

Just reviewing the hashfile function.

  1. There's no docstring. What does this function do?

  2. The MD5 hash is not collision-resistant: this means that an attacker can create two files with the same hash. This could be disastrous if hashfile were used for data integrity or content de-duplication. Use SHA-256 instead.

  3. The condition len(buf) > 0 can be written more simply as buf.

  4. The duplicated line of code here:

    buf = afile.read(blocksize)
    while len(buf) > 0:
        hasher.update(buf)
        buf = afile.read(blocksize)
    

    can be avoided if you put the loop termination condition in the middle, like this:

    while True:
        buf = afile.read(blocksize)
        if not buf:
            break
        hasher.update(buf)
    

    This is a standard pattern in Python: see PEP 314.

\$\endgroup\$
  • \$\begingroup\$ I confess much of this code is "found" code, but the docstring I came up with is "Create an md5 hash instance from a file by reading the binary data in blocks of 2**16 bytes and updating hash accordingly." Will check out that PEP and work with your suggestion. And also try out using SHA-256 instead, but is it just for security that you recommend it, 'cause this is just a "shed" tool at the moment. \$\endgroup\$ – MikeiLL Jun 15 '15 at 2:51
  • \$\begingroup\$ Swapping out hash method was sure easy. And putting the test within the loop is also really nice to avoid the code duplication. First time I've been referred to a rejected PEP. \$\endgroup\$ – MikeiLL Jun 15 '15 at 17:13

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.