2
\$\begingroup\$

This is my third (and hopefully final) time re-writing this program. It was recommended I use Pathlib and it simplified my code, it's easier to read and understand than the first two monstrosities.

I also changed the way it functions, instead of the user providing a text file, the user creates a directory called Repository, which contains sub directories of images that will be renamed, and a destination directory (R:\Pictures for me).

Suppose I have the following directories under Repository:

C:\Users\Kiska\Repository\Computers
   - comp-amd-12342.jpg
   - 7546345233-pcmag.jpg

C:\Users\Kiska\Repository\Landscape
   - land-345345.jpg
   - end-field.jpg

The program will move and rename it like this:

R:\Pictures\Pictures\Computers
   - computers_1.jpg
   - computers_2.jpg

R:\Pictures\Pictures\Landscape
   - landscape_1.jpg
   - landscape_2.jpg

Code:

import os
from pathlib import Path
import shutil

def check_if_folder_exists(folder : Path):
    if not Path.exists(folder):
        raise FileNotFoundError("{0} is not available".format(folder))


def main():      


    try:

        repository = Path("{0}\\{1}".format(os.getenv("USERPROFILE"), "Repository"))
        desination_root = Path("R:\\Pictures")

        check_if_folder_exists(folder=repository)
        check_if_folder_exists(folder=desination_root)

        for folder in repository.iterdir():
            destination = Path(desination_root).joinpath(folder.stem)
            Path.mkdir(destination, exist_ok=True)

            start = len(os.listdir(destination)) + 1

            for number, image in enumerate(folder.iterdir(), start=start):
                image_name = "{0}_{1}{2}".format(folder.stem.lower().replace(" ","_"), number,image.suffix)
                destination_image = Path(destination).joinpath(image_name)
                print(f"{image} will be named {destination_image}")
                shutil.move(image, destination_image)


    except(ValueError, FileNotFoundError, OSError) as error:
        print(error)
    finally:
        pass

if __name__ == '__main__':
    main()
\$\endgroup\$
3
\$\begingroup\$
  • You have lots of unneeded empty lines, they also hinder readability.
  • fn(arg=value) is abnormal. It's common to use fn(value). It's common only to use the former when the argument is a keyword or default argument.
  • You don't need to make a path another path.

    desination_root = Path("R:\\Pictures")
    destination = Path(desination_root).joinpath(folder.stem)
    
  • Path.mkdir isn't a static method, and so it's abnormal to use it as Path.mkdir(my_path) rather than my_path.mkdir().

  • check_if_folder_exists isn't needed as repository.iterdir and destination.mkdir will error if they don't exist.

    It should also be noted that you can pass parents=True to Path.mkdir to automatically create the folder if it doesn't exist, rather than force the user to do that.

  • It's better if you make another function that moves the files, and one, main, that passes it the values. Preferably main would be changed to not use static values and get the values from a config file.
  • finally isn't needed.
  • You should raise a SystemExit(1) or use sys.exit(1) if there is an error. They both do the same thing which notifies the system that the program failed. It didn't run without problems.
  • Given that generating a name that doesn't conflict with existing files is not a simple one-liner I'd recommend you move it out into a function.

    I should note that it may overwrite files, if you have a folder with file_1.png and file_3.png your code's going to overwrite file_3.png, but not have a file_2.png.

You changed away from some of the above, and so I'd recommend you think about the differences, which is easier to re-use, which is easier to read, which performs the correct action in the situation. If you don't know what the difference is even with my explanation, such as with SystemExit(1), then you should look up what it does.

import os
from pathlib import Path
import shutil


def gen_name(path, number):
    orig_name = path.parent.name.lower().replace(" ", "_")
    return f"{orig_name}_{number}{path.suffix}"


def move_files(src, dest):
    for folder in Path(src).iterdir():
        destination = Path(dest) / folder.stem
        destination.mkdir(exist_ok=True, parents=True)
        start = len(os.listdir(destination)) + 1
        for number, image in enumerate(folder.iterdir(), start=start):
            image_name = gen_name(image, number)
            destination_image = destination / image_name
            print(f"{image} will be named {destination_image}")
            shutil.move(image, destination_image)


def main():
    try:
        move_files(
            "{0}\\{1}".format(os.getenv("USERPROFILE"), "Repository"),
            "R:\\Pictures",
        )
    except(ValueError, FileNotFoundError, OSError) as error:
        print(error)
        raise SystemExit(1) from None


if __name__ == '__main__':
    main()

I think it could make more sense for gen_name to be a closure, as then it can correctly handle existing files. I won't change how the code functions, but will show how it could be implemented.

def gen_name_builder(folder):
    orig_name = folder.name.lower().replace(" ", "_")
    number = len(os.listdir(destination))
    def gen_name(file):
        nonlocal number
        number += 1
        return f"{orig_name}_{number}{path.suffix}"
    return name


def move_files(src, dest):
    for folder in Path(src).iterdir():
        destination = Path(dest) / folder.stem
        destination.mkdir(exist_ok=True, parents=True)
        gen_name = gen_name_builder(destination)
        for image in folder.iterdir():
            image_name = gen_name(image)
            destination_image = destination / image_name
            print(f"{image} will be named {destination_image}")
            shutil.move(image, destination_image)
\$\endgroup\$
  • \$\begingroup\$ Is it okay to use an if/else statement in the for loop responsible for moving the images? Check if it exists, if not, move it, if it does, leave it, but notify the user with a print statement \$\endgroup\$ – киска Jul 15 at 21:58
  • \$\begingroup\$ @Kiska Please look over the additional code I added to my answer. You should be able to handle if an item exists with that name in gen_name. Yes, you can use an if. As a hint I would use a while. \$\endgroup\$ – Peilonrayz Jul 15 at 22:06
  • 1
    \$\begingroup\$ @Kiska I'm not seeing the docs use Path.mkdir as a static method. I've checked the other languages too, and they also don't show that. I'm sorry that this has caused some confusion. \$\endgroup\$ – Peilonrayz Jul 15 at 22:11
  • 1
    \$\begingroup\$ Right at the top in yellow where you can link to it Path.mkdir(mode=0o777, parents=False, exist_ok=False). \$\endgroup\$ – киска Jul 15 at 22:12
  • 1
    \$\begingroup\$ @Kiska Ok, I get the confusion now. That isn't an example of how to use the method, it tells you where the method is defined, Path.mkdir, and shows the arguments it has, with any defaults they have. If you look at Path.open you can see the same thing, but it has an example showing it used in a different way. \$\endgroup\$ – Peilonrayz Jul 15 at 22:16
2
\$\begingroup\$
  • Give main method a more meaningful name. It is always better than a generic main.
  • Make repository and destination_root be parameters of the method which is now called main;
  • If you use python version that supports f-strings, be consistent, use them everywhere e.g. f'{folder} is not available';
  • finally: pass does not have any effect so if you are not going to put any logic there, it can be omitted;
  • regarding the start parameter passed to enumerate: can you guarantee that if that the destination folder contains 1 picture, it will be called xxxxx_1.jpg, not xxxxx_2.jpg or xxxxx_27.jpg?
  • I would also extract folder.stem.lower().replace(" ","_") into a separate variable which can get some meaningful name (which can be then used in f-string) to enhance reading;
\$\endgroup\$
  • \$\begingroup\$ The start parameter is the number of images already present + 1 so if 4 images already exist in the destination, we start at 5. \$\endgroup\$ – киска Jul 15 at 19:43
  • 1
    \$\begingroup\$ @Kiska Okay, let me rephrase, what would happen if before you run the program R:\Pictures\Pictures\Computers contained one file with the name computers_2.jpg? \$\endgroup\$ – Hlib Babii Jul 15 at 19:53
  • \$\begingroup\$ Ah! I see now what you mean. I should check first if the filename is already present in the destination directory. \$\endgroup\$ – киска Jul 15 at 21:16

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.