patternpythonMinor
Automatic ping check program
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
Ternary
You got repetition in
Becomes:
Comments
You are commenting weird things:
Logic extraction
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...
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.