I support all comments in xander27's answer, especially RE indentation vs
#end. With regards to naming, I'll add one additional comment - instead of giving a name to a variable you never use, just name it
hasFile implementation could be much simpler; what you really want is
return any(len(file_list) > 0 and not (len(file_list) == 1 and file_list == "Thumbs.db") for _, _, file_list in os.walk(p_path))
That's a little verbose, so I'll split it up into two functions.
return len(file_list) > 0 and not (
len(file_list) == 1 and file_list == "Thumbs.db"
for _, _, file_list in os.walk(p_path)
You can improve performance/correctness by modifying
dirs; from the documentation (emphasis mine):
True, the caller can modify the
dirnames list in-place, and
walk() will only recurse into the subdirectories whose names remain in
dirnames; this can be used to prune the search, impose a specific order of visiting, or even to inform
walk() about directories the caller creates or renames before it resumes
shutil.rmtree a directory, you should take it out of the list so that
os.walk doesn't bother continuing to recurse. This is fine if you keep your current approach, but I think I have a better option in my next section.
When you look even closer at
hasFile, you realize that they're basically the same function - one checks if there are any files in the directory, while the other checks if there are any folders in the directory and if so recurses (effectively, not literally). This is actually really weird when you realize that
os.walk already traverses all subdirectories; by doing it again in
hasFiles you're really just confusing things. Ultimately, a directory is empty if two things are true:
- It has no files (except
- It has no non-empty directories
We already have all of the information to determine whether or not this is true, and we can do so like this:
- Get the directories, bottom-up instead of top-down
- If the directory meets the requirements, keep track of it
- Once you've categorized all of them, clean house.
You don't necessarily have to get the full list of them before you clean things if you use a generator, like so:
return len(files) > 0 and not (
len(files) == 1 and files == "Thumbs.db"
path, subdirectories, cache
for subdirectory in subdirectories
cache = defaultdict(lambda _: False)
for root, subdirectories, files in os.walk(
has_files = directory_has_files(files)
if not subdirectories:
cache[root] = has_files
] = directory_has_nonempty_subdirectories(
root, subdirectories, cache
if not cache[root]:
for empty_directory in root_path:
You may notice that I've used
defaultdict to make it a bit easier to find out if something is known to be empty. Otherwise, I'm taking advantage of the bottom-up approach to avoid having to calculate something repeatedly on the way down.
If you don't want to issue as many
shutil.rmtree commands, you could make it a bit more clever, and only report the highest-level empty directory. For example, you could do this at the end of
find_empty_directories to not report until you find a non-empty, and then do the children that are empty.
for subdirectory in subdirectories:
subpath = os.path.join(root, subdirectory)