5
\$\begingroup\$

In an attempt to clean out my picture collection I made a script to find all duplicate files

import datetime
import hashlib
import json
import typing
from collections import defaultdict
from pathlib import Path


def filesize(path: Path) -> int:
    """returns the filesize"""
    return path.stat().st_size


def hash_first(
    path: Path, hash_function=hashlib.md5, blocksize: int = 65536
) -> str:
    """returns the hash of the first block of the file"""
    with path.open("rb") as afile:
        return hash_function(afile.read(blocksize)).hexdigest()


def hash_all(path: Path, hash_function=hashlib.md5, blocksize=65536) -> str:
    """returns the hash of the whole file"""
    with path.open("rb") as afile:
        hasher = hash_function()
        buf = afile.read(blocksize)
        while buf:
            hasher.update(buf)
            buf = afile.read(blocksize)
    return hasher.hexdigest()


def compare_files(
    comparison: typing.Callable[[Path], typing.Hashable], duplicates: dict
) -> dict:
    """
    Subdivides each group along `comparison`
    discards subgroups with less than 2 items
    """
    results: defaultdict = defaultdict(list)
    for old_hash, files in duplicates.items():
        for file in files:
            results[(*old_hash, comparison(file))].append(file)

    return {
        filehash: files
        for filehash, files in results.items()
        if len(files) > 1
    }


def find_duplicates(
    rootdir: Path,
    comparisons: typing.Iterable[
        typing.Callable[[Path], typing.Hashable]
    ] = (),
):
    """
    Finds duplicate files in `rootdir` and its subdirectories

    Returns a list with subgroups of identical files

    Groups the files along the each of the comparisons in turn.
    Subgroups with less than 2 items are discarded.

    Each of the `comparisons` should be a callable that accepts a
    `pathlib.Path` as only argument, and returns a hashable value

    if `comparisons` is not defined, compares along:
        1. file size
        2. MD5 hash of the first block (65536 bytes)
        3. MD5 hash of the whole file
    """
    if not comparisons:
        comparisons = filesize, hash_first, hash_all

    duplicates = {(): filter(Path.is_file, rootdir.glob("**/*"))}
    for comparison in comparisons:
        duplicates = compare_files(comparison, duplicates)
    return [tuple(map(str, files)) for files in duplicates.values()]


if __name__ == "__main__":
    pic_dir = r"./testdata"
    results = find_duplicates(rootdir=Path(pic_dir))

    # print(json.dumps(results, indent=2))

    timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S")
    output_file = Path("./output") / f"{timestamp}.json"
    with output_file.open("w") as fh:
        json.dump(results, fh, indent=2)

Any remarks on this code?

The code is formatted with black (linelength 79), with mypy pylint and pylama as linters.

If I try to specify the dict as argument and return value to compare_files to typing.Dict[typing.Tuple[typing.Hashable, ...], typing.Iterable[Path]], mypy complains: Incompatible types in assignment (expression has type "Dict[Tuple[Hashable, ...], Iterable[Path]]", variable has type "Dict[Tuple[], Iterator[Path]]")

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

Some suggestions:

  • I would usually inline single line functions unless what they do is hard to understand from just reading the command. filesize might be one such candidate.
  • I believe that not specifying the block size can be faster, by allowing Python to use whatever block size it thinks is appropriate. This would need some actual testing though.
  • Your function arguments are only partially typed. You might want to use a stricter mypy configuration, such as the following, in your setup.cfg:

    [mypy]
    check_untyped_defs = true
    disallow_untyped_defs = true
    ignore_missing_imports = true
    no_implicit_optional = true
    warn_redundant_casts = true
    warn_return_any = true
    warn_unused_ignores = true
    
  • MD5 was optimized for cryptographic use, not for speed. Another Q&A explores options for fast non-cryptographic hashing.
  • I would avoid converting digests to strings - it might be slightly easier for debugging purposes, but data type conversions are in general very costly.
  • Optional parameters mean you have to test at least two things rather than one. Your code only ever calls the hash_* functions without optional parameters, so you might as well either inline them.
  • On a related note, static values which are used in multiple places are perfect for pulling out as constants. The hashing function and block size would be obvious candidates for this.
  • Mutable default parameter values are an accident waiting to happen. The default pattern for this is to use a default of None and to assign a default if foo is None.
  • r-strings like r"./testdata" are meant for regular expressions. Since pic_dir is not used as such it should probably be a regular string.
  • duplicates is at first a list of potential duplicates and later is trimmed in stages. This makes the code hard to follow.
  • Things like the directory/directories to include should be arguments. argparse can deal with this easily.
  • To make this more scriptable I would print the result on standard output. It's trivial to then redirect it to a file.
\$\endgroup\$
3
\$\begingroup\$
  1. Instead of rootdir.glob("**/*") you can write rootdir.rglob("*"). rglob adds "**/" automatically before the given pattern.
  2. You specify the return type of the comparison functions as Hashable. I'm not really sure why. Are you thinking that you will have more comparison functions in the future that would return something other than int or str? I think you could limit it to FileHash = Union[int, str] for now. I doubt that you will have functions that would return such hashable objects as, for example, namedtuples or frozensets.
  3. Maybe I don't understand something, but I don't really see the point why compare_files function exists. I would expect it to take as an argument an iterable of paths to the files, but instead, it takes them packed in a dictionary which doesn't make much sense to me. By taking the logic out of it to the outer find_duplicates function you will also avoid the problem with mypy complaining about incompatible types. It could look, for example, like this

    FileHash = Union[int, str]
    Hashes = Tuple[FileHash, ...]
    
    
    def find_duplicates(rootdir: Path,
                        comparisons: Iterable[Callable[[Path], FileHash]] = ()
                        ) -> List[Tuple[str, ...]]:
        if not comparisons:
            comparisons = filesize, hash_first, hash_all
    
        files = filter(Path.is_file, rootdir.rglob("*"))
    
        files_per_hashes: Dict[Hashes, List[Path]] = defaultdict(list)
        for file in files:
            key = tuple(hasher(file) for hasher in comparisons)
            files_per_hashes[key].append(file)
        duplicates = (files for files in files_per_hashes.values() 
                      if len(files) > 1)
        return [tuple(map(str, files)) for files in duplicates]
    
  4. You are missing type hints for hash_functions. Unfortunately, I'm not sure how to solve the problem with their return types. If you are on Python 3.7, you could try something like this (idea taken from https://github.com/python/typeshed/issues/2928):

    from __future__ import annotations
    
    Hasher = Union[Callable[[ByteString], hashlib._hashlib.HASH],
                   Callable[[], hashlib._hashlib.HASH]]
    ...
    def hash_first(path: Path,
                   hash_function: Hasher = hashlib.md5,
                   blocksize: int = 65536) -> str:
    ...
    

    but mypy doesn't like it. It says:

    error: Name 'hashlib._hashlib.HASH' is not defined
    

    Maybe you could dig a bit more in this direction.

  5. I also think that it would be better to make the comparisons as an Optional argument in the find_duplicates function with default value set to None because right now you use tuple as an immutable version of list (the data you keep in that tuple is homogeneous but tuples are meant to keep heterogenous data, and the number of elements in the tuples is usually fixed). As Guido van Rossum said: "Tuples are *not* read-only lists."

\$\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.