4
\$\begingroup\$

I'm trying to improve my programming skills (not just using one language). I don't have a group of other programmers to bounce ideas off other than on the web - so please forgive me if I have posted this in the wrong place.

I'd like to improve my coding in this example. Here's my Python sketch that takes a number and converts it to either Fahrenheit or Celsius. It takes a number to convert with checks in place in the calc function, incase no text is entered (I could also check whether the values entered are computable but I've not done this). It uses type casting to convert the text entry string to an int and if you tick the checkbox it will calculate Fahrenheit. It uses one class to define the whole GUI layout as this could be a sub window in a larger GUI i.e. this could be a class among three or more as part of a larger GUI.

I'd appreciate your feedback about the code and what could be done to make it more efficient e.g. layout, code structure and general feedback. I'd appreciate it.

from Tkinter import Tk, Label, Button, Checkbutton, Entry, W,LEFT, RIGHT, CENTER
import Tkinter as tk

class MyFirstGUI:
    def __init__(self, master):
        self.master = master
        master.title("Celsius <-> Farenheit")

        self.var = tk.IntVar()

        self.label = Label(master, anchor=CENTER, text="Temperature Conversion")
        self.label.grid(columnspan=3, rowspan=2, sticky=W)

        #self.labelInput = Label(master, text="Enter temperature", borderwidth=2, relief="solid")
        #self.labelInput.grid(row=2, column=1)

        self.textEntry = Entry(master, text="enter temperature")
        self.textEntry.grid(row=2, column=1)

        self.calculate_button = Button(master, text="Convert", command=self.calc)
        self.calculate_button.grid(row=2, column=2)
        #self.labelInput.pack(side=LEFT)

        self.converstion_type = Checkbutton(master, text="Farenheit",  variable=var)
        self.converstion_type.grid(row=3, column=1)
        #self.labelInput.pack(side=RIGHT)

        self.output = Label(master, text="...")
        self.output.grid(row=3, column=3)

        self.outputInput = Label(master, text="Converted Temperature", borderwidth=2, relief="solid")
        #self.outputInput.grid(row=1, column=2)
        #self.outputInput.pack(side=RIGHT)

    def calc(self):
        entry = self.textEntry.get()
        if(entry != "" and var.get()==False):
            #convert celsius to celsius
            val =  5*(int(entry)-32)/9
            self.output.config(text=str(val))   

        elif(entry != "" and var.get()==True):
            #convert celsius to farenheit
            val =  9*(int(entry)/5) + 32
            self.output.config(text=str(val))


root = Tk()
var = tk.IntVar()
my_gui = MyFirstGUI(root)
root.mainloop()
\$\endgroup\$
2
\$\begingroup\$

Don't import tkinter twice

You are both importing individual parts of tkinter as well as the module as a whole. Don't do both. Stick with importing the whole library and then using the library as a prefix to the objects. This makes your code more self-documenting.

For example:

import Tkinter as tk
...
self.label = tk.Label(...)
self.textEntry = Entry(...)

...

Separate widget creation from widget layout

Instead of creating a widget, then calling grid, then creating a widget, calling grid, etc, create your widgets in one group and lay them out in another.

self.label = Label(master, anchor=CENTER, text="Temperature Conversion")
self.textEntry = Entry(master, text="enter temperature")
self.calculate_button = Button(master, text="Convert", command=self.calc)
self.converstion_type = Checkbutton(master, text="Farenheit",  variable=var)
self.output = Label(master, text="...")
self.outputInput = Label(master, text="Converted Temperature", borderwidth=2, relief="solid")

self.label.grid(columnspan=3, rowspan=2, sticky=W)
self.textEntry.grid(row=2, column=1)
self.calculate_button.grid(row=2, column=2)
self.converstion_type.grid(row=3, column=1)
self.output.grid(row=3, column=3)
self.outputInput.grid(row=1, column=2)

In my experience, this makes it much easier to visualize the GUI structure in the code, and also makes it easier to change the layout.

If you have multiple sections in your code (eg: a navigation area, a main area, a toolbar, etc.) you should group them by section.

Hide the main code

Even though this is a complete program rather than a reusable library, it's good to get into the habit of moving executable code to a section that won't get run when you import the file.

For example:

if __name__ == "__main__":
    root = Tk()
    var = tk.IntVar()
    my_gui = MyFirstGUI(root)
    root.mainloop()

Some people take the extra step of moving that code to a main function placed at the top of the file:

def main():
    root = Tk()
    var = tk.IntVar()
    my_gui = MyFirstGUI(root)
    root.mainloop()
...
if __name__ == "__main__":
    main()

The choice is simply one of readability. Some people like having their main logic at the top of the file.

Even if you never plan to make this a reusable library, by hiding the main logic like this you can create test cases that import the file and test individual pieces of the code. While that's overkill for this small program, it's a good habit to get into.

For more information see What does if__name__ == "main" do?

\$\endgroup\$
  • \$\begingroup\$ wow - thanks Bryan for taking the time to write this valuable feedback. I really appreciate it and taking what you said onboard. I can see that what you say makes more sense and makes the code clearer. \$\endgroup\$ – awyr_agored Jul 28 '17 at 0:48
1
\$\begingroup\$

In addition to what is mentioned in the previous answer, I suggested you:

  • Leave 2 empty lines between your import statement and the class name (PEP8).
  • MyFirstGUI is not a good name. You should choose one that reflects what your class is actually performing. Maybe something like TemperatureConverter(). This way, if someone reads your program, straightaway, he will know what is it about. Note that this name is better than CelsiusFarenheitTemperatureConverter() because in case you scale, one day in the future, your current program, and decide to enrich it with more temperature units, the first name is more generic, and thus you will not have to rename your program.

The rest of my suggestions is what I already wrote about on my personal website (Tkinter best practices):

  • Write a separate functions where lay all your current and future settings:

    def configure_gui(self):
       self.master.title("Temperature conversion: Celsius <-> Farenheit")
    
  • Write a function that creates and configures your different widgets. Call it, for instance, create_widgets()
  • This later function can consist in only calling to other fine grained functions where each one of them deals with a specific widget in terms of creation and configuration. The advantage of this approach is that you will have an easier unit testing task.
  • Unlike what is suggested by the previous answer, I think it is better to instantiate IntVar() inside the function where your program actually needs it, I mean inside the function which creates and configures the Tkinter.Entry() widget.
  • Your main program should not be more complicated than this:

     if __name__ == "__main__":
         root = Tk.Tk()
         temperature_converter = TemperatureConverter(root)
         root.mainloop()
    
\$\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.