6
\$\begingroup\$

I'm making a little Python script that reads a text file and puts it on a HTML page. I haven't finished that part yet, but I've been working on the GUI with tkinter. Is my code sloppy?

from Tkinter import *
import time
from datetime import datetime
#init variables
global r
r = 0
global delay
delay = 1000
#for testing
global lat
lat = "41.5345"
global lon
lon = "-85.343"
global zoom
zoom = 0
global mapDir
mapDir = ""
global mapDirEnabl
mapDirEnabl = 0
#clear log file
log = open("xpyMapLog.txt", "w")
log.write("")
log.close()
global log
log = open("xpyMapLog.txt", "a")
logStartMessage = "RUN TIME EXECUTED AT: %s \n" % str(datetime.now())
log.write(logStartMessage) 

#error check
class safe:
    def __init__(self, function):
        self.function = function

    def __call__(self, *args):
        try:
            return self.function(*args)
        except Exception, e:
            errorMsg = "Error: %r" % str(e)
            log.write(errorMsg)  
            print "Error: %s" % e
        #
    #
#

#open data file
@safe
def openData():
    global Data
    Data = open("Data.txt", "r")
#
openData()


#init root window
root = Tk()
root.geometry("550x300+200+200")
root.title("XPYmap for x-plane 10")


#init functions         

@safe
def mainLoop():
    if r == 1:
        #**************************************
        #DATA READING AND WRITING TO MAP FILE!*
        #**************************************
        print "Hello"       
        #im not done with the data reading yet so just pretend its here

        #######################################
    #       
#


@safe
def start():
    global r
    r = 1
    mainLoop()
#
@safe
def stop():
    global r
    r = 0
    posLabl["text"] = "Position: NOT RUNNING"
#
@safe
def setZoom(z):
    global zoomlabel
    zoom = int(z)
    curntZoomLabl["text"] = "Current zoom level: %s" % zoom
#
@safe
def setDelay(d):
    print "delay"
    global delay    
    delay = int(d)
    delayInSec = delay / 1000 # get seconds from milisec
    curntDelayLabl["text"] = "Current update delay: %s seconds." % delayInSec
#

@safe
def chooseMapDir(): 
    global mapDirEnabl
    global mapDir
    if mapDirEnabl == 0:
        mapDirEnabl = 1
        mapDirStat["text"] = "Custom map directory is currently: ENABLED!"
        mapDir = str(mapDirEntry.get())
        print mapDir
    elif mapDirEnabl == 1:
        mapDirEnabl = 0
        mapDirStat["text"] ="Custom map directory is currently: DISABLED!" 
        mapDir = ""
        print mapDir
    #

#


#widgets

posLabl = Label(root, text = "Position: NOT RUNNING")
butStart = Button(root, text = "Start", command = start)
butStop = Button(root, text = "Stop", command = stop)
zoomLabl = Label(root, text = "Click the zoom level")
curntZoomLabl = Label(root, text = "Current zoom level:")
delayLabl = Label(root, text = "Select update delay")
curntDelayLabl = Label(root, text = "Current update delay: NONE")
mapDirLabl = Label(root, text = "ADVANCED ONLY! If you want to save the map file to another directory, enter it in below.")
mapDirBut = Button(root, text = "enable/disable directory", command = chooseMapDir)
mapDirStat = Label(root, text = "Custom map directory is currently: DISABLED!")
mapDirEntry = Entry(root, width = 60)

#column span for buttons
colspan = 1

#zoom with grid

bz1 = Button(root, text = "1", width=2, command = lambda:setZoom("1"))  
bz1.grid(row = 5, column = 0, columnspan = colspan)       
bz2 = Button(root, text = "2", width=2, command = lambda:setZoom("2"))   
bz2.grid(row = 5, column = 1, columnspan = colspan  ) 
bz3 = Button(root, text = "3", width=2,  command = lambda:setZoom("3"))   
bz3.grid(row = 5, column = 2, columnspan = colspan  ) 
bz4 = Button(root, text = "4", width=2,  command = lambda:setZoom("4"))   
bz4.grid(row = 5, column = 3, columnspan = colspan  ) 
bz5 = Button(root, text = "5", width=2,  command = lambda:setZoom("5"))   
bz5.grid(row = 5, column = 4, columnspan = colspan  ) 
bz6 = Button(root, text = "6", width=2,  command = lambda:setZoom("6"))   
bz6.grid(row = 5, column = 5, columnspan = colspan  ) 
bz7 = Button(root, text = "7", width=2,  command = lambda:setZoom("7"))   
bz7.grid(row = 5, column = 6, columnspan = colspan  ) 

#delay with grid

bd1 = Button(root, text = "1", width=2, command = lambda:setDelay("1000")) 
bd1.grid(row = 10, column = 0, columnspan = 1)    
bd2 = Button(root, text = "2", width=2, command = lambda:setDelay("2000"))  
bd2.grid(row = 10, column = 1, columnspan = 1)
bd3 = Button(root, text = "3", width=2, command = lambda:setDelay("3000"))  
bd3.grid(row = 10, column = 2, columnspan = 1)
bd4 = Button(root, text = "4", width=2,  command = lambda:setDelay("4000"))  
bd4.grid(row = 10, column = 3, columnspan = 1)
bd5 = Button(root, text = "5", width=2,  command = lambda:setDelay("5000"))  
bd5.grid(row = 10, column = 4, columnspan = 1)
bd6 = Button(root, text = "6", width=2,  command = lambda:setDelay("6000"))  
bd6.grid(row = 10, column = 5, columnspan = 1)
bd7 = Button(root, text = "7", width=2,  command = lambda:setDelay("7000"))  
bd7.grid(row = 10, column = 6, columnspan = 1)
bd8 = Button(root, text = "8", width=2,  command = lambda:setDelay("8000"))  
bd8.grid(row = 10, column = 7, columnspan = 1)
bd9 = Button(root, text = "9", width=2,  command = lambda:setDelay("9000"))  
bd9.grid(row = 10, column = 8, columnspan = 1)
bd10 = Button(root, text = "10", width=2,  command = lambda:setDelay("10000"))  
bd10.grid(row = 10, column = 9, columnspan = 1)

#packing/grid

posLabl.grid(row = 1, column = 0, columnspan = 80)
butStart.grid(row = 2, column = 4, columnspan = 2)
butStop.grid(row = 2, column = 6, columnspan = 2)
zoomLabl.grid(row = 3, column = 0, columnspan = 80)
curntZoomLabl.grid(row = 4, column = 0, columnspan = 80)
delayLabl.grid(row = 6, column = 0, columnspan = 80)
curntDelayLabl.grid(row = 7, column = 0, columnspan = 80)
mapDirLabl.grid(row = 11, column = 0, columnspan = 55)
mapDirBut.grid(row = 12, column = 4, columnspan = 4)
mapDirStat.grid(row= 13, column = 0, columnspan = 7)
mapDirEntry.grid(row = 14, column = 0, columnspan = 25)

#mainloop
root.mainloop()

#close files
log.close()
\$\endgroup\$
3
\$\begingroup\$

Is my code sloppy?

It is, but there are very specifics things you can do to address that. Good news! First, use http://pep8online.com/ to make sure that your code follows PEP 8, which is a standard that most Python coders follow to make sure that every Python code can be easily understood by every Python coder.

from Tkinter import *
import time
from datetime import datetime

You need two spaces after the module imports.

#init variables
global r
r = 0
global delay
delay = 1000
#for testing
global lat
lat = "41.5345"
global lon
lon = "-85.343"
global zoom
zoom = 0
global mapDir
mapDir = ""
global mapDirEnabl
mapDirEnabl = 0

Please don't use global variables, you don't need them and they only clutter your code. The good thing about a function is that I can understand it by only looking at it. With global variables, it's not true anymore and I need to be aware of all those variables.

#clear log file
log = open("xpyMapLog.txt", "w")
log.write("")
log.close()
global log
log = open("xpyMapLog.txt", "a")
logStartMessage = "RUN TIME EXECUTED AT: %s \n" % str(datetime.now())
log.write(logStartMessage)

Python has a powerful logging module that you could use. To clear the log file, simply open it in 'w' mode.

#error check
class safe:
    def __init__(self, function):
        self.function = function

    def __call__(self, *args):
        try:
            return self.function(*args)
        except Exception, e:
            errorMsg = "Error: %r" % str(e)
            log.write(errorMsg)  
            print "Error: %s" % e

Make sure that you know why and when you're using the safe decorator. It's a bad idea most of the time: most exceptions are not recoverable and you should instead 1/ make sure they do not happen 2/ or recover when you know how to recover.

        #
    #
#

Remove those, and either get used to Python's indentation or find an editor that helps you to visualize the scope of your blocks.

#open data file
@safe
def openData():
    global Data
    Data = open("Data.txt", "r")
#
openData()

Why is this a function? You only need Data = open("Data.txt", "r") and nothing else if it's not a function. And you won't need the nasty global. @safe also makes no sense here. What do you want to do when opening the data file fails?

#init root window
root = Tk()
root.geometry("550x300+200+200")
root.title("XPYmap for x-plane 10")

Note that Tkinter is not considered a very nice Python library anymore, but I don't know what is the recommended one nowadays. Please rewrite your code considering what I have said already, and maybe resubmit the result on Code Review when it's done.

\$\endgroup\$
3
\$\begingroup\$

On top of @Quentin's comments, please try to avoid repeting yourself too much and declaring useless variables as it tends to make things harder to read.

bz1 = Button(root, text = "1", width=2, command = lambda:setZoom("1"))
bz1.grid(row = 5, column = 0, columnspan = colspan)
bz2 = Button(root, text = "2", width=2, command = lambda:setZoom("2"))
bz2.grid(row = 5, column = 1, columnspan = colspan  )
bz3 = Button(root, text = "3", width=2,  command = lambda:setZoom("3"))
bz3.grid(row = 5, column = 2, columnspan = colspan  )
bz4 = Button(root, text = "4", width=2,  command = lambda:setZoom("4"))
bz4.grid(row = 5, column = 3, columnspan = colspan  )
bz5 = Button(root, text = "5", width=2,  command = lambda:setZoom("5"))
bz5.grid(row = 5, column = 4, columnspan = colspan  )
bz6 = Button(root, text = "6", width=2,  command = lambda:setZoom("6"))
bz6.grid(row = 5, column = 5, columnspan = colspan  )
bz7 = Button(root, text = "7", width=2,  command = lambda:setZoom("7"))
bz7.grid(row = 5, column = 6, columnspan = colspan  )

bd1 = Button(root, text = "1", width=2, command = lambda:setDelay("1000"))
bd1.grid(row = 10, column = 0, columnspan = 1)
bd2 = Button(root, text = "2", width=2, command = lambda:setDelay("2000"))
bd2.grid(row = 10, column = 1, columnspan = 1)
bd3 = Button(root, text = "3", width=2, command = lambda:setDelay("3000"))
bd3.grid(row = 10, column = 2, columnspan = 1)
bd4 = Button(root, text = "4", width=2,  command = lambda:setDelay("4000"))
bd4.grid(row = 10, column = 3, columnspan = 1)
bd5 = Button(root, text = "5", width=2,  command = lambda:setDelay("5000"))
bd5.grid(row = 10, column = 4, columnspan = 1)
bd6 = Button(root, text = "6", width=2,  command = lambda:setDelay("6000"))
bd6.grid(row = 10, column = 5, columnspan = 1)
bd7 = Button(root, text = "7", width=2,  command = lambda:setDelay("7000"))
bd7.grid(row = 10, column = 6, columnspan = 1)
bd8 = Button(root, text = "8", width=2,  command = lambda:setDelay("8000"))
bd8.grid(row = 10, column = 7, columnspan = 1)
bd9 = Button(root, text = "9", width=2,  command = lambda:setDelay("9000"))
bd9.grid(row = 10, column = 8, columnspan = 1)
bd10 = Button(root, text = "10", width=2,  command = lambda:setDelay("10000"))
bd10.grid(row = 10, column = 9, columnspan = 1)

can be rewritten :

for i in range(7):
    txt = str(i+1)
    but = Button(root, text = txt, width=2, command = lambda:setZoom(txt))
    but.grid(row = 5, column = i, columnspan = colspan)

for i in range(10):
    txt = str(i+1)
    delay = str((i+1)*1000)
    but = Button(root, text = txt,  width=2, command = lambda:setDelay(delay))
    but.grid(row = 10, column = i, columnspan = 1)
\$\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.