8
\$\begingroup\$

This command line program applies defined labels along with Monday's date to a directory of images.

The script has to handle not being run on a Monday. Others are asking for the script so I cleaned it up as much as I could. I want to make it as friendly and as cross-platform as possible.

Labels should be applied just above where the white border region ends. I couldn't guarantee that the border region would always be white, nor how big the border region could be.

Any tips on making it shorter, more elegant, more user-friendly or more resilient would be helpful. I'm considering moving the conditions and text of the actual labels to a separate text file for more flexibility.

import os
import datetime
from datetime import timedelta
import argparse
import glob
import PIL
from PIL import Image, ImageFont, ImageDraw

path_sep = os.path.sep

def get_date_string(optionalArg):
    #return date of most recent Monday
    # or optional date param given by user.
    if optionalArg is not None:
        return optionalArg
    else:
        today = datetime.datetime.today()
        if today.weekday() == 0:
            t = today
        else:
            t = today - timedelta(days=today.weekday())
        return t.strftime("%m/%d/%Y")

def significant_difference(v, previous_v):
    return abs(sum(v) - sum(previous_v)) > 200

def find_v_border(png):
    h = png.height
    png_ims = png.load()
    x = png.width/2
    last_v = png_ims[x,h-1]
    # walk up image from bottom, one pixel at a time.  when color changes, identify that as the place to write text:    
    for y in range(h-1,0,-1):
        v = png_ims[x,y]
        if significant_difference(v,last_v):
            return y    
        last_v = v

def find_left_border(png,bottom_border):
    png_ims = png.load()
    last_v = png_ims[1,bottom_border]
    for x in range(1,png.width/2):
        v = png_ims[x,bottom_border]
        # when big difference between v and last v, then return x
        if significant_difference(v,last_v):
            return x
        last_v = v


def draw_on_image(raw_img, new_img,date_text):
    global path_sep
    font_size = 16
    png = Image.open(raw_img)
    offset = font_size + 3
    bottom_border = find_v_border(png) - offset
    right_border = find_left_border(png,bottom_border) + 3
    fg="#000000"
    ttfpath = os.path.dirname(PIL.__file__)

    fnt = PIL.ImageFont.truetype(ttfpath + path_sep +'OpenSans-Bold.ttf',font_size)
    draw = PIL.ImageDraw.Draw(png)
    draw.text((right_border,bottom_border), date_text,font=fnt, fill=fg)
    del draw
    png.save(new_img, "PNG",quality=100)

# accept arguments for image file directory and optional date string
parser = argparse.ArgumentParser(description='Process given image.')
parser.add_argument('-p', help='path in which to find images to mark up...', required=True) 
parser.add_argument('--d', help='optional to add date you want displayed')
args = parser.parse_args()
image_path = args.p

#path references start from current directory
image_path = os.path.abspath(os.curdir) + path_sep + image_path

image_date = get_date_string(args.d)

    # read all the png files in image_path:

all_files = glob.glob(image_path + path_sep +'*.png')
for file_name in all_files:
    #parse file name to determine text ("Server, Desktop, or All")
    if "server" in str.lower(file_name):
        text = " Server Systems"
        new_file_name =  "labelled_server.png"
    elif "desktop" in str.lower(file_name):
        text = " Desktop Desktop"
        new_file_name = "labelled_Desktop.png"
    else:
        text = " All Systems"
        new_file_name = "labelled_all.png"

    img_text = image_date + text
    new_file_name = image_path + path_sep +new_file_name

    draw_on_image(os.path.join(image_path,file_name),new_file_name,img_text)

print("{0} files labelled with {1}".format(len(all_files),image_date))
\$\endgroup\$
4
\$\begingroup\$

Top level docstring

It is good practice to start a file with a string delimited by triple quotes that explains the general purpose of your file.

Python treats it specially and displays it when queried for help(your_module) and saves it in the __doc__ special variable.

Other people will get to know the general purpose of the code without reading it all.

A very basic start would be:

"""
This script adds time-stamps to images,
with particular regards to Mondays.
"""

Confusing parameter name

You write def get_date_string(optionalArg): where optionalArg is not actually optional... I am forced to provide it, an optional argument is one like def foo(a=1):

When names contrast the code I become very confused, also I do not think that kind of returning if not None is so useful, but I am not sure...

Actually you only call the function from:

image_date = get_date_string(args.d)

So you can remove the argument and the None check and call it like:

image_date = args.d or get_date_string()

Or, the equivalent:

image_date = args.d if args.d is not None else get_date_string()

Reduce nesting

else:
    today = datetime.datetime.today()
    if today.weekday() == 0:
        t = today
    else:
        t = today - timedelta(days=today.weekday())
    return t.strftime("%m/%d/%Y")

Becomes:

today = datetime.datetime.today()
if today.weekday() == 0:
    t = today
else:
    t = today - timedelta(days=today.weekday())
return t.strftime("%m/%d/%Y")

Because you already returned, and when you return the function ends, so you can reduce nesting and make it clearer that the None check is just a precondition.

Inline ternary

I like ternaries because they are less powerful than conditionals, being less powerful means that they are easier to read and understand too:

if today.weekday() == 0:
    t = today
else:
    t = today - timedelta(days=today.weekday())

Becomes:

t = today if today.weekday() == 0 else today - timedelta(days=today.weekday())

Monday is equal to zero, but you should not care

You write today.weekday() == 0 to check if weekday is Monday, but it is much more intuitive to read: today.weekday() == MONDAY

To allow this just add:

MONDAY = 0

at the top of your code, and you gained a lot of readability with a small effort.

Avoid single letter names

t tells me nothing about the purpose of this variable, try and think about a more descriptive name.

200 it was said...

def significant_difference(v, previous_v):
    return abs(sum(v) - sum(previous_v)) > 200

Why 200? This is very confusing, make a constant containing that value and give an explanatory name to it, because a random magic number 200 in the code just leaves me wondering...

Similar ...

The function find_v_border and find_left_border are similar, but sadly I cannot think of a way to abstract the similarity out... I just wanted to point this out as a possible further modularization possibility.

Not needed global

global is needed only when modifying global variables, so you may delete it from draw_on_image.

Re-use

Re-using singular functions is impossible now, as all the script will get executed if I import it, just putting the top-level code under a if __name__ == "__main__": will allow the re-use of single functions.

Function for deciding name and content

This block of code:

if "server" in str.lower(file_name):
    text = " Server Systems"
    new_file_name =  "labelled_server.png"
elif "desktop" in str.lower(file_name):
    text = " Desktop Desktop"
    new_file_name = "labelled_Desktop.png"
else:
    text = " All Systems"
    new_file_name = "labelled_all.png"

Only does one thing, so extracting it to a function looks like a good idea:

def find_text_and_new_name(file_name):
    if "server" in str.lower(file_name):
        return " Server Systems", "labelled_server.png"
    elif "desktop" in str.lower(file_name):
        return " Desktop Desktop", "labelled_Desktop.png"
    else:
        return " All Systems", "labelled_all.png"

Calling this from the top-level makes it more abstract, and you always want your top-level code to be as abstract as possible.

Increase generality

The draw_on_image function may easily be customized by the use of optional arguments, for example why should font always be 16? Making it an optional argument increases generality thus utility of your function.

(minor) Avoid unnecessary assignment

os.path.sep is never going to change, so you do not need to store it in another variable.

\$\endgroup\$
  • \$\begingroup\$ great stuff. I implemented all of these suggestions and now have a shorter, more readable and more maintainable script. Is there a way around importing both PIL and from PIL import Image etc... ? \$\endgroup\$ – mcgyver5 Dec 18 '15 at 22:35
  • \$\begingroup\$ @mcgyver5 you may do from PIL import x, y, z ... where x y and z are all the functions of the PIL module you used, but I do not suggest it. Also remember that (after waiting a day or two) improved code may be reposted for further improvents that the first review may have missed \$\endgroup\$ – Caridorc Dec 18 '15 at 22:45

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.