Simple Python MVC app for incrementing numbers using buttons

Question

Intro: I am learning about the model view controller (MVC) architecture and I am looking for feedback from more experienced/knowledgeable people so I can check if my understanding is sound, and learn about where I can improve.

---

Background (optional): I have separately implemented a simple metronome app. While developing this, I realised that as the GUI becomes more complex, I need a better method of communication between the metronome logic and the GUI. An example of this would be having objects in the GUI that change colour based on which beat the metronome is currently on. These same objects can also be clicked on by the user to change the sound generated at each beat. A mockup is shown below, where there are 4 beats per bar, the current beat is beat 2 (blue highlight), and beats 1 and 3 will be accented, while 2 and 4 are not accented.

---

This question: I have implemented a basic MVC app using tkinter that displays a number which can be incremented up to some maximum value, before starting over at the value of 1. This is supposed to simulate a metronome in a very simple way, where some event (here it is a user clicking a button), causes the Model's beat value to increment, and this update is subsequently reflected in the View.

I started off with code from an MVC YouTube tutorial, and I have gradually modified it. I would like to address any errors or fundamental misunderstandings before I incorporate my real metronome code and expand the GUI.

Aside: I realise that the Model, View and Controller classes are ordinarily held in different modules, but I kept them in one here for simplicity.

Thanks in advance to anyone who can provide me with some feedback.

import tkinter as tk

'''
An MVC app that displays a number which gets incremented when the user clicks
a button. After exceeding the maximum number, it starts at 1 again.

'''

class Model():
    '''
    The model should be responsible for:
        - maintaining the current beat number
        - maintaining the number of beats per bar
    
    '''
    
    def __init__(self):
        self.beat = 1
        self.beats_per_bar = 4
        self.is_active = False
        
        self.red = "#f0021a"
        self.green = "#73fc03"
        
        
    def increment_beat(self):
        # Handle the case where the beats_per_bar value is reduced below the current beat
        # prior to the increment_beat method being called. Reset it to 1.
        if self.beat >= self.beats_per_bar:
            self.beat = 1
        else:
            self.beat = (self.beat % self.beats_per_bar) + 1


    def reset_beat(self):
        self.beat = 1
        
        
    def update_is_active_state(self, new_state):
        self.is_active = new_state
        
        
    def set_beats_per_bar(self, new_value):
        self.beats_per_bar = new_value
           

class View():
    '''
    The view should display a number and contain the following widgets:
        - button to increment the number
        - button to reset the number to 1
        - button to turn it on and off
        - spinbox to set the maximum value for the number

    '''
    
    # Set up tkinter.
    # This setup method is called from within the Controller class's start method.
    def setup(self, controller):
        # Populate widgets with initial values in here.
        self.root = tk.Tk()
        self.root.geometry("400x350")
        self.root.title("Incrementing Numbers")
        
        self.frame = tk.Frame(self.root, bg="#f0021a")
        self.frame.pack(fill=tk.BOTH, expand=1)
        
        # Label to show current number
        self.label = tk.Label(master=self.frame, text="1", font=("Arial", 42), bg="#f0021a")
        self.label.pack(pady=(20,0))
        
        self.increment_button = tk.Button(master=self.frame, text="Increment Number", state=tk.DISABLED, command=controller.handle_click_increment_number)
        self.increment_button.pack(padx=20, pady=(20, 0), fill=tk.BOTH)
        self.reset_button = tk.Button(master=self.frame, text="Reset Number", state=tk.DISABLED, command=controller.handle_click_reset_number)
        self.reset_button.pack(padx=20, pady=(0, 0), fill=tk.BOTH)
        
        # Add a spinbox to define the maximum allowed number
        self.spinbox_frame = tk.Frame(self.frame, bg="#f0021a")
        self.spinbox_frame.pack(fill=tk.BOTH, expand=0)
        
        self.spinbox_number = tk.DoubleVar(value=4)
        self.spinbox = tk.Spinbox(master=self.spinbox_frame, width=200, from_=1, to=7, state="readonly", justify=tk.CENTER, command=controller.handle_click_spinbox, textvariable=self.spinbox_number)
        self.spinbox_label = tk.Label(master=self.spinbox_frame, text="Beats per bar:", bg="#f0021a", fg="#000")
        
        self.spinbox_label.pack(side=tk.LEFT, padx=(20, 20), pady=(20, 20))
        self.spinbox.pack(side=tk.RIGHT, padx=(0, 20), pady=(20, 20))

        
        # Have another button that will update on/off state and change the background colour
        self.on_off_button = tk.Button(master=self.frame, text="Turn On", command=controller.handle_click_toggle_on_off_state)
        self.on_off_button.pack(padx=20, fill=tk.BOTH)
        
        
    def update_widget_states(self, new_state):
        self.increment_button.config(state=new_state)
        self.reset_button.config(state=new_state)
        
        
    def update_label_text(self, new_text):
        self.label.config(text=f"{str(new_text)}")
        
        
    def update_on_off_button_text(self, new_text):
        self.on_off_button.config(text=new_text)
        
    
    def set_background_colour(self, new_colour): 
        self.frame.config(bg=new_colour)
        self.label.config(bg=new_colour)
        self.spinbox_frame.config(bg=new_colour)
        self.spinbox_label.config(bg=new_colour)
    
    
    def start_main_loop(self):
        self.root.mainloop()


class Controller():
    '''
    The controller should have methods that are called when the user interacts
    with the widgets. e.g. handle_click_increment_number and
    handle_click_reset_number.
    
    '''
    def __init__(self, model, view):
        self.model = model
        self.view = view
    

    def handle_click_increment_number(self):
        self.model.increment_beat()
        new_beat = self.model.beat
        self.view.update_label_text(new_beat)
    
    
    def handle_click_reset_number(self):
        self.model.reset_beat()
        new_beat = self.model.beat
        self.view.update_label_text(new_beat)
    
    
    def handle_click_toggle_on_off_state(self):
        # If we are here, the user has clicked the button to turn it on/off
        if self.model.is_active:
            # Define the new background colour
            new_bg_colour = self.model.red
            # New state for model "is_active" attribute
            new_state = False
            # New text for on/off button
            new_on_off_text = "Turn On"
            # New state for some Button objects in the View
            new_widget_state = tk.DISABLED
            
            # Reset the number and the View's label text
            self.handle_click_reset_number()
        
        else:
            new_bg_colour = self.model.green
            new_state = True
            new_on_off_text = "Turn Off"
            new_widget_state = tk.NORMAL
            
        
        # Update the model's "is_active" state attribute
        self.model.update_is_active_state(new_state)
        # Update the text on the on/off button
        self.view.update_on_off_button_text(new_text=new_on_off_text)
        # Update the state of some Button objects in the View
        self.view.update_widget_states(new_widget_state)
        # Update the background colour
        self.view.set_background_colour(new_bg_colour)
    
    
    def handle_click_spinbox(self):
        new_val = int(self.view.spinbox.get())
        self.model.set_beats_per_bar(new_val)
    
    
    def start(self):
        # View's setup method needs a Controller object passed to it (in this case, self)
        self.view.setup(self)
        
        # Set the background of the view to be red initially (state is inactive)
        bg_colour = self.model.red
        self.view.set_background_colour(bg_colour)
        self.view.start_main_loop()



# Piece it all together
c = Controller(Model(), View())
c.start()

Answer 1

This is a good beginning but perhaps I can offer a few suggestions that might make the code more robust and easier to maintain:

Make Attributes "private"

Although Python does not have the notion of access levels, the names of attributes, methods and classes that clients should not access should begin with an underscore. Attributes that a client can access read-only should be exposed using a property or a method.

Better Naming of Variables

Try to give to a variable a name that conveys that best describes what the variable holds. For example, you have:

new_val = int(self.view.spinbox.get())

This could be improved with:

beats_per_minute = int(self.view.spinbox.get())

Improved Handling of a Change to "Beats per bar"

It seems to me that when the beats per bar are modified by the user, you have the possibility of the current bar number exceeding the beats per bar. My suggestion would be to reset the current beat to 1 whenever any change is made to beats per bar.

Assign Responsibilities to the Proper Class

Your Controller has the Model and View as collaborators (i.e. the Controller has references to these objects and calls methods on them). But otherwise, the Model and View do not collaborate with one another. This is one approach but not the only one upon which I will elaborate later. You have also placed some responsibilities with the wrong object. A few examples:

The Model has defined attributes such as self.red, a background color for the View. But the colors to be used for the background depending on the state of the View, should be the responsibility of the View.

The Model has a method update_is_active_state. From observing what occurs when this method is called, it toggles between enabling and disabling the View's presentation resulting in the updating of labels and colors according to whether the View is enabled or not. It seems to me that this should be the responsibility/concern of the View only and that the Model should not have the notion of it being "active" (i.e. enabled) or not.

The Controller handles the event that occurs when the "Turn On/Turn Off" button is clicked and based on whether the Model is active or not goes about enabling or disabling various widgets, setting background colors and setting button captions. But why should the Controller be making these presentation decisions? Wouldn't it be better for the Controller to tell the View that its state has been toggled and then have the View update its presentation as it seems fit? If you decide to change the presentation, you will only have to update the View class.

The Observer Pattern

Finally, an approach you might want to consider is to implement the Observer Pattern. The Controller responds to a View event that is pertinent to the Model by calling a Model method to update the Model's state. The View registers itself as an interested Observer of the Model to be notified whenever the state of the Model changes. Then when the View is notified of a state change it can query the state of the Model and update its presentation accordingly. I believe that can simplify implementing multiple Views on a Model or multiple Models for a View. I have therefore implemented this pattern in such a way that the View can observe multiple models by defining a separate _ModelObserver class rather than the View implementing the Observer interface itself. If there were multiple Models to be observed by a View, it would instantiate multiple such Observers passing to it a reference to the Model and the View that is to be updated on Model changes.

This pattern could be implemented so that different Observer methods are called according to the type of state change that has occurred. In the code below, since there are very few attributes comprising the Model's state, I have elected for now just to have a single notification method.

Putting the Above Suggestions to Code

import tkinter as tk
from abc import ABC, abstractmethod

'''
An MVC app that displays a number which gets incremented when the user clicks
a button. After exceeding the maximum number, it starts at 1 again.

'''

class Observer(ABC):
    @abstractmethod
    def model_changed(self):
        pass


class Model():
    '''
    The model should be responsible for:
        - maintaining the current beat number
        - maintaining the number of beats per bar

    '''

    def __init__(self):
        self._beat = 1
        self._beats_per_bar = 4
        self._observers = set()

    @property
    def beat(self):
        return self._beat

    @property
    def beats_per_bar(self):
        return self._beats_per_bar

    def increment_beat(self):
        # Handle the case where the beats_per_bar value is reduced below the current beat
        # prior to the increment_beat method being called. Reset it to 1.
        if self._beat >= self._beats_per_bar:
            self._beat = 1
        else:
            self._beat += 1
        self._model_changed()

    def reset_beat(self):
        self._beat = 1
        self._model_changed()

    def set_beats_per_bar(self, beats_per_bar):
        self._beats_per_bar = beats_per_bar
        # Also reset current beat:
        self._beat = 1
        self._model_changed()

    def add_observer(self, observer):
        self._observers.add(observer)

    def remove_observer(self, observer):
        self._observers.remove(observer)

    def _model_changed(self):
        for observer in self._observers:
            observer.model_changed()


class View():
    '''
    The view should display a number and contain the following widgets:
        - button to increment the number
        - button to reset the number to 1
        - button to turn it on and off
        - spinbox to set the maximum value for the number

    '''

    def __init__(self, model):
        self._red = "#f0021a"
        self._green = "#73fc03"
        self._model = model
        self._is_active = False


    def toggle_active_state(self):
        self._is_active = not self._is_active

        if self._is_active:
            new_state = tk.NORMAL
            bg_color = self._green
            on_off_button_text = "Turn Off"
        else:
            new_state = tk.DISABLED
            bg_color = self._red
            on_off_button_text = "Turn On"

        self._increment_button.config(state=new_state)

        self._reset_button.config(state=new_state)
        self._spinbox.config(state=new_state)

        self._frame.config(bg=bg_color)
        self._label.config(bg=bg_color)
        self._spinbox_frame.config(bg=bg_color)
        self._spinbox_label.config(bg=bg_color)

        self._on_off_button.config(text=on_off_button_text)


    @property
    def spinbox_value(self):
        return int(self._spinbox.get())
    

    # Set up tkinter.
    # This setup method is called from within the Controller class's start method.
    def setup(self, controller):
        # Populate widgets with initial values in here.
        self._root = tk.Tk()
        self._root.geometry("400x350")
        self._root.title("Incrementing Numbers")

        self._frame = tk.Frame(self._root, bg=self._red)
        self._frame.pack(fill=tk.BOTH, expand=1)

        # Label to show current number
        self._label = tk.Label(master=self._frame, text="1", font=("Arial", 42), bg=self._red)
        self._label.pack(pady=(20,0))

        self._increment_button = tk.Button(master=self._frame, text="Increment Number", state=tk.DISABLED, command=controller.handle_click_increment_number)
        self._increment_button.pack(padx=20, pady=(20, 0), fill=tk.BOTH)
        self._reset_button = tk.Button(master=self._frame, text="Reset Number", state=tk.DISABLED, command=controller.handle_click_reset_number)
        self._reset_button.pack(padx=20, pady=(0, 0), fill=tk.BOTH)

        # Add a spinbox to define the maximum allowed number
        self._spinbox_frame = tk.Frame(self._frame, bg=self._red)
        self._spinbox_frame.pack(fill=tk.BOTH, expand=0)

        self._spinbox_number = tk.DoubleVar(value=4)
        self._spinbox = tk.Spinbox(master=self._spinbox_frame, width=200, from_=1, to=7, state=tk.DISABLED, justify=tk.CENTER, command=controller.handle_click_spinbox, textvariable=self._spinbox_number)
        self._spinbox_label = tk.Label(master=self._spinbox_frame, text="Beats per bar:", bg=self._red, fg="#000")

        self._spinbox_label.pack(side=tk.LEFT, padx=(20, 20), pady=(20, 20))
        self._spinbox.pack(side=tk.RIGHT, padx=(0, 20), pady=(20, 20))

        # Have another button that will update on/off state and change the background colour
        self._on_off_button = tk.Button(master=self._frame, text="Turn On", command=controller.handle_click_toggle_on_off_state)
        self._on_off_button.pack(padx=20, fill=tk.BOTH)

        # To allow a view to observe multiple models
        class _ModelObserver(Observer):
            """Receives notifications of changes to Model instance."""
            def __init__(self, view, model):
                self._view = view
                self._model = model
                model.add_observer(self)

            def model_changed(self):
                self._view._label.config(text=str(self._model.beat))


        # Create observer on model to update this view:
        _ModelObserver(self, model)

    def start_main_loop(self):
        self._root.mainloop()


class Controller():
    '''
    The controller should have methods that are called when the user interacts
    with the widgets. e.g. handle_click_increment_number and
    handle_click_reset_number.

    '''
    def __init__(self, model, view):
        self._model = model
        self._view = view


    def handle_click_increment_number(self):
        self._model.increment_beat()


    def handle_click_reset_number(self):
        self._model.reset_beat()


    def handle_click_toggle_on_off_state(self):
        # If we are here, the user has clicked the button to turn it on/off
        # Update the model's "is_active" state attribute
        self._view.toggle_active_state()


    def handle_click_spinbox(self):
        beats_per_bar = self._view.spinbox_value
        self._model.set_beats_per_bar(beats_per_bar)


    def start(self):
        # View's setup method needs a Controller object passed to it (in this case, self)
        self._view.setup(self)
        self._view.start_main_loop()



# Piece it all together
model = Model()
view = View(model)
controller = Controller(model, view)
controller.start()
```