Like you noticed, no need to use multiple loops when you can do everything in one function and loop only once.
Aside from that, there are many small things that can be improved with this script:
- This script doesn't work if the script is not in the same directory as the source directory. Correct if me I am wrong, but this is what I encountered when I tested the code. To allow for this flexibility (of using this script at any arbritary location), it is necessary to use absolute paths.
- We should probably check if the directory we're about to create already exists. It may be possible that two images may have the same basename but different extensions. For this case, using
os.path.exists will save us down the road and is simple to implement.
- I noticed you excluded
.DS_Store from filenames. I took it a step further and excluded all hidden files and files that started with
__ (like Python build directories/files).
- When using a loop comprehension for the sake of the for loop and not for the results, there's no need to store the output. For example, when using
a = [making(directory) for directory in dirpaths], it is more useful to only write
[making(directory) for directory in dirpaths] on that line. Storing it as a result implies that the output is being used somewhere later in the script, when in this case, it is not useful in the final version.
- It is better to use
shutil.move for the
movingfiles function. Under the hood, Python will decide whether to use
os.rename or if a copy-remove operation is neccesary (see shutil.move for more information).
os.makedirs is overkill for this example here.
os.makedirs is meant for recursively creating directories, not for single-level directory creation. The similiarily named
os.mkdir is more appropriate here and probably will be more performant.
- It is not necessary to check if the source directory string contains a
'/'. On the documentation,
os.path.join verifies that the result will have "exactly one directory seperator". Therefore, we don't have worry about extra logic.
- Normally, I wouldn't bother with optimization when the context of the script is for running a few files. In the case of the thousands of objects, however, it is crucial to reduce how many loops we run. In the code below, you see implicit use of generators. Specifically, we will lazily evaluate our list of filenames upto the point we process each filename. This means we only run through our list of filenames once(!)
- On the topic of performance, when we have a custom function defined (read: not
lambda), it almost always better to use
map when we don't require storing the output. On Python2, the advantage was neglible, but on Python3, the difference is noticable. So we will use
map below. (
map is very much faster when using builtin Python functions that call on C code. Why? Because
map also reaches down to C, meaning you get a barebones C loop for those operations. Looping in Python is unintuitively bloated compared to C.)
- The use
os.path.splitext is correct for this script. However, there is a slight performance cost to using this function, and so I opted for the leaner
.split('.', 1) which is faster. As long as the filenames don't contain
'.' other than at the end as an extension, we'll be fine.
def mkdir_move_file(file, source):
directory = file.split('.', 1)
if not os.path.exists(directory):
if not os.path.exists(args.source):
raise Exception('Source folder cannot be found or does not exist!')
filenames = (os.path.join(source, x) for x in next(os.walk(source))
if not x.startswith('.') and not x.startswith('__'))
# basically the same as [mkdir_move_file(file, source) for file in filenames]
map(functools.partial(mkdir_move_file, source=source), filenames)
if __name__ == '__main__':
parser = argparse.ArgumentParser(description='Make folders for loose files in folder.')
parser.add_argument('source', action='store', type=str, help='The source directory to process.')
args = parser.parse_args()
Some further notes, beyond the initial scope of the question:
- Python makes it simple to create scripts-- the key is using the
argparse library. This library lets us parse arguments given through the command line, which can then be passed along the functions internally. Simply add parsing to your script and you can use the executable script from the command line!
- If our
mkdir_move_all_files function was more complicated/intensive, we could use asynchronous programming to speed up our overall program. That way we can fire off multiple evaluations without waiting for the result to complete. However, I don't see it being hugely beneficial for this case.
If you have any questions let me know.