9
\$\begingroup\$

I have been working for about a week on a calculator in Tkinter, and the result is a very ugly, but functional calculator. Is it possible to clean up the code, and make it more readable?

from Tkinter import *
import tkMessageBox
import sys
import os
import math
main = Tk()
string = []
x = 'add'
def exit():
        sys.exit()
def addNumberOne():
        string.append(1.0)
        Label(main, text = "1").grid(row = 0, column = 0)
def addNumberTwo():
        string.append(2.0)
        print '2'
        Label(main, text = "2").grid(row = 0, column = 0)
def addNumberThree():
        string.append(3.0)
        print '3'
        Label(main, text = "3").grid(row = 0, column = 0)
def addNumberFour():
        string.append(4.0)
        print '4'
        Label(main, text = "4").grid(row = 0, column = 0)
def addNumberFive():
        string.append(5.0)
        print '5'
        Label(main, text = "5").grid(row = 0, column = 0)
def addNumberSix():
        string.append(6.0)
        Label(main, text = "6").grid(row = 0, column = 0)
def addNumberSeven():
        string.append(7.0)
        print '7'
        Label(main, text = "7").grid(row = 0, column = 0)
def addNumberEight():
        string.append(8.0)
        print '8'
        Label(main, text = "8").grid(row = 0, column = 0)
def addNumberNine():
        string.append(9.0)
        print '9'
        Label(main, text = "9").grid(row = 0, column = 0)
def addNumberZero():
        string.append(0)
        print '0'
        Label(main, text = "0").grid(row = 0, column = 0)
def add():
        global x
        x = 'add'
        Label(main, text = "+").grid(row = 0, column = 1)
        #Label(main, text = "You have selected addition.").pack()
        #tkMessageBox.showinfo( "The Answer Is:", "%d"%(
        Label(main, text = 'Changed operator to "add"').grid(row = 1, column = 4, columnspan = 30)
def sub():
        global x
        x = 'sub'
        Label(main, text = "-").grid(row = 0, column = 1)
        Label(main, text = 'Changed operator to "sub"').grid(row = 1, column = 4)
def mult():
    global x
    x = 'mul'
    Label(main, text = "*").grid(row = 0, column = 1)
    Label(main, text = 'Changed operator to "mul"').grid(row = 1, column = 4)
def div():
    global x
    x = 'div'
    Label(main, text = "/").grid(row = 0, column = 1)
    Label(main, text = 'Changed operator to "div"').grid(row = 1, column = 4)
def square_root():
    Label(main, text = "%f"%(math.sqrt(string[0]))).grid(row = 0, column = 3)
def enter():
        global x
        if x == 'add':
        answer = Entry(main)
        answer.grid(row = 0, column = 3, columnspan = 30)
        answer.insert(0, "%f"%(string[0] + string[1]))
                #Label(main, text = "%f"%(string[0] + string[1])).grid(row = 0, column = 3)
                #tkMessageBox.showinfo( "The Answer Is:", "%d"%(string[0] + string[1]))
        elif x == 'sub':
        #        Label(main, text = "The answer is: %d"%(string[0] - string[1])).pack()
        #        tkMessageBox.showinfo( "The Answer Is:", "%d"%(string[0] - string[1]))
    #   Label(main, text = "%f"%(string[0] - string[1])).grid(row = 0, column = 3)
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] - string[1]))
    elif x == 'mul':
        #Label(main, text = "%f"%(string[0] * string[1])).grid(row = 0, column = 3)
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] * string[1]))
    elif x == 'div':
        #Label(main, text = "%f"%(string[0] / string[1])).grid(row = 0, column = 3)
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] / string[1]))
        string.pop(0)
        string.pop(0)
def viewString():
        tkMessageBox.showinfo('viewString', '%s\n[%f,%f] '%(x, string[0], string[1]))
        print string
        print x
def restartAPI():
        os.system("python calctest2.py")
        sys.exit()
def add_entry1():
    string.append(int(entry1.get()))
def add_entry2():
        string.append(int(entry2.get()))
def clear_entry():
    string.pop(0)
        string.pop(0)
Label(main, text = '?').grid(row = 0, column = 3)
Button(main, text = '1', command = addNumberOne).grid(row = 1, column = 0)
Button(main, text = '2', command = addNumberTwo).grid(row = 1, column = 1)
Button(main, text = '3', command = addNumberThree).grid(row = 1, column = 2)
Button(main, text = '4', command = addNumberFour).grid(row = 2, column = 0)
Button(main, text = '5', command = addNumberFive).grid(row = 2, column = 1)
Button(main, text = '6', command = addNumberSix).grid(row = 2, column = 2)
Button(main, text = '7', command = addNumberSeven).grid(row = 3, column = 0)
Button(main, text = '8', command = addNumberEight).grid(row = 3, column = 1)
Button(main, text = '9', command = addNumberNine).grid(row = 3, column = 2)
Button(main, text = '0', command = addNumberZero).grid(row = 4, column = 1)
Button(main, text = '=', command = enter).grid(row = 4, column = 0)
Button(main, text = 'exit', command = exit).grid(row = 4, column = 3)
Button(main, text = '+', command = add).grid(row = 5, column = 0)
Button(main, text = '-', command = sub).grid(row = 5, column = 1)
Button(main, text = '*', command = mult).grid(row = 5, column = 2)
Button(main, text = '/', command = div).grid(row = 5, column = 3)
Button(main, text = 'V', command = viewString).grid(row = 4, column = 2)
Button(main, text = 'RS', command  = restartAPI).grid(row = 3, column = 3)
Button(main, text = 'sqrt', command = square_root).grid(row = 2, column = 3)
Button(main, text = 'clr', command = clear_entry).grid(row = 1, column = 3)
Label(main, text = 'Num1:').grid(row = 6, column = 1)
Label(main, text = 'Num2:').grid(row = 7, column = 1)
entry1 = Entry(main)
entry1.grid(row = 6, column = 2, columnspan = 30)
entry2 = Entry(main)
entry2.grid(row = 7, column = 2, columnspan = 30)
Button(main, text = '>', command = add_entry1).grid(row = 6, column = 0)
Button(main, text = '>', command = add_entry2).grid(row = 7, column = 0)
main.mainloop()
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to CodeReview, silentphoenix. I'm certain you'll get some great answers. \$\endgroup\$ Commented Jul 3, 2015 at 17:11

2 Answers 2

Reset to default
8
\$\begingroup\$

First off, whitespace. You should have two blank lines between each top-level function/class/code block.

Secondly, the standard for naming in Python is snake_case for functions and variables, and PascalCase for classes.

I'm also noticing a lot of repetition with functions like addNumberOne, or addNumberTwo. I'd recommend extracting the repeated code, and putting it into one re-useable function, like this:

def add_number(number):
    string.append(number)
    print number
    Label(main, text=str(number)).grid(row=0, column=0)

You can also do this for your operator functions, div, mult, and others. Here's my refactored version of that.

def add_operator(operator, message):
    global x
    x = operator
    Label(main, text=operator).grid(row=0, column=1)
    Label(main, text=message).grid(row=1, column=4)

I also noticed that you're using % for string formatting. If you're using Python 2.6 or higher, this is deprecated, and it's recommended that you use str.format instead. Here's an example:

# Normal str.format, without positional
# or named parameters.
print "{} {}".format("Hello", "world")

# str.format using positional parameters.
print "{1} {0}".format("world", "Hello")

# str.format using named parameters.
print "{word1} {word2}".format(word1="Hello", word2="world")

Finally, when calling main.mainloop(), you need to add an if __name__ == "__main__": guard to it. See this Stackoverflow question for more details.

\$\endgroup\$
0
9
\$\begingroup\$

enter()

def enter():
        global x
        if x == 'add':
        answer = Entry(main)
        answer.grid(row = 0, column = 3, columnspan = 30)
        answer.insert(0, "%f"%(string[0] + string[1]))
                #Label(main, text = "%f"%(string[0] + string[1])).grid(row = 0, column = 3)
                #tkMessageBox.showinfo( "The Answer Is:", "%d"%(string[0] + string[1]))
        elif x == 'sub':
        #        Label(main, text = "The answer is: %d"%(string[0] - string[1])).pack()
        #        tkMessageBox.showinfo( "The Answer Is:", "%d"%(string[0] - string[1]))
    #   Label(main, text = "%f"%(string[0] - string[1])).grid(row = 0, column = 3)
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] - string[1]))
    elif x == 'mul':
        #Label(main, text = "%f"%(string[0] * string[1])).grid(row = 0, column = 3)
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] * string[1]))
    elif x == 'div':
        #Label(main, text = "%f"%(string[0] / string[1])).grid(row = 0, column = 3)
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] / string[1]))
        string.pop(0)
        string.pop(0)
  • Remove the noisy comments

These seem to be past code that you are no longer using. Get rid of them; they are making your code harder to read.

def enter():
        global x
        if x == 'add':
        answer = Entry(main)
        answer.grid(row = 0, column = 3, columnspan = 30)
        answer.insert(0, "%f"%(string[0] + string[1]))

        elif x == 'sub':
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] - string[1]))
    elif x == 'mul':
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] * string[1]))
    elif x == 'div':
        answer = Entry(main)
                answer.grid(row = 0, column = 3, columnspan = 30)
                answer.insert(0, "%f"%(string[0] / string[1]))
        string.pop(0)
        string.pop(0)
  • Your indentation is incosistent and incorrect

Some lines have an extra indent, like the if for 'add' and the lines when you are setting the position of answer.

def enter():
    global x
    if x == 'add':
        answer = Entry(main)
        answer.grid(row = 0, column = 3, columnspan = 30)
        answer.insert(0, "%f"%(string[0] + string[1]))

    elif x == 'sub':
        answer = Entry(main)
        answer.grid(row = 0, column = 3, columnspan = 30)
        answer.insert(0, "%f"%(string[0] - string[1]))

    elif x == 'mul':
        answer = Entry(main)
        answer.grid(row = 0, column = 3, columnspan = 30)
        answer.insert(0, "%f"%(string[0] * string[1]))

    elif x == 'div':
        answer = Entry(main)
        answer.grid(row = 0, column = 3, columnspan = 30)
        answer.insert(0, "%f"%(string[0] / string[1]))
        
    string.pop(0)
    string.pop(0)
  • You are repeating code

Notice some consistent among each case of your if/elif statement? These lines:

answer = Entry(main)
answer.grid(row = 0, column = 3, columnspan = 30)

Put these two lines at the top of your function, just before if/elif statement and then remove them from the rest of your function.

def enter():
    global x
    answer = Entry(main)
    answer.grid(row = 0, column = 3, columnspan = 30)
    if x == 'add':
        answer.insert(0, "%f"%(string[0] + string[1]))

    elif x == 'sub':
        answer.insert(0, "%f"%(string[0] - string[1]))

    elif x == 'mul':
        answer.insert(0, "%f"%(string[0] * string[1]))

    elif x == 'div':
        answer.insert(0, "%f"%(string[0] / string[1]))
        
    string.pop(0)
    string.pop(0)
  • This function will build up in noise as you expand your calculator.

Sometime in the future, you may try and add more functions to this calculator. Depending on how many more functions you add, this if/elif statement can get quite lengthy. And, some operations won't just be a single character like "+" or "/".

I recommend creating a dictionary/map where you map the name of an operation to a function. Then, in this function, all you have to do is call the function associated with the operation.

Here is what I mean:

operations = {
    "add": add,
    "sub": sub,
    "mul": mul,
    "div", div
}

Where the functions add, sub, mul, and div arithmetic functions that will take two numerical parameters.

Now, your enter function becomes this:

def enter():
    global x
    operations[x](string[0], string[1])

    string.pop(0)
    string.pop(0)

Check that out. This function just went from a giant jumble of text to just 4 lines.

EthanBierlein already covered most of the big parts of your code. I just a few more small things to point out.

x is really bad name for what you are using the variable name for. I recommend calling it operation.

That big chunk of code where you set up the buttons is very unappealing.

I came up with this way to handle the numbers:

  1. Setup a for loop with i.
  2. Setup an array of tuples. Each tuple will hold the row and column of a number button).
  3. Loop through the array and place the number buttons where the ith tuple says.

Here is the code:

positions = [(1,0),(1,1),(1,2)...]
for i in range(10):
    Button(main, text = str(i), command = add_number(i)).grid(row = positions[i][0], column = positions[i][1])

This uses the add_number function from EthanBeirlein's answer.

\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.