How to improve my script

[PsySc0rpi0n]

Hello.

I'm attending an online course about python3.
I'M trying to solve the following problem:

Write a script that takes 2 arguments from the command lines. Those 2 arguments are 2 folder paths in which a few images will be loaded and saved after being converted from 'jpeg' to 'png'.

By other words, I need to grab folder1 , from where the JPEG images are loaded, then we convert those images into PNG format and save them to folder2.

I want to use module Pathlib and PIL to solve this problem.

I have this code made by me but I'm not happy with it at all because I feel I'm running round and round to get to the final goal and I'm sure there's a way more logic way of doing this.

Code: Select all

import sys
from pathlib import Path
from PIL import Image

# grab first and second argument
# check if new/ exists, if not, create it
# loop through the folder and convert to png
# save to the new folder

def proc_args():
    """Process arguments from command line"""
    if len(sys.argv) < 3:
        print('Not enough parameters!')
        exit(-1)
    else:
        newpath = Path(sys.argv[2])
        try:
            newpath.mkdir(parents=True, exist_ok=False)
        except FileExistsError:
            print(f'{newpath} already exists!')
        except IOError as ioerr:
            print(f'{ioerr}: {newpath} could not be created')

    return newpath

def conv_imgs(pold_p, pnew_p):
    """Loop over files in folder"""
    for item in Path(pold_p).rglob('*.jpg'):
        img = Image.open(item)
        noext = item.with_suffix('')
        newext = noext.with_suffix('.png.png')
        tmp = Path(newext).stem
        print(tmp)
        img.save(Path(pnew_p).joinpath(tmp), 'png')

def main():
    """Main Function"""
    old_path = Path(sys.argv[1])
    new_path = proc_args()
    conv_imgs(old_path, new_path)

if __name__ == '__main__':
    main()

[neuraleskimo]

I hope it is not a graded assignment I am about to comment on.

In general, your solution looks good, but here are a few thoughts (from a very quick read):
1) I probably wouldn't worry about an existing destination directory. That is, set exist_ok to true.
2) I would warn on replacing an existing file though.
3) Small point: I would move the body of main() to if __name__ == '__main__':
4) Write a single function that returns both paths from the command line.
5) Instead of calling exit in a function, let any exceptions percolate to main. Put your catch block there and exit gracefully there.

Hope this helps and good job.

[PsySc0rpi0n]

I hope it is not a graded assignment I am about to comment on.

In general, your solution looks good, but here are a few thoughts (from a very quick read):
1) I probably wouldn't worry about an existing destination directory. That is, set exist_ok to true.
2) I would warn on replacing an existing file though.
3) Small point: I would move the body of main() to if __name__ == '__main__':
4) Write a single function that returns both paths from the command line.
5) Instead of calling exit in a function, let any exceptions percolate to main. Put your catch block there and exit gracefully there.

Hope this helps and good job.

Hi... No, this is not a graded assignment.
I'm attending an online Python course and I need to solve this problem. The course gives the solution using os.path module and it looks quite simple. However, I want to learn about the pathlib module and I'm trying to figure out how to do it. It's kind of an extra for me to learn something different. To find a different solution that the one given in the course.

I'll try to implement the suggestions on points 1, 3, 4. The other ones, need a little bit more understanding from me. They are not clear at first site to me. I mean, I need to think a little bit about them.

I already have another solution that was given to me but I don't want to just accept the solution without even try to dissect it and fully understand it. It uses dictionary comprehension and there are a few details I can't understand about it.

Finally, the solution I pasted here, I just don't like it because there is at least one part of it that it's completely awkward. I used .png.png in them .stem property because I knew it would remove the first .png instance.

It's like (if it was possible) to use a rock in a shotgun, just because you have your shotgun shell damaged. I mean, the shotgun was not meant to be used with rocks but you know you'll be able to kill the rabbit with the rock shot from the shotgun. You know what I mean?

By the way, the solution I was given is this:

Code: Select all

import pathlib


def main():
    folder1 = pathlib.Path('folder1')
    folder2 = pathlib.Path('folder2')
    sources = folder1.rglob('*.jpg')
    targets = {
        source: folder2 / source.relative_to(folder1).with_suffix('.png')
        for source in sources
    }
    for source, target in targets.items():
        print(source, target)


main()

[PsySc0rpi0n]

Ok, I changed my code to the following:

Code: Select all

"""Images converter"""
import sys
from pathlib import Path
from PIL import Image

def ret_args():
    """Process command line arguments"""
    paths = [Path(sys.argv[1]), Path(sys.argv[2])]
    return paths

def create_dir(pnew_p):
    """Create needed dirs"""
    newpath = Path(pnew_p)
    try:
        newpath.mkdir(parents=True, exist_ok=True)
    except IOError as ioerr:
        print(f'{ioerr}: {newpath} could not be created!')
        return -1
    return 0

def conv_imgs(pold_p, pnew_p):
    """Loop over files in folder"""
    old = Path(pold_p)
    new = Path(pnew_p)
    sources = old.rglob('*.jpg')
    targets = {
        source: new / source.relative_to(old).with_suffix('.png')
        for source in sources
    }
    for source, target in targets.items():
        print(source, target)
        img = Image.open(source, mode='r')
        img.save(target)

if __name__ == '__main__':
    if len(sys.argv) < 3:
        print('Not enough arguments!')
        exit(-1)
    pathslist = ret_args()
    create_dir(sys.argv[2])
    conv_imgs(pathslist[0], pathslist[1])

[neuraleskimo]

Ok, I changed my code to the following:

Code: Select all

...

This looks good. If you want to continue tweaking, I suspect I would end-up steering you toward something that looks like the dictionary solution (but probably a little different).
Here is some pseudo-code...

Code: Select all

get_root_paths():
    # if source doesn't exist, throw exception
    # if destination doesn't exist, create
    # return source, destination (note: you don't need to make them a list, a pair will do)

build_dst_filename(jpg_file, dst):
    # png_file = replace jpg with png on filename and append to dst
    # return png_file

convert_one_file(jpg_file, png_file):
    # open jpg_file
    # convert to png
    # save png

convert_all_files(src, dst):
    # for each jpg_file in src
    #    convert_one_file(jpg_file, build_dst_filename(src_file, dst))
 
main:
    # try
    #     convert_all_files(get_root_paths())
    # catch
    #     display nice message for user
    #     dump exception message and stack

This code above is starting to look a bit functional (intentionally). A next step might be to start looking for ways transform the code to use list comprehensions.

I hope this helps.

[PsySc0rpi0n]

Ok, I changed my code to the following:

Code: Select all

...

This looks good. If you want to continue tweaking, I suspect I would end-up steering you toward something that looks like the dictionary solution (but probably a little different).
Here is some pseudo-code...

Code: Select all

get_root_paths():
    # if source doesn't exist, throw exception
    # if destination doesn't exist, create
    # return source, destination (note: you don't need to make them a list, a pair will do)

build_dst_filename(jpg_file, dst):
    # png_file = replace jpg with png on filename and append to dst
    # return png_file

convert_one_file(jpg_file, png_file):
    # open jpg_file
    # convert to png
    # save png

convert_all_files(src, dst):
    # for each jpg_file in src
    #    convert_one_file(jpg_file, build_dst_filename(src_file, dst))
 
main:
    # try
    #     convert_all_files(get_root_paths())
    # catch
    #     display nice message for user
    #     dump exception message and stack

This code above is starting to look a bit functional (intentionally). A next step might be to start looking for ways transform the code to use list comprehensions.

I hope this helps.

Many thanks.
I want to try this whenever I have a bit more time.

At this moment I want to advance with the course I have enrolled with
I appreciate a lot your help and guidance.

Thanks
Psy