8
\$\begingroup\$

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()
\$\endgroup\$
3
\$\begingroup\$

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...

\$\endgroup\$
  • \$\begingroup\$ Can you verify I understand the Ternary suggestion? status_color = "Red" if has_packet_loss else "Green" works because has_packet_loss is an integer, and is checked. Because it's value is 0 the program goes to the else statement. Am I using the right terminology in my attempt at an explanation? \$\endgroup\$ – JohnSeuss Dec 23 '15 at 21:13
  • \$\begingroup\$ @JohnSeuss Yes, nice explanation. You may add == 0 for explicitness \$\endgroup\$ – Caridorc Dec 23 '15 at 21:15
  • \$\begingroup\$ I edited my original post with a new function status_ping. Is this what you meant with your Logic extraction suggestion? \$\endgroup\$ – JohnSeuss Dec 23 '15 at 21:37
2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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"
\$\endgroup\$
  • \$\begingroup\$ Are you sure you want just one function for packet loss and latency? If so you may delete the first line as color will be overwritten \$\endgroup\$ – Caridorc Dec 24 '15 at 8:00
  • \$\begingroup\$ Ah i see what you mean by the first line being overwritten, as the code says x <= 35 will turn green. So regardless of # of packet loss, it will always be green. I will change the 2nd condition in that function to if 4 < x < 35: color = "Green" and leave the first. My reason being that latency will never be below 4ms. And luckily the ping only checks for packet loss best out of 4 times. The script will no longer over-write, but could this solution be considered a poor way to go about things? Am I best just creating a separate function? \$\endgroup\$ – JohnSeuss Dec 24 '15 at 14:41
  • \$\begingroup\$ Yes, a separate function would make it much more obvious than this logic trick. Simple > Complex \$\endgroup\$ – Caridorc Dec 24 '15 at 15:46
1
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ You are right that it's an inconsistency, but I think maybe it should be \s* everywhere, to allow for both tabs and spaces and any number of them. Although \s allows line feeds as well, which should probably not be allowed. But for this case I think it is always using spaces so it's a good point that = \d* would be better. \$\endgroup\$ – Simon Forsberg Feb 21 '18 at 16:51
  • \$\begingroup\$ If i understand correcrly, this is better and works too? if "Minimum" in i: average_latency = int(re.search(r'\d+', str(re.findall(r'Average = \d*',i))).group()) \$\endgroup\$ – GinTora Feb 21 '18 at 18:01

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.