patternpythonMinor

Draw Image using PIL and pillow - without repeating blocks of code

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

withoutimageblocksdrawrepeatingpilpillowcodeusingand

Problem

I have a Python script that takes a map of a building and then draws green or red squares on the map corresponding to the physical location of building access points, the colors represent wether or not the device responds to ping. My code currently works, however as you'll notice there is a TON of repetition, which looks bad and I don't think is best practice. I'm wondering if you guys can get me off on the right foot here, any suggestions are welcome I'm still pretty new so even simple tips are awesome!

The "IP's" are mostly just websites like google at this point in time instead of the actual devices I will eventually use. The process shown continues to repeat the same ping, if draw, else draw process about 10 more times I just wanted to show the idea. Image I'm importing is 2100x1536

```
#Opens and Manipulates an image
#Followed the basic image editting ideas here https://automatetheboringstuff.com/chapter17/

#Imports
from PIL import Image, ImageDraw, ImageFont
import subprocess
import os

#Opens Image from dir defined and gives the dim. of the image
blank_map = Image.open('/Users/user/Google Drive/Python Programs/Map/map_copy.jpg')
print('This image is', blank_map.size, 'pixels big!')

#Creates the KEY
draw = ImageDraw.Draw(blank_map)
draw.rectangle((914,30,949,56), fill='green')
draw.rectangle((914,58,949,84), fill='red')
arialFont = ImageFont.truetype('/Library/Fonts/Arial.ttf', 18)
georgiaFont = ImageFont.truetype('/Library/Fonts/georgia.ttf', 36)
draw.text((960,33), '= Access Point Online', fill = 'black', font=arialFont)
draw.text((960,61), '= Access Point Offline', fill = 'black', font=arialFont)
draw.text((900,1112), 'Building Map', fill = 'red', font=georgiaFont)
draw.text((1733,597), 'Pool', fill = 'black', font=arialFont)

#Covers un-wanted text and writes again
draw.rectangle((983,607,1087,631), fill='white')
draw.text((1010,607), 'MPR', fill = 'black', font=arialFont)

#Pings an IP Address - use ping -n for windows and ping -c for Unix

#Offic

Solution

When I started the review, the first thing I did was putting the functionality of the ping and draw into a function. This works, as long as draw is global (or a parameter of that function), which are both not nice. This function looked like this:

def draw_status(draw, ip_list, position):
    for ip in ip_list:
        status, result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % ip)
        if status == 0:
            print('ok')
            draw.rectangle(position, fill='green')
            return
    print('damn')
    draw.rectangle(position, fill='red')

This can be used, like so:

draw_status(draw, ['google.com'], (541,54,619,86))

which already gets rid of a lot of repetition. Note that this will try all ips in the ip list, until one responds (after which it will return) or none responded and then a red rectangle will be drawn. This way you avoid stacking many red rectangles on top of each other.

I then decided to make this a class that handles everything, instead.
For this I first defined a collections.namedtuple Server that has two fields, the list of ips for that server and its rectangle on the map.

I made the fonts global constants and named them, according to PEP8, with ALL_CAPS.

The class itself, UpMap, takes a list of Servers and (optionally) a filepath as base map. If no filepath is given it creates blank white map.

I added a main function to hold the execution of this whole module and a if __name__ == '__main__': guard to execute the main function only if you are not importing this module from some other script.

from PIL import Image, ImageDraw, ImageFont
import subprocess
import os
from collections import namedtuple

Server = namedtuple("Server", "ip_list position")

ARIAL = ImageFont.truetype('/Library/Fonts/Arial.ttf', 18)
GEORGIA = ImageFont.truetype('/Library/Fonts/georgia.ttf', 36)

class UpMap:
    def __init__(self, servers, blank_map=None):
        self.servers = servers
        if blank_map is None:
            self.blank_map = Image.new("RGB", (2100, 1536), "white")
        else:
            self.blank_map = Image.open(blank_map)
        self.draw = ImageDraw.Draw(self.blank_map)
        self.draw_info()

    def draw_info(self):
        self.draw.rectangle((914,30,949,56), fill='green')
        self.draw.rectangle((914,58,949,84), fill='red')
        self.draw.text((960,33), '= Access Point Online', fill='black', font=ARIAL)
        self.draw.text((960,61), '= Access Point Offline', fill='black', font=ARIAL)
        self.draw.text((900,1112), 'Building Map', fill='red', font=GEORGIA)
        self.draw.text((1733,597), 'Pool', fill='black', font=ARIAL)

        #Covers un-wanted text and writes again
        self.draw.rectangle((983,607,1087,631), fill='white')
        self.draw.text((1010,607), 'MPR', fill='black', font=ARIAL)

    def draw_status(self):
        for server in self.servers:
            for ip in server.ip_list:
                status, result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % ip)
                if status == 0:
                    print('ok')
                    self.draw.rectangle(server.position, fill='green')
                    return
            print('damn')
            self.draw.rectangle(server.position, fill='red')

    def save(self, path='map_copy1.jpg'):
        self.blank_map.save(path)

def main():
    servers = [Server(['192.168.1.1'], (602, 693, 615, 705)),
               Server(['192.168.1.10'], (412,56,474,87)),
               Server(['google.com'], (541,54,619,86)),
               ...]

    server_map = UpMap(servers)
    server_map.draw_status()
    server_map.save()

if __name__ == '__main__':
    main()

Code Snippets

def draw_status(draw, ip_list, position):
    for ip in ip_list:
        status, result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % ip)
        if status == 0:
            print('ok')
            draw.rectangle(position, fill='green')
            return
    print('damn')
    draw.rectangle(position, fill='red')

draw_status(draw, ['google.com'], (541,54,619,86))

from PIL import Image, ImageDraw, ImageFont
import subprocess
import os
from collections import namedtuple

Server = namedtuple("Server", "ip_list position")

ARIAL = ImageFont.truetype('/Library/Fonts/Arial.ttf', 18)
GEORGIA = ImageFont.truetype('/Library/Fonts/georgia.ttf', 36)

class UpMap:
    def __init__(self, servers, blank_map=None):
        self.servers = servers
        if blank_map is None:
            self.blank_map = Image.new("RGB", (2100, 1536), "white")
        else:
            self.blank_map = Image.open(blank_map)
        self.draw = ImageDraw.Draw(self.blank_map)
        self.draw_info()

    def draw_info(self):
        self.draw.rectangle((914,30,949,56), fill='green')
        self.draw.rectangle((914,58,949,84), fill='red')
        self.draw.text((960,33), '= Access Point Online', fill='black', font=ARIAL)
        self.draw.text((960,61), '= Access Point Offline', fill='black', font=ARIAL)
        self.draw.text((900,1112), 'Building Map', fill='red', font=GEORGIA)
        self.draw.text((1733,597), 'Pool', fill='black', font=ARIAL)

        #Covers un-wanted text and writes again
        self.draw.rectangle((983,607,1087,631), fill='white')
        self.draw.text((1010,607), 'MPR', fill='black', font=ARIAL)

    def draw_status(self):
        for server in self.servers:
            for ip in server.ip_list:
                status, result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % ip)
                if status == 0:
                    print('ok')
                    self.draw.rectangle(server.position, fill='green')
                    return
            print('damn')
            self.draw.rectangle(server.position, fill='red')

    def save(self, path='map_copy1.jpg'):
        self.blank_map.save(path)


def main():
    servers = [Server(['192.168.1.1'], (602, 693, 615, 705)),
               Server(['192.168.1.10'], (412,56,474,87)),
               Server(['google.com'], (541,54,619,86)),
               ...]

    server_map = UpMap(servers)
    server_map.draw_status()
    server_map.save()

if __name__ == '__main__':
    main()

Context

StackExchange Code Review Q#154920, answer score: 3

Revisions (0)

No revisions yet.