1

I have been programming for a long time. Only recently have I decided to take a stab at python (I should be working with C# as I am in school for it, but I don't care for windows. Long story)

Anyway I was on this site and it showed source for a calculator. I took it, and put it in PyCharm and started to study. By the time I was done I had changed the source significantly. I had added keyboard binding and reduced a lot of the redundant code in it.

My question is simple, is this code that I wrote efficient from a python standard viewpoint?

# -*-coding: utf-8-*-
# !/usr/bin/python3.5

from tkinter import Tk, Button, Entry, END
import math

class Calc:
    def getandreplace(self):  # replace x, + and % to symbols that can be used in calculations
        # we wont re write this to the text box until we are done with calculations

        self.txt = self.e.get() # Get value from text box and assign it to the global txt var
        self.txt = self.txt.replace('÷', '/')
        self.txt = self.txt.replace('x', '*')
        self.txt = self.txt.replace('%', '/100')

    def evaluation(self, specfunc):  # Evaluate the items in the text box for calculation specfunc = eq, sqroot or power
        self.getandreplace()
        try:
            self.txt = eval(str(self.txt))  # evaluate the expression using the eval function
        except SyntaxError:
            self.displayinvalid()
        else:
            if any([specfunc == 'sqroot', specfunc == 'power']):  # Square Root and Power are special
                self.txt = self.evalspecialfunctions(specfunc)

            self.refreshtext()

    def displayinvalid(self):
        self.e.delete(0, END)
        self.e.insert(0, 'Invalid Input!')

    def refreshtext(self):  # Delete current contents of textbox and replace with our completed evaluatioin
        self.e.delete(0, END)
        self.e.insert(0, self.txt)

    def evalspecialfunctions(self, specfunc):  # Calculate square root and power if specfunc is sqroot or power
        if specfunc == 'sqroot':
            return math.sqrt(float(self.txt))
        elif specfunc == 'power':
            return math.pow(float(self.txt), 2)

    def clearall(self): # AC button pressed on form or 'esc" pressed on keyboard
        self.e.delete(0, END)
        self.e.insert(0, '0')

    def clear1(self, event=None):
        # C button press on form or backspace press on keyboard event defined on keyboard press

        if event is None:
            self.txt = self.e.get()[:-1]  # Form backspace done by hand
        else:
            self.txt = self.getvalue()  # No need to manually delete when done from keyboard

            self.refreshtext()

    def action(self, argi: object):  # Number or operator button pressed on form and passed in as argi
        self.txt = self.getvalue()

        self.stripfirstchar()

        self.e.insert(END, argi)

    def keyaction(self, event=None):  # Key pressed on keyboard which defines event
        self.txt = self.getvalue()

        if event.char.isdigit():
            self.stripfirstchar()
        elif event.char in '/*-+%().':
            self.stripfirstchar()
        elif event.char == '\x08':
            self.clear1(event)
        elif event.char == '\x1b':
            self.clearall()
        elif event.char == '\r':
            self.evaluation('eq')
        else:
            self.displayinvalid()
            return 'break'

    def stripfirstchar(self):  # Strips leading 0 from text box with first key or button is pressed
        if self.txt[0] == '0':
            self.e.delete(0, 1)

    def getvalue(self):  # Returns value of the text box
        return self.e.get()

    def __init__(self, master):  # Constructor method
        self.txt = 'o'  # Global var to work with text box contents
        master.title('Calulator')
        master.geometry()
        self.e = Entry(master)
        self.e.grid(row=0, column=0, columnspan=6, pady=3)
        self.e.insert(0, '0')
        self.e.focus_set()  # Sets focus on the text box text area

        # Generating Buttons
        Button(master, text="=", width=10, command=lambda: self.evaluation('eq')).grid(row=4, column=4, columnspan=2)
        Button(master, text='AC', width=3, command=lambda: self.clearall()).grid(row=1, column=4)
        Button(master, text='C', width=3, command=lambda: self.clear1()).grid(row=1, column=5)
        Button(master, text="+", width=3, command=lambda: self.action('+')).grid(row=4, column=3)
        Button(master, text="x", width=3, command=lambda: self.action('x')).grid(row=2, column=3)
        Button(master, text="-", width=3, command=lambda: self.action('-')).grid(row=3, column=3)
        Button(master, text="÷", width=3, command=lambda: self.action('÷')).grid(row=1, column=3)
        Button(master, text="%", width=3, command=lambda: self.action('%')).grid(row=4, column=2)
        Button(master, text="7", width=3, command=lambda: self.action('7')).grid(row=1, column=0)
        Button(master, text="8", width=3, command=lambda: self.action('8')).grid(row=1, column=1)
        Button(master, text="9", width=3, command=lambda: self.action('9')).grid(row=1, column=2)
        Button(master, text="4", width=3, command=lambda: self.action('4')).grid(row=2, column=0)
        Button(master, text="5", width=3, command=lambda: self.action('5')).grid(row=2, column=1)
        Button(master, text="6", width=3, command=lambda: self.action('6')).grid(row=2, column=2)
        Button(master, text="1", width=3, command=lambda: self.action('1')).grid(row=3, column=0)
        Button(master, text="2", width=3, command=lambda: self.action('2')).grid(row=3, column=1)
        Button(master, text="3", width=3, command=lambda: self.action('3')).grid(row=3, column=2)
        Button(master, text="0", width=3, command=lambda: self.action('0')).grid(row=4, column=0)
        Button(master, text=".", width=3, command=lambda: self.action('.')).grid(row=4, column=1)
        Button(master, text="(", width=3, command=lambda: self.action('(')).grid(row=2, column=4)
        Button(master, text=")", width=3, command=lambda: self.action(')')).grid(row=2, column=5)
        Button(master, text="√", width=3, command=lambda: self.evaluation('sqroot')).grid(row=3, column=4)
        Button(master, text="x²", width=3, command=lambda: self.evaluation('power')).grid(row=3, column=5)

        # bind key strokes
        self.e.bind('<Key>', lambda evt: self.keyaction(evt))


# Main
root = Tk()
obj = Calc(root)  # object instantiated
root.mainloop()

I don't really care for the names of some of the function names and variable names. I like using descriptive names, so names like self.e would have been called self.textbox or something. so these things are leftovers from the web copy I found and haven't changed them.

Riv
  • 323
  • 2
  • 8
  • 4
    Hello and Welcome to StackOverflow. If there are no errors or problems with your code, your question might receive better feedback or answers on [StackExchanges's Code Review](http://codereview.stackexchange.com/) – chickity china chinese chicken Sep 20 '18 at 23:45
  • As a side note, Windows is not a necessity for C#. There is a cross platform open source version of c#. You can check it out here: https://www.mono-project.com/. – John Forbes Sep 20 '18 at 23:49
  • Oh, thanks, I didn't know about the review site, though I did try and google it, I really didn't think this was the best place. I will post there. I also didn't know about the cross platform for c#, seriously checking that out. thanks guys – Riv Sep 20 '18 at 23:55
  • The if/elif/else bit in the middle isn't too nice, check out [this answer](https://stackoverflow.com/questions/17166074/most-efficient-way-of-making-an-if-elif-elif-else-statement-when-the-else-is-don) to fix that bit. Not really a site to review full programs for efficiency. – d_kennetz Sep 21 '18 at 00:23

1 Answers1

0

Not really the place for code reviews, but I'm going to do it anyway because I'm hardcore procrastinating.

Good code

First up, your code works (I assume), it looks fine. Good code is good code in any language. Some people obsess over code being Pythonic, I'm not one of those people.

GUI structuring

GUI code sucks. It is repetitive, verbose, fiddly and ugly. This isn't a critique of your code, it is a comment on all GUI code, there doesn't seem to be a good solution. We manage this by separating the GUI code from everything else. There are formal approaches to this like Model-View-Controller and principals like the Separation of Concerns. The key is to fully split your ugly fragile GUI code from the real code you care about so we can try and forget how ugly it is.

Your implementation closely ties the GUI with the functionality. This is a trivial level problem so this isn't so bad. However as a learning exercise you should split them and do it the "right" way, start by creating a second class and move the evaluation function across.

eval

The second big thing is the use of the eval function. It is very elegant, convert the input into python compatible math and just ask python to give you the answer. Putting it slightly differently you are taking user input, filtering it a bit and executing it outright. With my security background it gives me the shakes. Not an issue for this program, you are executing locally, the user can't do anything they can't do normally. Don't do anything like this with online code though.

main

Finally, standard practice when combining classes and code like this is to put the code behind a __main__ conditional. This allows you to import the code elsewhere for testing etc. The following link explains it nicely:

Community
  • 1
  • 1
lod
  • 1,098
  • 10
  • 13
  • Thanks so much. being new I hadn't thought about multi classes and such. I would have came around to that eventually, and maybe I'll keep playing with it. In C++ I would have broke this out to at least three classes. Eval, GUI and input checking. Thanks for the words of wisdom, its appreciated. – Riv Sep 21 '18 at 01:21