21
\$\begingroup\$

I had a task to convert some JPG images into PNG format using Python. I successfully did it but I was wondering if somehow I could make my code better than my current one.

Code -

#Python Code

import os  
from PIL import Image  

path1 = sys.argv[1]  
path2 = sys.argv[2]  

if not os.path.exists(path2):
    os.makedirs(path2)  

for filename in os.listdir(path1):
    if filename.endswith(".jpg"):
        # print(filename.split(".")[0])
        im = Image.open(f"{path1}{filename}")
        # print(Image.open(f"{path1}{filename}"))
        im.save(f"{path2}/{filename.split('.')[0]}.png", "png")
        print("Lets go")
\$\endgroup\$
  • 17
    \$\begingroup\$ Welcome to Code Review. I rolled back your last edit.Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Feb 12 at 5:01
  • 1
    \$\begingroup\$ Depending on your use-case and intended user-base, consider whether someone might try either python test.py foo/img.jpg bar/img.png, or python test.py in/ out/ where in/foo.jpeg exists. \$\endgroup\$ – Quuxplusone Feb 12 at 14:34
  • 10
    \$\begingroup\$ On a slight tangent, it's probably worth noting that converting JPEG images to PNG format without cleaning them up to remove compression artifacts is almost never actually useful. It'll just make the file (possibly many times) bigger without improving the quality of the image in any way. (Of course, there can be situational exceptions, like if you're trying to feed the images to some program that can parse PNG but not JPEG.) \$\endgroup\$ – Ilmari Karonen Feb 13 at 0:10
  • 1
    \$\begingroup\$ Do you really want to print("Lets go") once for each file? \$\endgroup\$ – Mawg says reinstate Monica Feb 13 at 12:55
  • 6
    \$\begingroup\$ I'd go even further than Ilmari here, and say that if I were given this task, I would feel compelled to tell my employer that if you think you need a tool to convert JPEG to PNG, there's almost certainly something wrong with your process as a whole. You're basically pouring water from a 1-gallon bucket into a 5-gallon bucket; you'll still only have 1 gallon. Find out what part of your process lost the other 4, and elimintate that. \$\endgroup\$ – Lee Daniel Crocker Feb 13 at 18:19
37
\$\begingroup\$

Not much to improve upon, just some minor housekeeping.


Help the end user!

If I was using this script, and forgot to put one or both of the filepaths when calling the script, there would be this error: IndexError: list index out of range. This error wouldn't help the user in a significant way. Consider this:

try:
    path1 = sys.argv[1]  
    path2 = sys.argv[2]
except IndexError:
    raise Exception("Usage: python3 jpg_to_png.py <path to input folder> <path to output folder>")

This gives the user a detailed message on how the use your script.

Remove commented code.

Code you don't need anymore adds useless clutter, and can make your code harder to read.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you Linny, I will improve the code by using your suggestion and thanks for the suggestion regarding the commented out code. \$\endgroup\$ – Praguru 14 Feb 12 at 4:10
  • 2
    \$\begingroup\$ @Praguru After someone answers your question, you're not supposed to edit your code. Can you please rollback your edit to make their answrr valid agian? \$\endgroup\$ – S.S. Anne Feb 12 at 4:27
  • 5
    \$\begingroup\$ Even better is to just use the argparse library. \$\endgroup\$ – AleksandrH Feb 12 at 14:36
  • 1
    \$\begingroup\$ argparse every time Upvote \$\endgroup\$ – Mawg says reinstate Monica Feb 13 at 12:53
  • 1
    \$\begingroup\$ While argparse is handy, it's rather painful to use, and for small projects that do one single task, it just causes a hindrance imo. \$\endgroup\$ – Ave Feb 14 at 13:21
30
\$\begingroup\$

The variable names path1 and path2 are bad since they are not as descriptive as possible. They should rather be srcdir and dstdir (if you prefer abbreviations), or source_directory and destination_directory if you want to have them spelled out.

Instead of manipulating strings, it's better to use the pathlib library, which has the handy function with_suffix.

You should test what happens with filenames that have multiple dots, to prevent any surprise about losing parts of the filename.

|improve this answer|||||
\$\endgroup\$
15
\$\begingroup\$

Apart from what's already mentioned I would like to point out that the file super.picture.jpg will be converted to super.png

That can be a problem if someone runs your program in a loop and iterates through a folder with files named anniversary.1.jpg anniversary.2.jpg....

Instead, because you have used endswith('.jpg') you can just use a substring of length filename.length - 4.

|improve this answer|||||
\$\endgroup\$
  • 6
    \$\begingroup\$ This wouldn't work for .jpeg. Instead of using substrings, consider using os.path.splitext. It correctly accounts for this use case, and even more general ones. OP could extend this script to convert all images of a specified format to another format. Alternatively, just search for the first period from the end of the string and split there. But it's better to use an existing library. \$\endgroup\$ – AleksandrH Feb 12 at 16:53
10
\$\begingroup\$

Consider using __main__ to make your script easier to import (see here ).

Also, consider the use of functions to isolate the different steps, so that it is easier to add new functionalities in the future.

One last thing, you could use a more explicit log message, something along the lines of Converting {filename} from jpg to png instead of Lets go, maybe using the logging module instead of a simple print.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I doubt that the intent is to import this script in other Python source files. This sounds more like a standalone script. \$\endgroup\$ – AleksandrH Feb 12 at 14:37
  • 2
    \$\begingroup\$ @AleksandrH agree, yet preparing it for a future unforeseen use-case is not harmful if not overdone, like in this case. Also, it is always a good exercise to get better at modularity and architectures. \$\endgroup\$ – bracco23 Feb 12 at 14:40
  • \$\begingroup\$ Yeah, that's fair. It wouldn't involve much effort to just wrap everything in the __main__ check. \$\endgroup\$ – AleksandrH Feb 12 at 16:56
5
\$\begingroup\$

function

I would abstract this into a function you can call. Then you can also easily incorporate a few checks to see whether the source path really is a directory, that the destination path actually exists,...

pathlib.Path

Has a lot of convenience methods that help with file path handling.

from PIL import Image
from pathlib import Path
def convert_jpg_to_png(
    source: Path, 
    destination: Path, 
    overwrite=False,
) -> None:
    if not source.is_dir():
        raise ValueError(f"{source} is not a directory")
    if not source.exists():
        raise ValueError(f"{source} does not exist")
    if not destination.is_dir():
        raise ValueError(f"{destination} is not a directory")
    if not destination.exists():
        destiny.mkdir(parents=True)

    for source_name in source.glob("**/*.jpg"):
#     If you want to cover both jpg and jpeg, 
#     for source_image in source.glob("**/*"):
#         if not source_image.suffix.lower in {".jpg", ".jpeg"}:
#             continue
        destination_name = destination / (
            source_name.relative_to(source.with_suffix(".png"))
        )
        if destination_name.exists() and not overwrite:
            print(f"{destination_name} already exists")
            continue
        print(f"renaming {source_name} to {destination_name}")
        with Image.open(source_name) as im:
            im.save(destination_name, format="PNG")

The commented bits are if you also want to transform .JPG, .JPEG and .jpeg

main guard

You can then hide your logic behind a if __name__ == "__main__": guard, so you can later import this module in another program if you want to:

if __name__ == "__main__":
    if len(sys.argv) == 1:
        source = Path(".") # default: current location
        destination = source
        overwrite = False
    source = Path(sys.argv[1])
    if len(sys.argv) > 2:
        if sys.argv[2] in {"True", "False"}:
            destination = source
            overwrite = sys.argv[2] == "True"
        else:
            destination = Path(sys.argv[2])
    else:
        destination = source
        overwrite = False
    if len(sys.argv) > 3:
        overwrite = sys.argv[3] == "True"
    convert_jpg_to_png(source, destination, overwrite)

Here I manually handled the arguments, but you can better check the argparse module to simplify this

|improve this answer|||||
\$\endgroup\$
4
\$\begingroup\$

In general the code is simple and concise.

Depending on the target usage, apart from what other answers suggest, I would add:

  • support for files ending on ". jpeg" not only .jpg;
  • likewise I would make filename filter also find uppercase or mixed case JPEG files;
  • if I had to deal with unreliable sources I would add also Image.verify() and/or imghdr checks to see if I get a valid jpeg file, and to give meaningful error messages otherwise.
|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

A couple of things that I would suggest that you do:

  • A level of nesting can be removed by using a generator expression to filter the list of files:

    files_to_convert = (f for f in os.listdir(path1) if f.endswith(".jpg"))
    for filename in files_to_convert:
        ... process the file ...
    
  • Ensure that the listing of *.jpg are files, not subdirectories named *.jpg:

    files_to_convert = (f for f in os.listdir(path1) if f.endswith(".jpg") and os.path.isfile(f))
    
  • Use os.path.join to construct paths:

    im = Image.open(os.path.join(path1, filename))
    
  • Use os.path.splitext to split the filename:

    root, _ = os.path.splitext(filename)
    png_file = os.path.join(path2, f'{root}.png')
    im.save(png_file, "png")
    
|improve this answer|||||
\$\endgroup\$

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.