Automatic ping check program

Question

Am I commenting enough? Are my variables properly following correct styling format? Is there a more efficient way to code my "Label coloring" conditions?

from tkinter import *
import subprocess
import re

running = False # Infinite loop condition
idle_status = 'To begin, please press "Start"' # Message for Idle Status

# Start, Stop & Scanning functions
def start():
    Status.configure(text='Loading, please wait...')
    global running
    running = True

def stop():
    global running
    running = False
    Status.configure(text=idle_status, background="Grey")
    StatusPing.configure(text="", background="Grey")

def scanning():
    if running:
        output = subprocess.check_output("ping 104.160.131.1", shell = False, universal_newlines=True).splitlines()
        for i in output:
            if "Packets" in i: var1 = int(re.search(r'\d+', str(re.findall(r'Lost =\s\d*',i))).group())
            if "Minimum" in i: var2 = int(re.search(r'\d+', str(re.findall(r'Average =\s\d*',i))).group())
    Status.configure(text="Packet lost: {0}".format(var1))
    StatusPing.configure(text="Average ms: {0}".format(var2))

    # Packet loss label coloring
    if var1 == 0:
        Status.configure(background="Green")
    else:
        Status.configure(background="Red")

    # Ping Status label coloring
    if var2 <= 35:
        StatusPing.configure(background="Green")
    if 35 < var2 < 70:
        StatusPing.configure(background="Yellow")
    if var2 >= 70:
        StatusPing.configure(background="Red")
    root.after(10000, scanning)

# GUI
root = Tk()
root.geometry("200x120")
root.wm_title("Ping Checker")

# Ping Check Label
Status = Label(root, text = idle_status, height="0", width="30", background="Grey")
Status.pack(pady=1) # For visible division between two labels
StatusPing = Label(root, height="0", width="30", background="Grey")
StatusPing.pack()

# Start & Stop Buttons
Start = Button (root, text = "Turn on", command = start).pack()
Stop = Button (root, text = "Turn off", command = stop).pack()

root.after(10000, scanning) # Global Loop
root.mainloop()

Answer 1

Naming

The elephant in the room: names like var1 and var2 really hinder the readibility. var1 probably means has_packet_loss, var2 -> latency. Naming is hard, but it also is a fundamental property of readable code.

Ternary

You got repetition in

if var1 == 0: # has_packet_loss
    Status.configure(background="Green")
else:
    Status.configure(background="Red")

Becomes:

  status_color = "Red" if has_packet_loss else "Green"
  Status.configure(background=status_color)

Comments

You are commenting weird things: # Start & End Button is a thing I clearly see from the code, but the general purpose of the script is not immediately obvious. I suggest avoiding line-by-line comments and instead go for a docstring explaining the general purpose.

Logic extraction

StatusPing.configure is repeated 3 times:

if var2 <= 35:
    StatusPing.configure(background="Green")
if 35 < var2 < 70:
    StatusPing.configure(background="Yellow")
if var2 >= 70:
    StatusPing.configure(background="Red")

Write a function to determine background color and use it to avoid this repetition (Or a list of tuples)

Named constants

Define parameters at the start and use them later, this makes the code easier to re-use and makes it funnier to tinker (pun intended) with it. For example the ID to ping, the size of the widget, the colours, the messages...

Answer 2

Note: Bear in mind that you are relying on the OS's ability to correctly calculate packet loss. (Please see: https://networkengineering.stackexchange.com/questions/30940/difference-between-ping-timed-out-and-destination-host-unreachable)

You may wish to check for such phrases as 'Destination host unreachable' in your output as Windows does not count those responses as lost packets.

To bullet-proof your code, you might want to call the subprocess.check_output method from within a try...except. Losing your network connection during the call can result in an unexpected error which will prevent your scanning function from reaching its last line and looping.

Answer 3

Solutions to Ternary and Logic Extraction @Caridorc suggestions

def scanning():
    if running:
        output = subprocess.check_output("ping 104.160.131.1", shell = False, universal_newlines=True).splitlines()
        for i in output:
            if "Packets" in i: packet_loss = int(re.search(r'\d+', str(re.findall(r'Lost =\s\d*',i))).group())
            if "Minimum" in i: average_latency = int(re.search(r'\d+', str(re.findall(r'Average =\s\d*',i))).group())

    Status.configure(text="Currently scanning every 10s...", background="Grey")

    StatusPacketLoss.configure(text="Packet(s) lost: {0}".format(packet_loss))
    StatusPacketLoss.configure(background=status_color(packet_loss))
    StatusLatency.configure(text="Average ms: {0}".format(average_latency))
    StatusLatency.configure(background=status_color(average_latency)) # Function for simplicity

    root.after(10000, scanning)

def status_color(x):
    # Determine Packet Loss Color
    color = "Green" if x == 0 else "Red"

    # Determine Average Latency Color
    if x <= 35: color = "Green"
    if 35 < x < 70: color= "Yellow"
    if x >= 70: color = "Red"

    return color

If anyone has any suggestions to remove redundancy or any excess, feel free to respond.

new status_color function:

def status_color(x):
    # Determine Packet Loss Color, best out of 4 pings
    if x == 0:
        color = "Green"
    elif x <= 4:
        color = "Red"
    # Determine Average Latency Color
    # Ping will never be low er than 4ms
    elif x <= 35:
        color = "Green"
    elif x <= 70:
        color= "Yellow"
    else:
        color = "Red"

Answer 4

I got a question about the code, why use \s and not just enter a space and use \d*,?

What I mean is,

(r'Average = \d*',i)

Instead of

(r'Average =\s\d*,i)

Why use \s? If we did not use it before the = sign?