5
\$\begingroup\$

I have a program that allows me to check (in a nicer way than using command prompt) what my ping will be when playing League Of Legends. I've implemented all of the elements that I had hoped, however now I'm simply looking for ways that my code can be improved.

I do have some gripes with my current code that I'm not completely satisfied with... including;

  • Having to use "// = "information" in order to display tips for the settings file, I'd much rather just have some form of comment system such as # as in python
  • I'm unsure whether the way I'm checking for errors in the settings file is as effective as it can be.
  • I'm also aware that the use of place is not exactly ideal. However I like the way that my program is currently layed out to the exact pixels.
import subprocess
import re
import os
from tkinter import *

master = Tk()
master.config(bg="#2b2f35")
master.resizable(False, False)
master.geometry('500x305+50+50')
master.title("Ping Check")

topFrame = Label(text="Ping Checker", bg="#454c56", fg="#fff", width=43, height=3, font="Bahnschrift 14")
topFrame.place(x=11,y=12)

displayPing = Label(text="", bg="#2ecc71", font="Bahnscrift 10", width=18, height=6, fg="#fff")
displayPing.place(x=11, y=93)

infoFrame = Label(text="Lorem Ipsum", bg="#454c56", fg="#fff", width=52, height=7, font="Bahnschrift 8", wraplength=300)
infoFrame.place(x=172, y=93)

moreInfoFrame = Label(text="Lorem Ipsum", bg="#454c56", fg="#fff", width=52, height=7, font="Bahnschrift 8", wraplength=300)
moreInfoFrame.place(x=172, y=194)

informError = Label(text="", fg="red", bg="#2b2f35", width=23, height=3,font="Bahnschrift 8")
informError.place(x=11, y=242)

def changeSettingsO():
    os.startfile('settings.txt')

changeSettings = Button(text="Change Settings", bg="#3498db", font="Bahnscrift 10", width=18, height=2, fg="#fff", relief=FLAT, borderwidth=0, command=changeSettingsO)
changeSettings.place(x=11, y=198)

try:
    with open("settings.txt", "r") as f:
        for line in f:
            key,value = line.strip().split(" = ")
            value = value.strip('"')

            if key == 'region':
                region = value

            if key == 'pingSpeed':
                pingSpeed = int(value)

except IOError:
    file = open("settings.txt","w+")
    file.write('// = "If you edit anything below, please save the file and restart the program for changes to apply"')
    file.write('// = "The region setting should be set to the region that you play in. Valid options -  (EUW)(NA)"')
    file.write('// = "The pingSpeed setting dictates how often to check your current ping. Valid options - 1-500"')

    file.write('\nregion = "default"')
    file.write('\npingSpeed = "default"')

    pingIP = "104.160.141.3"
    region = "default"
    file.close()

errorCheck = 0

if region == "EUW": pingIP = "104.160.141.3"
elif region == "NA": pingIP = "104.160.131.3"

else:
    informError.config(text="Error in settings - check\n your region value\n (should be EUW or NA)")
    region = "default"
    pingIP = "104.160.141.3"
    errorCheck =+ 1

if 1 <= pingSpeed <= 500: pingSpeedInt = pingSpeed * 1000

else:
    informError.config(text="Error in settings - check\n your PingSpeed value\n (should be within 1-500)")
    pingSpeed = 1
    errorCheck = errorCheck + 1
    pingSpeedInt = pingSpeed * 1000

def pingDisplay():
    displayPing.config(bg="#2ecc71")
    master.after(10, checkPing)

def checkPing():
    global region
    global pingSpeed
    global pingSpeedInt

    if errorCheck == 0:
        informError.config(text="No errors detected\n looks like the progrm\nis running correctly")

    if errorCheck == 2:
        informError.config(text="Critical Error - It appears \nboth settings in the file have \nerrors, please check!")

    try:
        pattern = r"Average = (\d+\S+)"
        ping = subprocess.Popen(["ping", pingIP, "-n", "1"], stdout = subprocess.PIPE,stderr = subprocess.PIPE, shell=True)
        fullOutput = ping.communicate()
        scrubbedOutput = re.findall(pattern, fullOutput[0].decode())[0] + " (" + region + ")" + "\n" + "check every: "  + str(pingSpeed) + "s"
        displayPing.config(text=scrubbedOutput, bg="#27ae60")
        master.after(pingSpeedInt, pingDisplay)

    except:
        print("Error")

master.after(10, checkPing)
master.mainloop()
| improve this question | | | | |
\$\endgroup\$
1
\$\begingroup\$

Regarding the settings file I would suggest using configparser. This saves some hassles with parsing the settings file as the library will do it for you and will make the error handling much easier. This will get you your comments syntax supported for free.

Additionally with Tkinter you may wish to use a geometry manager such as pack or grid to get UI that will scale with the window size.

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ Hey. Thanks for the reply. I guess I just like the simplicity of my current setup for settings.. I don't really see the point of using a whole module to do something I've already got working, but maybe that's just me.. In terms of using grid or pack, I really should. I'm planning to change it when I get time.. and patience. Although right now it's not a huge deal as the window is set to not be resizable anyway. \$\endgroup\$ – Chris Feb 14 '18 at 10:17
  • \$\begingroup\$ The main reason to use libraries like configparser from the standard library is that it will let you get code in place faster, I'm assuming you have tests for your configuration parser and you know that it works well. These things take time to put into place, so cutting back on that time can be very beneficial. \$\endgroup\$ – shuttle87 Feb 15 '18 at 14:37
1
\$\begingroup\$

Couple of quick things, be consistent with file opening and use the context manager always as this handles exceptions for you:

So instead of:

except IOError:
    file = open("settings.txt","w+")
    file.write('// = "If you edit anything below, please save the file and restart the program for changes to apply"')
    file.write('// = "The region setting should be set to the region that you play in. Valid options -  (EUW)(NA)"')
    file.write('// = "The pingSpeed setting dictates how often to check your current ping. Valid options - 1-500"')

    file.write('\nregion = "default"')
    file.write('\npingSpeed = "default"')

    pingIP = "104.160.141.3"
    region = "default"
    file.close()

prefer:

except IOError:
    with open("settings.txt","w+") as f:
        f.write('// = "If you edit anything below, please save the file and restart the program for changes to apply"')
        f.write('// = "The region setting should be set to the region that you play in. Valid options -  (EUW)(NA)"')
        f.write('// = "The pingSpeed setting dictates how often to check your current ping. Valid options - 1-500"')

        f.write('\nregion = "default"')
        f.write('\npingSpeed = "default"')

    pingIP = "104.160.141.3"
    region = "default"

(Note also that I have change the name here from file to f this is because file is actually a keyword corresponding to a type in python 2.x (but not in 3.x) so if this was to run on Python 2.x you just shadowed this variable, which is something you should avoid. This is more a habit from Python 2.x days though, if you don't plan to support 2.x then this doesn't apply and using file would actually be a preferable name.)

Also set defaults in one place and one place only, currently pingIP and region are set twice when the default case needs to be handled:

    pingIP = "104.160.141.3"
    region = "default"
    file.close()

errorCheck = 0

if region == "EUW": pingIP = "104.160.141.3"
elif region == "NA": pingIP = "104.160.131.3"

else:
    informError.config(text="Error in settings - check\n your region value\n (should be EUW or NA)")
    region = "default"
    pingIP = "104.160.141.3"
    errorCheck =+ 1

It's much better to just do this once and set it only once in the else case. This is a fairly easy bit of duplicated code to remove.

Try to catch as narrow an exception as possible and only put the exception handling around that part of the code, currently you have this:

try:
    pattern = r"Average = (\d+\S+)"
    ping = subprocess.Popen(["ping", pingIP, "-n", "1"], stdout = subprocess.PIPE,stderr = subprocess.PIPE, shell=True)
    fullOutput = ping.communicate()
    scrubbedOutput = re.findall(pattern, fullOutput[0].decode())[0] + " (" + region + ")" + "\n" + "check every: "  + str(pingSpeed) + "s"
    displayPing.config(text=scrubbedOutput, bg="#27ae60")
    master.after(pingSpeedInt, pingDisplay)

except:
    print("Error")

instead I'd do something like this:

pattern = r"Average = (\d+\S+)"
try:
    ping = subprocess.Popen(["ping", pingIP, "-n", "1"], stdout = subprocess.PIPE,stderr = subprocess.PIPE, shell=True)
    fullOutput = ping.communicate()
except subprocess.SubprocessError:
    print("Error with subprocess")
else:
    scrubbedOutput = re.findall(pattern, fullOutput[0].decode())[0] + " (" + region + ")" + "\n" + "check every: "  + str(pingSpeed) + "s"
    displayPing.config(text=scrubbedOutput, bg="#27ae60")
    master.after(pingSpeedInt, pingDisplay)
| improve this answer | | | | |
\$\endgroup\$

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.