4
\$\begingroup\$

I just joined here. I have been doing python for a couple of weeks and I would like for some experienced programmers to review my code. I made this for a school project and I am proud of it. The code below works well but I would like to know what things I can improve on. Thank you for taking the time to review this.

main.py

main_grid = [['+' for col in range(3)] for row in range(3)]
player_one = {'key': 'P1', 'name': None, 'symbol': 'X', 'pos': []}
player_two = {'key': 'P2', 'name': None, 'symbol': 'O', 'pos': []}
player_list = [player_one, player_two]


def title_screen():
    print(' '*15, ' _____ _____ _____')
    print(' '*15, '|__ __|__ __| ____|')
    print(' '*15, '  | | |_| |_| |___')
    print(' '*15, ' _|_|_|_____|_____|')
    print(' '*15, '|__ __| ___ | ____|')
    print(' '*15, '  | | | | | | |___ ')
    print(' '*15, '__|_|_|_|_|_|_____|')
    print(' '*15, '|__ __| ___ | ____|')
    print(' '*15, '  | | | |_| | ____|')
    print(' '*15, '  |_| |_____|_____|')


def info_screen():
    print('-'*51)
    print('2021 L.A | A python school project. ')
    print()
    print("This game is a python version of 'TIC TAC TOE'.")
    print('I hope you enjoy! :]')
    print()


def intro_screen():
    print('RULES:')
    print('Rows and columns can only have inputs from [0-2].')
    print('Players can only enter value once unless value is')
    print('not valid.')
    print("Players can't put their symbol in already occupied")
    print('cells.')
    print('Players win if their symbol is placed 3 in a row')
    print('horizontally, vertically or diagonally. Anything')
    print('else does not count as a win.')
    print('If the grid is filled, the game results as a tie.')
    print()


def start_screen():
    title_screen()
    info_screen()
    intro_screen()


def get_name():
    for player_type in player_list:
        print(f"Enter [{player_type['key']}] name:")
        player_type['name'] = input(f"[{player_type['key']}] > ")
        print()


def get_input(player_type, desc):
    print(desc)
    user_input = input(f"[{player_type['key']}] {player_type['name']} > ")
    print()
    return user_input


def get_point(player_type, desc):
    """Makes user get a point value which could be a row or col."""
    point = -1
    while point not in range(3):
        try:
            point = int(get_input(player_type, desc))
        except ValueError:
            print('The entered value must be an integer.')
            print('Please try again.')
            print()
            continue
        if point in range(3):
            return point
        elif point not in range(3):
            print('The entered value must be in range between 0-2.')
            print('Please try again.')
            print()
            continue
        else:
            print('Unknown error')
            print('Please try again.')
            print()
            continue


def check_pos(pos):
    """Check whether the pos value is in a player's coords list."""
    if pos in player_one['pos'] or pos in player_two['pos']:
        print('The entered coordinate is occupied by a player.')
        print('Please try again.')
        print()
        return False
    else:
        return True


def update_grid(pos):
    """Updates grid using player coords."""
    for player_type in player_list:
        for prev_pos in player_type['pos']:
            main_grid[prev_pos[0]][prev_pos[1]] = player_type['symbol']


def print_stats(player_type, turn_count):
    """Prints current game stats."""
    print('-'*51)
    print(f"CURRENT TURNS  | {turn_count}/9")
    print(f"CURRENT PLAYER | {player_type['key']}")
    print(f"CURRENT SYMBOL | {player_type['symbol']}")


def print_grid():
    """Prints the grid"""
    print()
    for row in range(3):
        print(' '*21, end='')
        for col in range(3):
            print(main_grid[row][col], end='   ')
        print()
        print()


def check_grid(player_type, pos):
    """End game when symbol is three in a row or grid is filled. Continues if not three in a row or not filled."""
    row = pos[0]
    col = pos[1]
    symbol = player_type['symbol']
    win_msg = f"[{player_type['key']}] {player_type['name']} has won the game."
    tie_msg = 'The game has resulted a tie.'
    if symbol == main_grid[row][0] == main_grid[row][1] == main_grid[row][2]:
        print_result(win_msg)
        return True
    elif symbol == main_grid[0][col] == main_grid[1][col] == main_grid[2][col]:
        print_result(win_msg)
        return True
    elif row == col and symbol == main_grid[0][0] == main_grid[1][1] == main_grid[2][2]:
        print_result(win_msg)
        return True
    elif row + col == 2 and symbol == main_grid[0][2] == main_grid[1][1] == main_grid[2][0]:
        print_result(win_msg)
        return True
    elif '+' not in main_grid[0] and '+' not in main_grid[1] and '+' not in main_grid[2]:
        print_result(tie_msg)
        return True
    else:
        return False


def print_result(msg):
    """Prints end result of game"""
    print('-'*51)
    print('RESULT:')
    print_grid()
    print(msg)
    exit_result()


def setup_pos(player_type):
    """Sets up position value

    Gets each point value.
    Checks each point value
    Checks position value
    Adds position value to player dictionary
    
    """
    valid_pos = False
    while valid_pos is False:
        row = get_point(player_type, 'Select a row:')
        col = get_point(player_type, 'Select a column:')
        new_pos = (row, col)
        valid_pos = check_pos(new_pos)
        if valid_pos is True:
            player_type['pos'].append(new_pos)
            return new_pos


def exit_result():
    """Choice to exit result screen."""
    print()
    print(f"Press any key to exit.")
    user_input = input('> ')
    exit(0)


def setup_game():
    turn_count = 1
    game_finish = False
    while game_finish is False:
        for player_type in player_list:
            print_stats(player_type, turn_count)
            print_grid()
            new_pos = setup_pos(player_type)
            update_grid(new_pos)
            game_finish = check_grid(player_type, new_pos)
            turn_count += 1
            if game_finish is True: # If player won, break for loop on player's turn.
                break


def main():
    start_screen()
    get_name()
    setup_game()


if __name__ == '__main__':
    main()
\$\endgroup\$
1
  • 2
    \$\begingroup\$ This is really good for someone just starting out! Keep it up! \$\endgroup\$ Nov 21, 2021 at 9:56

2 Answers 2

1
\$\begingroup\$

So this is a great start for someone just starting out. I will go off on a huge tangent here, but hopefully you can take something away from how I approached improving a very minor part of your code.

Player states

class Player:
    id: int
    name: str = ""
    symbol: str = ""
    is_winner: bool = False

Now we can build the list of players as follows

player_list = [Player(1, symbol="X"), Player(2, symbol="O")]

Where did pos go? It would be better (and clearer) if the gamestate was handled globally and not as a property of the players. It makes sense that the players are part of the game, not that the game is part of the players.

There is no reason why your players could not be implemented as classes

Printing

The line print("-"*51) can be improved in a few ways. 51 (and in general all non specified numbers in code) are known colloquially as magic-numbers and is an anti-pattern. Where does the number come from? Why is it not 52 or 50? We can fix this by defining it as a constant

BLANKLINE = "-" * 51

As a sidenote notice that we wrote * and not *, this is because Python has style guide that specifies that operators need a little breathing roomThis is a lie. If you use Black or another formater it fixes these things for you.

info_screen can now look like

def info_screen() -> str:
    """Information about the game"""

    return f"""{BLANKLINE}
    
    2021 L.A | A python school project. "
    This game is a python version of 'TIC TAC TOE'.
    "I hope you enjoy! :]"
    """

Where we have added a very short docstring and typing hints. Perhaps more importantly we have split generating the information and printing the information. While now there would be nothing wrong with printing in the info_screen file, what if we wanted to append text to it later?

Overall principles

Turing this

def setup_pos(player_type):
    valid_pos = False
    while valid_pos is False:
        row = get_point(player_type, "Select a row:")
        col = get_point(player_type, "Select a column:")
        new_pos = (row, col)
        valid_pos = check_pos(new_pos)
        if valid_pos is True:
            player_type["pos"].append(new_pos)
            return new_pos

into this

def get_pos(player_type):
    row = get_point(player_type, "Select a row:")
    col = get_point(player_type, "Select a column:")
    return (row, col)

def setup_pos(player_type):
    while not valid_pos(pos := get_pos(player_type)):
        pass
    player_type["pos"].append(pos)
    return pos

Where check_pos was renamed valid_pos.

  • The player_type["pos"].append(pos) in the code above is another antipattern. Notice how player_type is a global function, we try to avoid those as best as we can. Limiting the scope of functions and variables makes it much easier to squash bugs! Again the proper solution is classes =)

Overall principles

I will leave the bullet points above for you to implement and adapt. Instead I will focus on how I would "improve" (totally over-engineer).

Notice how I would never do the improvements I do here in production

If you can (and you should) find libraries that can handle the pretty printing to the command line. However, just for learning. Let us see how we can implement something simple

def title_screen():
    print(' '*15, ' _____ _____ _____')
    print(' '*15, '|__ __|__ __| ____|')
    print(' '*15, '  | | |_| |_| |___')
    print(' '*15, ' _|_|_|_____|_____|')
    print(' '*15, '|__ __| ___ | ____|')
    print(' '*15, '  | | | | | | |___ ')
    print(' '*15, '__|_|_|_|_|_|_____|')
    print(' '*15, '|__ __| ___ | ____|')
    print(' '*15, '  | | | |_| | ____|')
    print(' '*15, '  |_| |_____|_____|')
  • There is repetition as aforementioned.
  • While subjective I find the particular ASCII font hard to read. Particularly an issue is that the lines are not connected.

An improvement would be something like this

enter image description here

So how do we get there?

  1. We split everything to do with generating a fancy title into it's own file. At the end of the day we will use it as follows

    from title import Title, asciify, fullborder
    
    TIC_TAC_TITLE = Title(asciify("TIC TAC "TOE"), border=fullborder, width=79)
    
    
    def title_screen():
        print(TIC_TAC_TITLE)
    

Which is your main and with much imagination we named the new file "title.py".

It is getting quite late but I will just leave the code here with a few highlights

  • We split the generation of the fancy title into three parts

    • We need to generate the ascii letters
    • We need to make a border
    • We need to combine the letters and the borders into a title
  • The first problem is solved in the asciify file. Using a dictionary we have defined a few handful of letters.

  • The title and border can both naturally be expressed as a classes

  • We add typing hints, docstrings and doctests wherever necessary.

Note that for improving the rest of the code you would do exactly the same. You would try to split the code into separate parts, each part would go into it's own file. Here you would add classes as needed, add typing hints, doctests and then import it into your main file.

Code

"""Takes in a list of letters [A,C,E,O,T,I, ] and returns them as ascii letters
"""
from dataclasses import dataclass
from typing import Annotated

ASCII = {
    "A": """
┌─────┐
│ ┌─┐ │
│ ├─┤ │
└─┘ └─┘
""",
    "C": """
┌─────┐
│ ┌───┘
│ └───┐
└─────┘
""",
    "E": """
┌─────┐
│  ───┤
│  ───┤
└─────┘
""",
    "O": """
┌─────┐
│ ┌─┐ │
│ └─┘ │
└─────┘
""",
    "T": """
┌─────┐
└─┐ ┌─┘
  │ │ 
  └─┘  
""",
    "I": """
┌─────┐
└─┐ ┌─┘
┌─┘ └─┐
└─────┘
""",
    " ": """
 
  
  
""",
    "-": """

 ┌───┐ 
 └───┘ 
""",
}

PYTHON_MAX_CHAR_WIDTH = 79
AsciiLetters = Annotated[str, "A multiline string of big ascii letters"]
Length = Annotated[int, "How long a particular string should be"]


def asciify(letters: str, ascii_dict: dict = ASCII) -> AsciiLetters:
    """Turns a string into big ascii letters

    Args:
        letters: Optional. The word / collection of letters you want to asciify
        ascii_dict: A dictionary (mapping) between letters and their ascii representation

    Returns:
        The inputed letters in asccii form formated as a multiline string

    Example:
        >>> print(asciify('A'))
        ┌─────┐
        │ ┌─┐ │
        │ ├─┤ │
        └─┘ └─┘
        >>> print(asciify('TAC'))
        ┌─────┐┌─────┐┌─────┐
        └─┐ ┌─┘│ ┌─┐ ││ ┌───┘
          │ │  │ ├─┤ ││ └───┐
          └─┘  └─┘ └─┘└─────┘
        >>> print(asciify('I-E-A-I-A-I-O'))
        ┌─────┐       ┌─────┐       ┌─────┐       ┌─────┐       ┌─────┐       ┌─────┐       ┌─────┐
        └─┐ ┌─┘ ┌───┐ │  ───┤ ┌───┐ │ ┌─┐ │ ┌───┐ └─┐ ┌─┘ ┌───┐ │ ┌─┐ │ ┌───┐ └─┐ ┌─┘ ┌───┐ │ ┌─┐ │
        ┌─┘ └─┐ └───┘ │  ───┤ └───┘ │ ├─┤ │ └───┘ ┌─┘ └─┐ └───┘ │ ├─┤ │ └───┘ ┌─┘ └─┐ └───┘ │ └─┘ │
        └─────┘       └─────┘       └─┘ └─┘       └─────┘       └─┘ └─┘       └─────┘       └─────┘
    """
    ascii_letter_list = [ascii_dict[l] for l in list(letters)]
    lines = [letter.split("\n") for letter in ascii_letter_list]
    letter_widths = [max(map(len, line)) for line in lines]
    ascii_art = []

    for letterlines in zip(*lines):
        line = ""
        for i, letter in enumerate(letterlines):
            line = line + f"{letter:{letter_widths[i]}}"
        ascii_art.append(line)

    ascii_letters = "\n".join(ascii_art)
    return ascii_letters.strip()


def repeat_to_length(string: str, wanted: Length) -> str:
    """Repeats a string to the desired length"""
    a, b = divmod(wanted, len(string))
    return string * a + string[:b]


@dataclass
class Border:
    SW: str = ""
    SE: str = ""
    NE: str = ""
    NW: str = ""

    top: str = ""
    bottom: str = ""
    right: str = ""
    left: str = ""

    symmetric: bool = False

    def __post_init__(self):
        if not self.symmetric:
            return
        self.top, self.bottom = self.equal_values([self.top, self.bottom])
        self.left, self.right = self.equal_values([self.left, self.right])
        self.SW, self.SE, self.NE, self.NW = self.equal_values(
            [self.SW, self.SE, self.NE, self.NW]
        )

    def border_top(self, width):
        return self.horizontal(self.top, self.NW, self.NE, width)

    def border_bottom(self, width):
        return self.horizontal(self.bottom, self.SW, self.SE, width)

    @staticmethod
    def horizontal(symbol, left_corner, right_corner, width):
        if not symbol:
            return ""
        if width == 1:
            return left_corner
        main = repeat_to_length(symbol, width)
        return left_corner + main[:-2] + right_corner if len(main) >= 2 else main

    @staticmethod
    def equal_values(values):
        total_values = len(values)
        for value in values:
            if value is None:
                continue
            return [value for _ in range(total_values)]
        return [None for _ in range(total_values)]


@dataclass
class Title:
    text: str = ""
    border: Border = Border()
    center: bool = True
    width: int = 0

    def __post_init__(self):
        left, center = "<", "^"
        self.alignment = center if self.center else left

    def __str__(self):
        title = self.center_text() if self.center else self.text
        if self.border == Border():
            return title.rstrip()
        return self.add_border(text=title.rstrip())

    def center_text(self, width=None):
        width = width if width is not None else self.width
        textlines = self.text.split("\n")
        textwidth = (
            max(len(l) for l in textlines)
            + len(self.border.left)
            + len(self.border.right)
        )
        width = textwidth if not width else width
        lines = [f"{line:^{width}}" for line in textlines]
        minimum_indent = " " * min(map(lambda l: len(l) - len(l.lstrip()), lines))
        return "\n".join(minimum_indent + line for line in textlines)

    def add_border(self, text, width=None):
        """
        Adds a border to the self.text property of the Title class

        Args:
            width: Optional. If set the top and bottom border keeps this width.

        Returns:
            A title formated as a multiline string

        Example:
            >>> print(Title("A", border=Border(), center=True, width=3))
             A
            >>> print(Title("A", border=Border(top="=", SW="*", left="|", symmetric=True), center=True, width=3))
            *=*
            |A|
            *=*
            >>> print(Title("A", border=Border(top="=", SW="*", left="|", symmetric=True), center=True, width=5))
            *===*
            | A |
            *===*
            >>> print(Title("title", border=Border(bottom="="), center=True, width=79))
                                                 title
            =============================================================================
        """
        text = text if text is not None else self.text
        width = width if width is not None else self.width
        textlines = text.split("\n")
        textwidth = (
            max(len(l.strip()) for l in textlines)
            + len(self.border.left)
            + len(self.border.right)
        )
        width = textwidth if not width else width

        top_border = self.border.border_top(width)
        bottom_border = self.border.border_bottom(width)
        if len(top_border) < textwidth:
            if self.center:
                top_border = f"{top_border:^{textwidth}}".rstrip()
                bottom_border = f"{bottom_border:^{textwidth}}".rstrip()
            content_with_border = "" if not top_border else (top_border + "\n")
            content_with_border += text
            content_with_border += "" if not bottom_border else ("\n" + bottom_border)
            return content_with_border

        content_with_border_list = []
        start = 0 if not self.border.left else 1
        for line in textlines:
            post_indent = " " * (
                width
                - len(line)
                - (0 if not self.border.right else len(self.border.right))
            )
            content_with_border_list.append(
                (
                    self.border.left + line[start::] + post_indent + self.border.right
                ).rstrip()
            )
        content_with_border = "\n".join(content_with_border_list)
        return "\n".join([top_border, content_with_border, bottom_border]).rstrip()


starborder = Border(SW="*", top="=-", left="", symmetric=True)
fullborder = Border(
    SW="└", SE="┘", NE="┐", NW="┌", top="─", bottom="─", left="│", right="│"
)

if __name__ == "__main__":
    import doctest

    doctest.testmod()
\$\endgroup\$
1
  • \$\begingroup\$ "As a sidenote notice that we wrote * and not *" – I think the Stackexchange software gobbles any spaces that you put into the code markup. \$\endgroup\$
    – Schmuddi
    Nov 24, 2021 at 15:16
0
\$\begingroup\$

As a beginner, you have done fantastic!

There is always something to improve, in your code, mainly, performance.

  • print()

Instead of priniting again and again, create a string of output, and print it in a single call. It is way faster than printing in sperate calls.


Your code has doc-strings, follows pep8 and is overall very readable. You could also add type-hints to make it even more explicit.


In terms of user experience, it would be more comfortable to type 1-9 instead of both the row and column. You could manage your grid as a nine element list.

At the end, your project is awesome!

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I disagree with your print-aggregation suggestion - at this scale, performance differences will be invisible and legibility is more important. \$\endgroup\$
    – Reinderien
    Nov 24, 2021 at 2:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.