Nsryjdtyk

I've been implementing a simple Python calculator for learning purposes. Since there are no properties or bindings like in Javafx, I implemented my own MVC pattern. I'm aware of missing logic like checking for division by zero and stuff, but I wanted to keep it simple for this review.

So there are some questions:

• Is the MVC pattern okay this way?


• What's the best way to check passed parameters of methods? (see Model.digit(int))


• Is the visibility of fields okay this way?


• Are the imports organized correctly?

import tkinter as tk

from tkinter import *

import tkinter.font

from abc import ABC, abstractmethod

from enum import IntEnum





class Observer(ABC):



    @abstractmethod

    def notify(self):

        pass





class Observable():



    def __init__(self):

        self._observers = set()



    def register(self, observer: Observer):

        self._observers.add(observer)



    def notify_all(self):

        for observer in self._observers:

            observer.notify()





class Operator(IntEnum):

    NONE = 0

    EQUALS = 1

    PLUS = 10

    MINUS = 11

    MULT = 12

    DIV = 13





class Model(Observable):



    def __init__(self):

        Observable.__init__(self);

        self._temp = 0

        self._result = 0

        self._operator = Operator.NONE



    def digit(self, digit: int):

        if digit is None:

            raise TypeError

        if self._operator == Operator.EQUALS:

            self.reset()

        self._temp = self._temp * 10 + digit

        self.notify_all()



    def operator(self, operator: Operator):

        if operator == Operator.EQUALS:

            if self._operator == Operator.PLUS:

                self._temp += self._result

                self._operator = Operator.EQUALS

            elif self._operator == Operator.MINUS:

                self._temp = self._result - self._temp

                self._operator = Operator.EQUALS

            elif self._operator == Operator.MULT:

                self._temp *= self._result

                self._operator = Operator.EQUALS

            elif self._operator == Operator.DIV:

                self._temp = self._result / self._temp

                self._operator = Operator.EQUALS

        elif Operator.PLUS <= operator <= Operator.DIV:

            self._result = self._temp

            self._temp = 0

            self._operator = operator

        else:

            raise NotImplementedError("unexpected enum")

        self.notify_all()



    def reset(self):

        self._temp = 0

        self._result = 0

        self._operator = Operator.NONE

        self.notify_all()





class Controller():



    def __init__(self, model: Model):

        self._model = model



    def digit(self, digit: int):

        self._model.digit(digit)



    def operator(self, operator: Operator):

        self._model.operator(operator)





class View(tk.Tk, Observer):



    CONST_TITLE = "Calculator"

    CONST_GEOMETRY = "300x400"



    def __init__(self):

        tk.Tk.__init__(self)

        self.title(self.CONST_TITLE)

        self.geometry(self.CONST_GEOMETRY)

        self._model = Model()

        self._model.register(self)

        self._controller = Controller(self._model)

        self._frame = tk.Frame(self, bg="white")

        self._frame.pack(fill=BOTH, expand=1)

        for row in range(6):

            self._frame.rowconfigure(row, weight=1)

        for column in range(4):

            self._frame.columnconfigure(column, weight=1)

        self._font = tkinter.font.Font(root=self._frame, family="Helvetica", size="30", weight=tkinter.font.BOLD)

        self._text = tk.Label(self._frame, text="INIT", font=self._font, justify=RIGHT, anchor=E, bg="white", padx=20, pady=20)

        self._text.grid(row="0", column="0", columnspan="4", sticky="NSWE")

        self._button_0 = tk.Button(self._frame, text="0", font=self._font, command=lambda:self._controller.digit(0))

        self._button_0.grid(row="5", column="0", columnspan="2", sticky="NSWE")

        self._button_1 = tk.Button(self._frame, text="1", font=self._font, command=lambda:self._controller.digit(1))

        self._button_1.grid(row="4", column="0", sticky="NSWE")

        self._button_2 = tk.Button(self._frame, text="2", font=self._font, command=lambda:self._controller.digit(2))

        self._button_2.grid(row="4", column="1", sticky="NSWE")

        self._button_3 = tk.Button(self._frame, text="3", font=self._font, command=lambda:self._controller.digit(3))

        self._button_3.grid(row="4", column="2", sticky="NSWE")

        self._button_4 = tk.Button(self._frame, text="4", font=self._font, command=lambda:self._controller.digit(4))

        self._button_4.grid(row="3", column="0", sticky="NSWE")

        self._button_5 = tk.Button(self._frame, text="5", font=self._font, command=lambda:self._controller.digit(5))

        self._button_5.grid(row="3", column="1", sticky="NSWE")

        self._button_6 = tk.Button(self._frame, text="6", font=self._font, command=lambda:self._controller.digit(6))

        self._button_6.grid(row="3", column="2", sticky="NSWE")

        self._button_7 = tk.Button(self._frame, text="7", font=self._font, command=lambda:self._controller.digit(7))

        self._button_7.grid(row="2", column="0", sticky="NSWE")

        self._button_8 = tk.Button(self._frame, text="8", font=self._font, command=lambda:self._controller.digit(8))

        self._button_8.grid(row="2", column="1", sticky="NSWE")

        self._button_9 = tk.Button(self._frame, text="9", font=self._font, command=lambda:self._controller.digit(9))

        self._button_9.grid(row="2", column="2", sticky="NSWE")

        self._button_plus = tk.Button(self._frame, text="+", font=self._font, command=lambda:self._controller.operator(Operator.PLUS))

        self._button_plus.grid(row="2", column="3", rowspan="2", sticky="NSWE")

        self._button_plus = tk.Button(self._frame, text="-", font=self._font, command=lambda:self._controller.operator(Operator.MINUS))

        self._button_plus.grid(row="1", column="3", sticky="NSWE")

        self._button_plus = tk.Button(self._frame, text="*", font=self._font, command=lambda:self._controller.operator(Operator.MULT))

        self._button_plus.grid(row="1", column="2", sticky="NSWE")

        self._button_plus = tk.Button(self._frame, text="/", font=self._font, command=lambda:self._controller.operator(Operator.DIV))

        self._button_plus.grid(row="1", column="1", sticky="NSWE")

        self._button_equals = tk.Button(self._frame, text="=", font=self._font, command=lambda:self._controller.operator(Operator.EQUALS))

        self._button_equals.grid(row="4", column="3", rowspan="2", sticky="NSWE")

        self._button_clear = tk.Button(self._frame, text="C", font=self._font, command=self._model.reset)

        self._button_clear.grid(row="1", column="0", sticky="NSWE")

        self.notify()



    def notify(self):

        self._text.config(text=self._model._temp)





if __name__ == "__main__":

    View().mainloop()

Don't import tkinter twice.

You're importing tkinter twice:

import tkinter as tk

from tkinter import *

Just import it once, and prefix everything with tk.:

import tkinter as tk

...

self._frame.pack(fill=tk.BOTH, ...)

...

self._text = tk.Label(..., justify=tk.RIGHT, anchor=tk.E, ...)

Group all of your layout code together

When you create a widget, then call grid, then create a widget, then call grid, etc, it makes it very hard to visualize the interface, and to see the logical and physical groupings. Instead, separate widget creation from widget layout.

This will also help expose some typos that you have (you try to save four different buttons as self._button_plus).

Example:

    self._text = tk.Label(self._frame, text="INIT", font=self._font, justify=RIGHT, anchor=E, bg="white", padx=20, pady=20)

    self._button_0 = tk.Button(self._frame, text="0", font=self._font, command=lambda:self._controller.digit(0))

    self._button_1 = tk.Button(self._frame, text="1", font=self._font, command=lambda:self._controller.digit(1))

    self._button_2 = tk.Button(self._frame, text="2", font=self._font, command=lambda:self._controller.digit(2))

    self._button_3 = tk.Button(self._frame, text="3", font=self._font, command=lambda:self._controller.digit(3))

    self._button_4 = tk.Button(self._frame, text="4", font=self._font, command=lambda:self._controller.digit(4))

    self._button_5 = tk.Button(self._frame, text="5", font=self._font, command=lambda:self._controller.digit(5))

    self._button_6 = tk.Button(self._frame, text="6", font=self._font, command=lambda:self._controller.digit(6))

    self._button_7 = tk.Button(self._frame, text="7", font=self._font, command=lambda:self._controller.digit(7))

    self._button_8 = tk.Button(self._frame, text="8", font=self._font, command=lambda:self._controller.digit(8))

    self._button_9 = tk.Button(self._frame, text="9", font=self._font, command=lambda:self._controller.digit(9))

    self._button_plus = tk.Button(self._frame, text="+", font=self._font, command=lambda:self._controller.operator(Operator.PLUS))

    self._button_minus = tk.Button(self._frame, text="-", font=self._font, command=lambda:self._controller.operator(Operator.MINUS))

    self._button_mult = tk.Button(self._frame, text="*", font=self._font, command=lambda:self._controller.operator(Operator.MULT))

    self._button_div = tk.Button(self._frame, text="/", font=self._font, command=lambda:self._controller.operator(Operator.DIV))

    self._button_equals = tk.Button(self._frame, text="=", font=self._font, command=lambda:self._controller.operator(Operator.EQUALS))

    self._button_clear = tk.Button(self._frame, text="C", font=self._font, command=self._model.reset)



    self._text.grid(row="0", column="0", columnspan="4", sticky="NSWE")

    self._button_0.grid(row="5", column="0", columnspan="2", sticky="NSWE")

    self._button_1.grid(row="4", column="0", sticky="NSWE")

    self._button_2.grid(row="4", column="1", sticky="NSWE")

    self._button_3.grid(row="4", column="2", sticky="NSWE")

    self._button_4.grid(row="3", column="0", sticky="NSWE")

    self._button_5.grid(row="3", column="1", sticky="NSWE")

    self._button_6.grid(row="3", column="2", sticky="NSWE")

    self._button_7.grid(row="2", column="0", sticky="NSWE")

    self._button_8.grid(row="2", column="1", sticky="NSWE")

    self._button_9.grid(row="2", column="2", sticky="NSWE")

    self._button_plus.grid(row="2", column="3", rowspan="2", sticky="NSWE")

    self._button_minus.grid(row="1", column="3", sticky="NSWE")

    self._button_mult.grid(row="1", column="2", sticky="NSWE")

    self._button_div.grid(row="1", column="1", sticky="NSWE")

    self._button_equals.grid(row="4", column="3", rowspan="2", sticky="NSWE")

    self._button_clear.grid(row="1", column="0", sticky="NSWE")

Use a loop to create nearly identical widgets

Your buttons are all nearly identical, with the only difference being the character that they insert and the value they pass to the controller. You can cut down on the number of lines of code by using a loop, and storing the widgets in a dictionary:

self.buttons = {}

for char in ("0", "1", "2", "3", "4", "5", "6", "7", "8", "9"):

    self.buttons[char] = tk.Button(self._frame, text=char, font=self._font, command=lambda c=char: self._controller.digit(c))

With that, instead of using something like self._button_1, you would use self.buttons['1'] if you need to reference the buttons elsewhere in the code. Plus, by using a loop you reinforce to the reader that these buttons are intended to be virtually identical.

Imports and layout have already been addressed by Bryan Oakley, so I will focus on the remaining two questions:

• Is the MVC pattern OK this way?

Your Controller does not serve any purpose right now. It just passes the value from the View class through to the Model.

The Model on the other hand does all the work like checking what operator to apply and so on. But I think that's OK for now.

What is definitely missing is a reference to View in the Controller class. The task of the Controller is to establish the connection between View and Model and to validate the data which is sent across these two.

As you stated error handling is not part of your implementation yet, but division by zero would be a great example where your Controller definitely needs a reference to the View.

In the current implementation your program would throw a ZeroDivisionError in Model.operator(). You could catch that there, but what next? You have no means to communicate this information back to the view.

The Controller should be responsible to check for a possible division by zero. In your current implementation, Controller.operator() would be the correct place for that. In case of an error you might want to notify the View to display an error message and reset all data in your model.

Furthermore, if your Controller holds a reference to the View, you could also get rid of the Observer pattern. Instead of letting the Model notify its View observer, the digit and operator methods can pass the value of _temp back to the View after calling the corresponding methods on the Model.

• What's the best way to check passed parameters of methods? (see Model.digit(int))

I guess that you want to make sure that the method actually received an integer. Your current implementation (taken from the Model class, although validating the inputs coming from the view should be in the responsibility of the Controller) will only detect if None is passed to it. A string, float or any other type will pass through undetected, potentially causing a TypeError in the second to last line:

def digit(self, digit: int):

    if digit is None:

        raise TypeError

    if self._operator == Operator.EQUALS:

        self.reset()

    self._temp = self._temp * 10 + digit

    self.notify_all()

If you want to validate the type of a parameter beforehand you can da an isinstance check with a suitable superclass. In case of an integer, you would import numbers.Integral and check isinstance(digit, numbers.Integral). You could also directly check isinstance(digit, int), but that narrows down the possible data types to actual int, whereas numbers.Integral also allows for other compatible types which are registered as a virtual subclass of itself.

The same goes for other types. If you want to allow for any iterable, you would check against collections.abc.Iterable instead of list or tuple or whatever.

It is worth mentioning that in Python it is a widespread practice to not check the type at all and just put the critical code in a try ... except block and catching a possible TypeError. In your MVC example however this is not feasible since you want to validate the type already in the Controller before passing it to the Model which does the actual computation.