2
\$\begingroup\$

I wrote this little snippet which moves files from several subfolders to their parent folder. Since the snippet uses different nested for loops I was wondering if it could be refactored to a cleaner and more maintainable solution? I tried extracting the different for loops to separate functions, but they rely on the same variables so I couldn't get this to work.

import os
import shutil
import sys

currentDirectory = os.getcwd()

def changeStructure(currentFolder):
    parentDirectory = currentDirectory + "/" + currentFolder
    for folder in os.listdir(parentDirectory):
        folderPath = parentDirectory + "/" + folder
        if os.path.isdir(folderPath): 
            for file in os.listdir(folderPath):
                    if not file.startswith('.'): 
                        shutil.move(folderPath + "/" + file, parentDirectory)
            shutil.rmtree(folderPath)
    print("All folders removed")

if __name__ == '__main__':
    globals()[sys.argv[1]](sys.argv[2])
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

You should replace os with pathlib equivalents since it has a nicer, object-oriented interface.

currentDirectory (which should be called current_directory) should not be a global. In a situation where you load the module, change the working directory and then call changeStructure this will do the wrong thing.

Add PEP484 type hints.

Consider inverting your if isdir condition to be able to de-dent the rest of the loop.

Your use of globals() is evil. It allows people invoking your application unfettered access to anything that you import and anything that you define on the global namespace, even if it starts with underscores. Do basically anything that isn't this. Since this is a single-method program, in this case just pass the first argument directly to your subroutine.

Suggested

Not tested because I don't want to mangle my filesystem.

import shutil
import sys
from pathlib import Path


def change_structure(current_folder: str) -> None:
    parent_dir = Path.cwd() / current_folder
    for folder in parent_dir.iterdir():
        if not folder.is_dir():
            continue

        for child in folder.iterdir():
            if not child.name.startswith('.'):
                child.rename(parent_dir / child.name)
        shutil.rmtree(folder)
    print("All folders removed")


if __name__ == '__main__':
    change_structure(sys.argv[1])
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.