HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Automatic ping check program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
automaticcheckprogramping

Problem

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

Solution

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

Code Snippets

if var1 == 0: # has_packet_loss
    Status.configure(background="Green")
else:
    Status.configure(background="Red")
status_color = "Red" if has_packet_loss else "Green"
  Status.configure(background=status_color)
if var2 <= 35:
    StatusPing.configure(background="Green")
if 35 < var2 < 70:
    StatusPing.configure(background="Yellow")
if var2 >= 70:
    StatusPing.configure(background="Red")

Context

StackExchange Code Review Q#114903, answer score: 3

Revisions (0)

No revisions yet.