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

Question

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
#Office AP
ip_list = ['192.168.1.1']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((602, 693, 615, 705), fill='green')
 else:
 print('damn')
 draw.rectangle((602, 693, 615, 705), fill='red')
#N110 AP
ip_list = ['192.168.1.10']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((412,56,474,87), fill='green')
 else:
 print('damn')
 draw.rectangle((412,56,474,87), fill='red')
#N111
ip_list = ['google.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((541,54,619,86), fill='green')
 else:
 print('damn')
 draw.rectangle((541,54,619,86), fill='red')
#N112
ip_list = ['google.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((739,89,674,60), fill='green')
 else:
 print('damn')
 draw.rectangle((739,89,674,60), fill='red')
#N113
ip_list = ['10.30.119.27']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((844,86,785,60), fill='green')
 else:
 print('damn')
 draw.rectangle((844,86,785,60), fill='red')
#W108
ip_list = ['mys1pace.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((177,561,241,591), fill='green')
 else:
 print('damn')
 draw.rectangle((177,561,241,591), fill='red')
#W107
ip_list = ['knowledgeparrot.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((170,667,229,701), fill='green')
 else:
 print('damn')
 draw.rectangle((170,667,229,701), fill='red')
#W106
ip_list = ['bing.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((174,747,229,785), fill='green')
 else:
 print('damn')
 draw.rectangle((174,747,229,785), fill='red')
#W105
ip_list = ['yahoo.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((178,860,245,902), fill='green')
 else:
 print('damn')
 draw.rectangle((178,860,245,902), fill='red')
#Cafeteria
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((999,523,1064,559), fill='green')
 else:
 print('damn')
 draw.rectangle((999,523,1064,559), fill='red')
#Cafeteria 2
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1001,453,1063,481), fill='green')
 else:
 print('damn')
 draw.rectangle((1001,453,1063,481), fill='red')
#124
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((408,270,456,295), fill='green')
 else:
 print('damn')
 draw.rectangle((408,270,456,295), fill='red')
#122
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((403,361,441,383), fill='green')
 else:
 print('damn')
 draw.rectangle((403,361,441,383), fill='red')
#119
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((408,473,451,496), fill='green')
 else:
 print('damn')
 draw.rectangle((408,473,451,496), fill='red')
#117
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((291,557,338,582), fill='green')
 else:
 print('damn')
 draw.rectangle((291,557,338,582), fill='red')
#116
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((296,654,336,677), fill='green')
 else:
 print('damn')
 draw.rectangle((296,654,336,677), fill='red')
#114
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((410,741,460,768), fill='green')
 else:
 print('damn')
 draw.rectangle((410,741,460,768), fill='red')
#112
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((295,862,350,889), fill='green')
 else:
 print('damn')
 draw.rectangle((295,862,350,889), fill='red')
#126
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((567,181,619,204), fill='green')
 else:
 print('damn')
 draw.rectangle((567,181,619,204), fill='red')
#128
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((571,361,616,385), fill='green')
 else:
 print('damn')
 draw.rectangle((571,361,616,385), fill='red') 
#131
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((676,359,720,376), fill='green')
 else:
 print('damn')
 draw.rectangle((676,359,720,376), fill='red')
#134
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((796,182,840,200), fill='green')
 else:
 print('damn')
 draw.rectangle((796,182,840,200), fill='red') 
#135
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((796,270,839,289), fill='green')
 else:
 print('damn')
 draw.rectangle((796,270,839,289), fill='red')
#136
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((793,359,837,385), fill='green')
 else:
 print('damn')
 draw.rectangle((793,359,837,385), fill='red')
#Lab 1
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((792,578,836,603), fill='green')
 else:
 print('damn')
 draw.rectangle((792,578,836,603), fill='red') 
#Library
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((629,587,662,598), fill='green')
 else:
 print('damn')
 draw.rectangle((629,587,662,598), fill='red')
#104
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((664,860,703,882), fill='green')
 else:
 print('damn')
 draw.rectangle((664,860,703,882), fill='red')
#103
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((660,1009,709,1032), fill='green')
 else:
 print('damn')
 draw.rectangle((660,1009,709,1032), fill='red') 
#101
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((797,860,838,880), fill='green')
 else:
 print('damn')
 draw.rectangle((797,860,838,880), fill='red')
#102
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((804,1012,848,1032), fill='green')
 else:
 print('damn')
 draw.rectangle((804,1012,848,1032), fill='red')
#141
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((921,707,961,730), fill='green')
 else:
 print('damn')
 draw.rectangle((921,707,961,730), fill='red')
#143
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1096,709,1135,724), fill='green')
 else:
 print('damn')
 draw.rectangle((1096,709,1135,724), fill='red')
#Office Conf. Center
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1144,936,1169,953), fill='green')
 else:
 print('damn')
 draw.rectangle((1144,936,1169,953), fill='red')
#145/146 Office
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1317,359,1343,379), fill='green')
 else:
 print('damn')
 draw.rectangle((1317,359,1343,379), fill='red')
#147/148 Office
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1521,380,1555,401), fill='green')
 else:
 print('damn')
 draw.rectangle((1521,380,1555,401), fill='red')
#149
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1680,264,1735,287), fill='green')
 else:
 print('damn')
 draw.rectangle((1680,264,1735,287), fill='red')
#Pool
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1739,687,1776,714), fill='green')
 else:
 print('damn')
 draw.rectangle((1739,687,1776,714), fill='red')
#Choir 153
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1564,895,1601,925), fill='green')
 else:
 print('damn')
 draw.rectangle((1564,895,1601,925), fill='red')
#Stage
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1561,651,1616,685), fill='green')
 else:
 print('damn')
 draw.rectangle((1561,651,1616,685), fill='red')
#Gym
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((1325,648,1383,677), fill='green')
 else:
 print('damn')
 draw.rectangle((1325,648,1383,677), fill='red')
#110
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((415,1013,458,1041), fill='green')
 else:
 print('damn')
 draw.rectangle((415,1013,458,1041), fill='red')
#111
ip_list = ['espn.com']
for item in ip_list:
 status,result = subprocess.getstatusoutput("ping -c 1 -W 1 %s" % item)
 if status == 0:
 print('ok')
 draw.rectangle((416,863,460,881), fill='green')
 else:
 print('damn')
 draw.rectangle((416,863,460,881), fill='red')
 #End of Program - Save the file
 blank_map.save('/Users/user/Desktop/map_copy1.jpg')

• Graipher: Maybe use blank_map = Image.new("RGB", (1800, 1200), "white") instead of Image.open(...) and save to a file in the working directory with blank_map.save('map_copy1.jpg')
• MingyJ: @Graipher I updated my title, thanks for the pointer. I do not want to create a new image as the image I'm opening is the actual building map and I'm just drawing on that image. As a side note, I'm not sure I should upload the image I'm editing as its the floor plan for a public building and that could get dicey.
• Kaz: Please feel free to post all the repetitive code. We prefer to see as much of your code as possible.
• Graipher: It's not for you, it is for us, so we can actually run the code.
• Graipher: I totally agree on not showing the floor plan. This is why I recommended commenting out that line and putting in a way to generate a blank white map, so the code actually runs without the path pointing to an actual file.

Answer 1

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()

• MingyJ: That works so well! Thank you very much! I was mentally stuck thinking about how to keep the coordinates for each square paired with the IP address. This is wonderful ;)