3
\$\begingroup\$

I'm designing a board game with my wife, and am writing a console based program that can track and analyze the statistics of playtests to see if the game is balanced, what the dominant strategies are, etc.

My first goal is to write a console based program for our own use, and maybe add a GUI on top later (which would be a good occasion to learn something like PySide). After that, I'd like to make a more user friendly app-version of the stats-tracker so that other people can track stats on their phones during open playtesting (which would be great to learn about app development). I'd like to automatically track stats from a Tabletop Simulator mod as well (I have some experience with Lua and TTS modding, but would be a good way to learn more) and maybe eventually, I could even develop some tools to help other board game designers with their playtesting. I know that this is all rather ambitious (especially since I'm not that experienced!) but I think this can be a project that can help me learn a lot of valuable things.

I've been trying to learn how to apply Model-View-Presenter to structure my program, and this is my first attempt at it. Right now, it's just the player select screen, and admittedly, there's a large amount of boilerplate just for that screen. But I think the MVP can help keeping the program structured as it grows. I tried to apply some of the advice I got in this thread. I have the following questions:

  • Has the Model-View-Presenter been implemented well? Are the responsibilities divided up as they should? I decided to link the view with the presenter through a simple callback method, is this a good idea?
  • Are the classes structured in a logical way and are the methods readable? I generally try to adhere to the Single Responsibility Principle, and find it relatively intuitive for functions, but find it harder for classes. Main area of concern is the Presenter class: I've already split out the logic for formatting the player table, but the class is still quite large. Is this a problem?
  • I'm still rather inexperienced at exception handling, has it been dealt with correctly?
  • How detailed should the docstrings be? I used ChatGPT to generate some of the docstrings as I find this difficult to do, and decided to use ChatGPT's more detailed docstrings for the main methods, and wrote my own less detailed docstrings for the "sub-methods". Is this the right way to go?

Any other advice and feedback is also very welcome!

#model.py

from collections.abc import Iterable
from sortedcontainers import SortedSet

# In the final implementation, this should connect up to a database.

class Model:
    def __init__(self, player_names: Iterable):
        self._player_names = SortedSet(player_names)
        self._selected_players = SortedSet()

    @property
    def amount_all_players(self) -> int:
        """Returns the total number of players."""
        return len(self._player_names)

    @property
    def amount_selected_players(self) -> int:
        """Returns the total number of selected players."""
        return len(self._selected_players)

    def get_slice_all_players(self, begin: int = None, end: int = None) -> list:
        """Returns a slice of the player list from the given beginning index up to,
        but not including, the given end index."""
        return self._player_names[begin:end]

    def get_selected_players(self) -> set:
        """Returns set of selected players."""
        return set(self._selected_players)

    def get_player_by_index(self, player_index: int) -> str:
        """Returns the player name at the given index. Raises IndexError if index is out of range."""
        try:
            return self._player_names[player_index]
        except IndexError:
            raise IndexError(f"player index {player_index} out of range")

    def get_index_by_name(self, player_name: str) -> int:
        """Returns the index of the player with the given name. Raises ValueError if player not found."""
        try:
            return self._player_names.index(player_name)
        except ValueError:
            raise ValueError(f"player {player_name} not found")

    def players_binary_search(self, search: str) -> int:
        """Performs a binary search on the player names."""
        return self._player_names.bisect_left(search)

    def index_startswith(self, index: int, search: str) -> bool:
        """Checks whether the player name at the given index starts with the search string."""
        return True if self._player_names[index].startswith(search) else False

    def add_new_player(self, player_name: str) -> None:
        """Adds a new player with the given name. Raises ValueError if player already exists."""
        if player_name in self._player_names:
            raise ValueError(f"player '{player_name}' already exists")
        else:
            self._player_names.add(player_name)

    def add_selected_player(self, player_name: str) -> None:
        """Adds a player to selected players. Raises ValueError if player doesn't exist
        or player already selected."""
        if player_name not in self._player_names:
            raise ValueError(f"player '{player_name}' doesn't exist")
        elif player_name in self._selected_players:
            raise ValueError(f"player '{player_name}' already selected")
        else:
            self._selected_players.add(player_name)
#view.py

from collections.abc import Callable
from typing import Optional


class View:
    def __init__(self):
        self.on_command: Optional[Callable] = None

    def display_text(self, text: str) -> None:
        """Displays given text to the user."""
        print(text)

    def get_command(self, prompt: str) -> None:
        """Retrieves a command from the user and passes it to the on_command function."""
        command = input(prompt).lower()
        self.on_command(command)

    def enter_to_continue(self, text: str) -> None:
        """Displays the given text to the user and waits for them to press Enter."""
        input(text)

    def clear(self) -> None:
        """Clears the console by printing 80 newline characters."""
        print("\n" * 80)
#presenter.py

from collections.abc import Callable
import re
import textwrap
from typing import Optional
from model import Model
from table_formatter import TableFormatter
from view import View

DEBUG = False


class Presenter:
    def __init__(self, model: Model, view: View):
        self.model = model
        self.view = view
        self.table_formatter = TableFormatter()

        self.view.on_command = self.handle_user_command

        self.current_page_number: int = 1
        self.current_table: str = ""
        self.current_status: str = "STATUS:"

    @property
    def amount_pages(self) -> int:
        """Calculates the total number of pages needed to display all players, based on the number of names
        that can be displayed per page."""
        return 1 + (self.model.amount_all_players // self.table_formatter.names_per_page)

    def start(self) -> None:
        """Initiates the interaction with the user, by updating the view and prompting for user command."""
        self._update_table()
        self._update_view()
        self._get_command()

    def handle_user_command(self, user_input: str) -> None:
        """Handles user command by parsing it, executing the corresponding function and updating the view."""
        try:
            function = self._get_function(user_input)
            valid_status_info = function(self._strip_command(user_input))
        except Exception as e:
            if DEBUG:
                raise
            self._update_invalid_status(user_input, str(e))
        else:
            self._update_table()
            self._update_valid_status(valid_status_info)

        self._update_view()
        self._get_command()

    def _get_command(self) -> None:
        """Asks user for a command."""
        self.view.get_command("Input command here (type ? for help): ")

    def _get_function(self, user_input: str) -> Optional[Callable]:
        """Maps user input to corresponding function, if any. Raises ValueError if command is not recognized."""
        user_commands = (
            (re.compile(r'^\?$'), self._give_help),
            (re.compile(r'^new .*'), self._add_new_player),
            (re.compile(r'^select \d+$'), self._select_player_by_index),
            (re.compile(r'^select .*'), self._select_player_by_name),
            (re.compile(r'^goto \d+$'), self._goto_by_page_num),
            (re.compile(r'^goto .*'), self._goto_by_search)
        )

        for regex, function in user_commands:
            if regex.search(user_input):
                return function
        raise ValueError(f"no matching command for '{user_input}'")

    def _strip_command(self, user_input: str) -> str | None:
        """Removes the command from the user's input, leaving only the argument."""
        try:
            _, argument = user_input.split(maxsplit=1)
            return argument
        except ValueError:
            return

    def _give_help(self, user_input=None) -> None:
        """Displays help text containing available commands and their usage."""
        help_text = textwrap.dedent("""
            HELP
            ----

            Available Commands:

            new: 
                Add a new player to the database.
                Usage: 'new NAME' (e.g., 'new john cleese')

            select: 
                Select a player to participate in the game.
                Usage: 'select INDEX' or 'select NAME' (e.g., 'select 24' or 'select michael palin')

            goto: 
                Navigate to a specific page.
                Usage: 'goto PAGE' or 'goto TERM' 
                (e.g., 'goto 5' to go to page 5, or 'goto er' to go to the first page with a name starting with 'er'
                or the closest match if there's no exact match)

            Press enter to continue...
        """)

        self.view.clear()
        self.view.enter_to_continue(help_text)

    def _add_new_player(self, player_name: str) -> str:
        """Adds a new player to the model. Raises ValueError if player already exists."""
        self.model.add_new_player(player_name)
        player_index = self.model.get_index_by_name(player_name)
        return f"new player '{player_name}' added (index: {player_index})"

    def _select_player_by_index(self, player_index: int | str) -> str:
        """Selects a player based on the provided index. Raises IndexError if index out of range
        or ValueError if player already selected."""
        player_name = self.model.get_player_by_index(int(player_index))
        self.model.add_selected_player(player_name)
        player_num = self.model.amount_selected_players
        return f"{player_name} selected (player {player_num})"

    def _select_player_by_name(self, player_name: str) -> str:
        """Selects a player based on the provided name. Raises ValueError if player doesn't exist
        or player already selected."""
        self.model.add_selected_player(player_name)
        player_num = self.model.amount_selected_players
        return f"{player_name} selected (player {player_num})"

    def _goto_by_page_num(self, page_num: str | int) -> None:
        """Move to a specific page number. Raises IndexError if page number out of range."""
        if not 0 < int(page_num) <= self.amount_pages:
            raise IndexError(f"page number {page_num} is out of range")
        else:
            self.current_page_number = int(page_num)

    def _goto_by_search(self, search: str) -> None | str:
        """Navigates to the page containing the first player name that starts with the given search string.
        If no exact prefix match is found, navigates to the closest match instead."""
        bisect_result = self.model.players_binary_search(search)
        page_num = self._page_num_from_index(bisect_result)
        self._goto_by_page_num(page_num)
        if not self.model.index_startswith(bisect_result, search):
            return f"no exact prefix-match found for '{search}'"

    def _page_num_from_index(self, player_index: int) -> int:
        """Converts a player's index into the corresponding page number."""
        return 1 + ((1 + player_index) // self.table_formatter.names_per_page)

    def _update_table(self) -> None:
        """Updates the current table of players, considering currently selected players and the current page."""
        player_names = self.model.get_slice_all_players(*self._calc_page_indexes())
        selected_players = self.model.get_selected_players()
        new_table = self.table_formatter.create_table(player_names=player_names,
                                                      selected_players=selected_players,
                                                      page_number=self.current_page_number,
                                                      total_pages=self.amount_pages
                                                      )
        self.current_table = new_table

    def _calc_page_indexes(self) -> tuple[int, int]:
        """Calculates the starting and ending indexes for the current page."""
        start = self.table_formatter.names_per_page * (self.current_page_number - 1)
        end = self.table_formatter.names_per_page * self.current_page_number
        return start, end

    def _update_valid_status(self, information: str | None = None) -> None:
        """Updates the status message after a successful operation."""
        if information:
            self.current_status = f"STATUS: {information}."
        else:
            self.current_status = "STATUS:"

    def _update_invalid_status(self, user_input: str | None = None, information: str | None = None) -> None:
        """Updates the status message after an unsuccessful operation."""
        if information:
            self.current_status = f"STATUS: INVALID INPUT ({information})."
        else:
            self.current_status = f"STATUS: INVALID INPUT '{user_input}'."

    def _update_view(self) -> None:
        """Updates the view with the current table of players and status message."""
        new_view = (
            f"PLAYER SELECT\n"
            f"-------------\n\n"
            f"{self.current_table}\n\n"
            f"{self.current_status}\n"
        )

        self.view.clear()
        self.view.display_text(new_view)

#table_formatter.py

from itertools import zip_longest
from wcwidth import wcswidth


class TableFormatter:
    """
     Initializes the table formatter with the given settings.

     Args:
         num_columns (int): The number of columns in the table.
         col_length (int): The maximum length of each column.
         col_width (int): The width of each column.
         col_spacing (int): The amount of spacing between columns.
         truncate_amount (int): The amount of characters to truncate in case of overflow.
     """
    def __init__(self,
                 num_columns: int = 3,
                 col_length: int = 8,
                 col_width: int = 24,
                 col_spacing: int = 3,
                 truncate_amount: int = 3
                 ):

        self.num_columns = num_columns
        self.col_length = col_length
        self.col_width = col_width
        self.col_spacing = col_spacing
        self.truncate_amount = truncate_amount

    @property
    def names_per_page(self) -> int:
        """Returns the total number of names that can fit on a single page."""
        return self.num_columns * self.col_length

    def create_table(self,
                     player_names: list[str],
                     selected_players: set[str],
                     page_number: int,
                     total_pages: int,
                     ) -> str:
        """
        Generates a formatted table of player names with pagination.

        Args:
            player_names (list[str]): List of all player names.
            selected_players (set[str]): Set of selected player names.
            page_number (int): The current page number.
            total_pages (int): The total number of pages.

        Returns:
            str: A string representation of the table.
        """
        formatted_players = self._format_players(player_names, selected_players, page_number)
        player_table = self._format_table(formatted_players, len(player_names))
        page_info = self._format_page_info(total_pages, page_number)
        return player_table + "\n" + page_info

    def _format_players(self, player_names: list, selected_players: set, page_number: int) -> list[str]:
        """Formats player names into a table, displaying their index and name.
        Truncates name if longer than the column width, adds strikethrough to selected players,
        pads index and end of name (if necessary)."""
        table_entries = []

        for i, player_name in enumerate(player_names, start=self._calc_start_index(page_number)):
            entry = f"{self._pad_index(i)}: {player_name}"
            entry = self._truncate(entry)
            entry = self._strikethrough(entry, player_name, selected_players)
            entry = self._pad_end(entry)
            table_entries.append(entry)

        return table_entries

    def _format_table(self, table_entries: list[str], player_amount: int) -> str:
        """Divides table entries into rows, joins rows in a string, adds empty rows to keep
        table size consistent (if necessary)."""
        rows = self._divide_into_rows(table_entries)
        table = self._join_rows(rows)
        table = self._add_empty_rows(table, player_amount)

        return table

    def _format_page_info(self, total_pages: int, page_number: int) -> str:
        """Creates a string showing the current page number and total pages."""
        return f"page {page_number} of {total_pages}"

    def _calc_start_index(self, page_number: int) -> int:
        """Calculates the first index of the page"""
        return self.names_per_page * (page_number - 1)

    def _pad_index(self, name_index: int) -> str:
        """Adds a whitespace to an index if there are wider indexes in the same column."""
        last_index_of_column = name_index + (self.col_length - 1) - (name_index % self.col_length)
        if len(str(name_index)) < len(str(last_index_of_column)):
            return str(name_index) + " "
        return str(name_index)

    def _truncate(self, entry: str) -> str:
        """Truncates name if it is longer than column with, adds trailing ..."""
        if len(entry) > self.col_width:
            truncated_width = self.col_width - self.truncate_amount
            return entry[:truncated_width] + ("." * self.truncate_amount)
        return entry

    def _strikethrough(self, entry: str, player_name: str, selected_players: set[str]) -> str:
        """Adds strikethrough to selected player."""
        strike_char = '\u0336'
        if player_name in selected_players:
            return strike_char.join(entry) + strike_char
        return entry

    def _pad_end(self, entry: str) -> str:
        """Pads the end of the string to match column width and adds whitespaces for column spacing."""
        if wcswidth(entry) < self.col_width:
            amount_whitespaces = self.col_width - wcswidth(entry) + self.col_spacing
            return entry + (" " * amount_whitespaces)
        return entry + (" " * self.col_spacing)

    def _divide_into_rows(self, table_entries: list[str]) -> list[tuple[str, ...]]:
        """Divides the table entries into rows."""
        columns_on_page = 1 + (len(table_entries) // self.col_length)
        columns = [[] for _ in range(columns_on_page)]
        for i, name in enumerate(table_entries):
            columns[i // self.col_length].append(name)
        return list(zip_longest(*columns, fillvalue=""))

    def _join_rows(self, rows: list[tuple[str, ...]]) -> str:
        """Joins the rows into a table string."""
        table = ""
        for row in rows:
            table += "".join(row) + "\n"
        return table

    def _add_empty_rows(self, table: str, amount_players: int) -> str:
        """Pads table with empty rows if the amount of players on the page is smaller than the column length."""
        empty_rows = self.col_length - amount_players
        if empty_rows > 0:
            table += "\n" * empty_rows
        return table

#main.py

# Generates random names for the example program, names will eventually be stored in a database.

import random
from model import Model
from presenter import Presenter
from view import View

first_names = ['sara', 'sofie', 'julia', 'myla', 'zofia', 'emma', 'zoey', 'yarah', 'olivia', 'tess', 'milou', 'lotte',
               'saar', 'hannah', 'liv', 'eva', 'anna', 'noor', 'nina', 'lauren', 'lois', 'emily', 'lieke', 'isa',
               'elynn', 'maud', 'fien', 'roos', 'lina', 'luna', 'ella', 'nova', 'hailey', 'mia', 'fleur', 'julie',
               'fenna', 'beau', 'noa', 'lilly', 'esmee', 'puck', 'cato', 'maeve', 'noah', 'lucas', 'luka', 'levi',
               'sem', 'daan', 'mylan', 'james', 'muhammed', 'mees', 'adam', 'sam', 'bram', 'zayn', 'mads', 'benjamin',
               'jesse', 'max', 'boaz', 'siem', 'teun', 'kay', 'julian', 'thomas', 'vinz', 'jaxx', 'gijs', 'thijs',
               'seb', 'olivier', 'lars', 'guus', 'joep', 'jack', 'ayden', 'seff', 'owen', 'jan', 'hugo', 'morris',
               'david']


last_names = ['jong', 'jansen', 'vries', 'berg', 'dijk', 'bakker', 'janssen', 'visser', 'smit', 'meyer', 'boer',
              'mulder', 'groot', 'bos', 'vos', 'peters', 'hendriks', 'leeuwen', 'dekker', 'brouwer', 'wit', 'dijkstra',
              'smits', 'graaf', 'meer', 'linden', 'kok', 'jacobs', 'haan', 'vermeulen', 'heuvel', 'veen', 'broek',
              'bruijn', 'bruin', 'heijden', 'schouten', 'beek', 'willems', 'vliet', 'ven', 'hoekstra', 'maas',
              'verhoeven', 'koster', 'dam', 'wal', 'prins', 'blom', 'huisman']


def generate_names(amount: int) -> list[str]:
    return [f"{random.choice(first_names)} {random.choice(last_names)}" for _ in range(amount)]


model = Model(generate_names(150))
view = View()
presenter = Presenter(model, view)
presenter.start()

\$\endgroup\$
3
  • \$\begingroup\$ Please note we have a fairly strict policy on content generated by generative artificial intelligence tools. \$\endgroup\$
    – Mast
    Feb 25 at 12:39
  • \$\begingroup\$ @Mast Does this also include generating a few docstrings, as is the case here? That seems a bit rigid, but if that's the policy, then so be it. \$\endgroup\$ Feb 25 at 19:33
  • \$\begingroup\$ It does. To quote: "Not in questions, not in answers, not in code, not in comments. Not anywhere." \$\endgroup\$
    – Mast
    Feb 25 at 19:37

1 Answer 1

3
\$\begingroup\$

Has the Model-View-Presenter been implemented well? Are the responsibilities divided up as they should? I decided to link the view with the presenter through a simple callback method, is this a good idea?

Your implementation looks closer to MVC (Model-View-Controller) than MVP. The main problem of MVC is that the Controller (called Presenter in your code) does too much: it works with the Model handling incoming requests and it tells the View what to display. In MVP the latter is done in the View, thus freeing the Presenter from this responsibility. You can tell there is a problem by comparing the size of the View class with the Presenter. Here is a great read on the topic. It's about Android but you can draw parallels.

Are the classes structured in a logical way and are the methods readable? I generally try to adhere to the Single Responsibility Principle, and find it relatively intuitive for functions, but find it harder for classes. Main area of concern is the Presenter class: I've already split out the logic for formatting the player table, but the class is still quite large. Is this a problem?

Let's elaborate on the previous paragraph. Your View class is very abstract: it doesn't know anything about what is to be displayed. This abstraction would make sense if you're planning another way of displaying things, e.g. a GUI. In this case you can turn this logic into a dependency of the main View class: interface UI with 2 implementations - existing CLI that prints lines into the console and a hypothetical GUI that draws them on the screen. View, which is responsible for constructing strings from the information provided by the Presenter, calls to the UI when it needs to display something.

Method names are all very good.

I'm still rather inexperienced at exception handling, has it been dealt with correctly?

It was done very well, however there is one problem:

def _strip_command(self, user_input: str) -> str | None:
    """Removes the command from the user's input, leaving only the argument."""
    try:
        _, argument = user_input.split(maxsplit=1)
        return argument
    except ValueError:
        return

Here you use exceptions to differentiate between functions that take one parameter and those that zero. Never use exceptions for handling normal logic, they are for cases when something doesn't go according to plan. Every time I use the "help" command this exception will fire and you have to use a stub (user_input=None) in the signature of _give_help in order for things to work properly. If you ever need a function that takes 2 parameters, this entire logic will have to be rewritten. The right thing would be to pass the number of expected arguments as a parameter to _strip_command and get rid of exception handling in the method (it will be guaranteed to succeed because of the regexp).

How detailed should the docstrings be? I used ChatGPT to generate some of the docstrings as I find this difficult to do, and decided to use ChatGPT's more detailed docstrings for the main methods, and wrote my own less detailed docstrings for the "sub-methods". Is this the right way to go?

If the class / function is not a public API (other developers won't be using it), don't bother at all. Your code should speak for itself. Otherwise it's important that the following things are clear:

  • what the function / class is for?
  • does the function have side-effects?
  • what does the function return?
  • what does the function expect to be passed in its parameters? If the purpose is unclear without a comment, change the name to describe it better.

Random nitpicks:

True if self._player_names[index].startswith(search) else False can be replaced with just self._player_names[index].startswith(search)

def generate_names(amount: int) -> list[str]:
    return [f"{random.choice(first_names)} {random.choice(last_names)}" for _ in range(amount)]

This can produce duplicate names.

player_names: Iterable Iterable of what? If you're using typehints, use them to the fullest, otherwise it seems like this player_names can be filled with anything.

self.table_formatter = TableFormatter() This dependency should be injected along with the others, not created inside the dependant. You are using DI everywhere else in the code, so I assume it's just a mistake.


Extremely good code for a beginner, keep it up!


Upd: here is a before / after scheme for a potential refactoring: enter image description here

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your excellent answer! I've read the article, but am a bit at a loss how to refactor my project to truly make it MVP (for example: does all the formatting logic go to the view? Does the view keep state about current page number and current table/status? etc.) Could you give a few pointers as how you would go about refactoring this project? \$\endgroup\$ Jun 20, 2023 at 10:08
  • 1
    \$\begingroup\$ @JanvanWijk yes, all the formatting goes into the View. No, the view doesn't keep any state, only Model does. I've added a before/after diagram of the architecture to the answer. I would start by renaming current View into UI and creating a new class called View with a dependency on UI and Presenter. When the user does something, View tells the Presenter to handle the input. \$\endgroup\$ Jun 20, 2023 at 12:00

Not the answer you're looking for? Browse other questions tagged or ask your own question.