5
\$\begingroup\$

What improvements could be made to this script and can you explain why you would make those changes?

import hashlib
import os
import time
time = time.strftime("%Y%m%d-%H%M%S")
Masterhash = open("Filehash" + time, "a")
#get a list of files from your directory
for filename in os.listdir('.'):
    #ignore directory names
    if not os.path.isdir(filename):
        #open the file for readying
        primingfiletohash = open(filename, "r")
        #read contents of file
        filetohash = primingfiletohash.read()
        #close the file
        primingfiletohash.close()
        #use hashlib to generate an md5 hash of the file contents
        Filehash = hashlib.md5(filetohash)
        Filehash = Filehash.hexdigest()
        #write file names and hashes to a file
        Masterhash.write(filename + "\t")
        Masterhash.write(Filehash + "\n")
        #close the file
Masterhash.close()
\$\endgroup\$
3
\$\begingroup\$

First of all, I'd make a variable which will keep the current path. For that, I'd use os.getcwd(...):

current_path = getcwd()

From the docs:

Return a string representing the current working directory.

After this, I'd normalize the path, so that your script will be able to run on multiple OSs:

current_path = normpath(getcwd())

From the docs:

Normalize a pathname by collapsing redundant separators and up-level references so that A//B, A/B/, A/./B and A/foo/../B all become A/B. This string manipulation may change the meaning of a path that contains symbolic links. On Windows, it converts forward slashes to backward slashes.

Then, I'd get a list of all the files in the directory using listdir(...):

only_files = [join(current_path, f) for f in listdir(current_path) if isfile(join(current_path, f))]

After getting the files, I'd wrap everything within a function:

def get_files():
    current_path = normpath(getcwd())
    return [join(current_path, f) for f in listdir(current_path) if isfile(join(current_path, f))]

Then, I'd make another function which will return a list of hashes for all the files in the list:

def get_hashes():
    files = get_files()
    list_of_hashes = []
    for each_file in files:
        hash_md5 = hashlib.md5()
        with open(each_file, "rb") as f:
            for chunk in iter(lambda: f.read(4096), b""):
                hash_md5.update(chunk)
        list_of_hashes.append('Filename: {}\tHash: {}\n'.format(basename(each_file), hash_md5.hexdigest()))
    return list_of_hashes

After this, I'd just write this list of hashes in the desired file:

def write_hashes():
    hashes = get_hashes()
    with open('list_of_hashes.txt', 'w') as f:
        for md5_hash in hashes:
            f.write(md5_hash)

Finally, the code would look like this:

from os import listdir, getcwd
from os.path import isfile, join, normpath, basename
import hashlib


def get_files():
    current_path = normpath(getcwd())
    return [join(current_path, f) for f in listdir(current_path) if isfile(join(current_path, f))]


def get_hashes():
    files = get_files()
    list_of_hashes = []
    for each_file in files:
        hash_md5 = hashlib.md5()
        with open(each_file, "rb") as f:
            for chunk in iter(lambda: f.read(4096), b""):
                hash_md5.update(chunk)
        list_of_hashes.append('Filename: {}\tHash: {}\n'.format(basename(each_file), hash_md5.hexdigest()))
    return list_of_hashes


def write_hashes():
    hashes = get_hashes()
    with open('list_of_hashes.txt', 'w') as f:
        for md5_hash in hashes:
            f.write(md5_hash)


if __name__ == '__main__':
    write_hashes()

Now, why I'd do it this way:

  1. Compare your code with this new one, and tell me which one is more readable.
  2. You should read PEP8, which is the standard style guide for python.
  3. You should use with() when opening a file as this will make sure the file will be closed when needed.
  4. I've used for chunk in iter(lambda: f.read(4096), b"") which is a better approach for hashing large files (sometimes you won't be able to fit the whole file in memory. In that case, you'll have to read chunks of 4096 bytes sequentially).
  5. I've split the logic of your main program into multiple functions which I'll let you document using docstrings:

E.g:

def get_files():
    """Returns a list of all files in the current directory"""
    # ...
  1. You can see that I also added if __name__ == '__main__'. By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.

To be considered:

MD5 is known broken and (IMHO) should come with scary deprecation warnings and removed from the library, so you should actually do the hashing thing using sha256 from the the same module:

Remember:

Each statement that you write is important, and if changing the name of a local variable / method / anything with a descriptive name makes your code a lot clearer and more readable then do it!

\$\endgroup\$
  • \$\begingroup\$ I would say that writing an intermediate function to check for "should hash" may be warranted. Depending on the exact needs, I would expect both files for which isfile as well as islink are true to be hashed. But it is probably also clear that not isdir is an incorrect choice (you really don't want to hash neither block nor char devices). \$\endgroup\$ – Vatine Nov 16 '16 at 16:30
2
\$\begingroup\$

Import what you need

Do not import whole modules if you are going to use only few of its members (functions, classes, etc.) but import those members directly if they do not cause name clashes.

from os import getcwd, listdir
from os.path import join, isfile
from time import strftime
from hashlib import md5

This gives other readers of your code a direct overview about which module members you are actually using in your program. Another perk of this style is that you will have shorter lines since you do not need to specify each member's namespace. This is important since PEP 8 specifies a maximum of 79 characters per line of code.

Also check the order of your imports. It is common to have imports close to the system like os and os.path at the top of the module.

Divide an conquer

Your program has several concerns:

  1. Listing files within a directory.
  2. Generating an MD5 hash for each single file.
  3. Writing the hashes into a file.

In your simple case an object-oriented approach might well be overkill. But consider breaking your code down into several functions:

def list_files(basedir=None):
    """List only files within the respective directory"""

    if basedir is None:
        basedir = getcwd()

    for item in listdir(basedir):
        path = join(basedir, item)

        if isfile(path):
            yield path


def md5sum(f, block_size=None):
    """Returns the MD5 checksum of the respective file"""

    if block_size is None:
        block_size = 4096

    hash = md5()

    with open(f, 'rb') as fh:
        block = fh.read(block_size)

        while block:
            hash.update(block)
            block = fh.read(block_size)

    return hash.hexdigest()


def md5sums(basedir=None, block_size=None):
    """Yields (<file_name>, <md5sum>) tuples
    for files within the basedir.
    """

    for f in list_files(basedir=basedir):
        yield (f, md5sum(f, block_size=block_size))


if __name__ == '__main__':
    hash_file = strftime('Filehash%Y%m%d-%H%M%S')

    with open(hash_file, 'w') as fh:
        for file_hash in md5sums():
            fh.write('\t'.join(file_hash) + '\n')

Code style guidelines

Especially in Python there are strict code style guidelines mainly defined in PEP 8. These include snake_case names for functions and variable names.

Comments

Comment only what needs to be commented. Comments, introduced by #, are meant to clarify why certain code is written as it is. This is for you and other developers. Do not state the obvious e.g. commenting that stuff is written to a file before invoking Masterhash.write.

Docstrings

It is also a good practice to document your functions (and classes or methods) with docstrings. They should describe briefly what the respective function / class is doing.

Chose your tools carefully

As another user already pointed out, MD5 has a relatively high risk of collisions compared to other modern hash algorithms. So consider using another one e.g. SHA-256.

\$\endgroup\$

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.