4
\$\begingroup\$

I have some code that searches for duplicate files and then removes those files from a csv. To make the set of files I am searching over much smaller, I first group the files by size; if a file has a size that it is unique, it is safe to assume that it is unique and does not need to be included in the set of files I am checking for duplicates. Additionally, I must only consider files that are listed in the csv. Occasionally the csv has filenames of files that are not in the directory, which leaves me with several choices.

  1. I can initially check my list of files for those which do not exist and remove them. This adds a loop over all the files and seems unnecessary/inefficient; it is very rare that a file does not exist.
  2. Use the following code with exception handling when the file is not found and delete file from my list at this point. However this introduces a side effect to the grouping function. Clearly this function does more than its name suggests.

Code:

def group_by_size(self, files_to_group):
    grouped_by_size = dict()
    for fileid in files_to_group:
        full_path = os.path.join(self.folder, fileid)
        try:
            size = os.path.getsize(full_path)
        except WindowsError:
            print WindowsError
            print "The file wasn't found. Deleting row from PremsPhotos."
            del self.files[fileid]
            del self.spreadsheet_reader.main_row_dict[fileid]
        if grouped_by_size.get(size):
            grouped_by_size[size].append(fileid)
        else:
            grouped_by_size[size]=[]
            grouped_by_size[size].append(fileid)
    return grouped_by_size

Alternatively, eliminate duplicates beforehand:

    def remove_nonexistant_photos(self):
        for photo in self.files:
            os.path.exists(os.path.join(self.folder, photo))

The context of this code is in a script that will likely be run a lot, but will not likely be modified once it works. Looping over all the files in the directory takes about half a second when there are 15k files in the directory. With both of these in mind, I suppose the decision is not of huge importance, but I have been doing a lot of reading about best practices, and want to improve my coding intuition/style as I go forward.

\$\endgroup\$
1
\$\begingroup\$

The group_by_size function has multiple responsibilities:

  • Group files by size
  • Verify file
  • Modify attributes of the class (self.files, self.spreadsheet_reader.main_row_dict)

It would be better to reorganize your code so that each function has a single responsibility.


There is some code duplication here that could be improved:

if grouped_by_size.get(size):
    grouped_by_size[size].append(fileid)
else:
    grouped_by_size[size]=[]
    grouped_by_size[size].append(fileid)

Like this:

files = grouped_by_size.get(size)
if files is None:
    files = []
    grouped_by_size[size] = files
files.append(fileid)
\$\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.