8
\$\begingroup\$

I created a simple text-based Tic Tac Toe game in Python using OOP. Currently the computer moves are completely random - I plan to add some sort of algorithm later (no idea how to do that yet though)

Any suggestions about how to improve it are welcome.

from random import randint
from itertools import cycle


class Board:
    def __init__(self):
        self._board = [["-"]*3 for i in range(3)]

    def display(self):
        for row in self._board:
            for tile in row:
                if tile != "-":
                    tile = tile.symbol
                print(tile, end=" ")
            print()

    def place_symbol(self, player, tile):
        """Try to place the player inside the tile
        The important thing here is that it returns None if it fails
        """
        row, colmn = tile
        if self._board[row][colmn] == "-":
            self._board[row][colmn] = player
            return True

    def check_win(self):
        """Checks all possible winning combinations,
        Returns True for a win and False otherwise.
        """
        # Store all checks here
        checks = set()

        # Add rows
        for row in self._board:
            checks.add(tuple(row))

        # Add columns
        colmns = zip(self._board[0], self._board[1], self._board[2])
        for colmn in colmns:
            checks.add(tuple(colmn))

        # Add diagonals
        diag1 = (self._board[0][0], self._board[1][1], self._board[2][2])
        diag2 = (self._board[0][2], self._board[1][1], self._board[2][0])
        checks.update((diag1, diag2))

        # Check every option for a win
        checks = {True if (len(set(lst)) == 1 and lst[0] != "-") else False for lst in checks}
        if True in checks:
            return True
        return False

    def is_full(self):
        if "-" not in (self._board[0]+self._board[1]+self._board[2]):
            return True
        return False


class Player:
    def __init__(self, is_human, symbol, name):
        self.is_human = is_human
        self.symbol = symbol
        self.name = name
        self.score = 0

def get_player_input(choices, text=''):
    while True:
        inpt = input(text)
        if inpt in choices:
            return inpt
        print(f"Enter one of the following: {', '.join(choices)}")


def main():
    print("Welcome to tic tac toe!")
    print("type the appropiate number to choose a game option:")
    print("1.player vs player\n2.player vs computer\n3.computer vs computer")
    choice = get_player_input(('1', '2', '3'),)
    if choice == '1':
        player1_name = input("Choose a Name for player 1: ")
        player2_name = input("Choose a Name for player 2: ")
        player1_is_human = True
        player2_is_human = True
    elif choice == '2':
        player1_name = input("Choose a name: ")
        player2_name = "Computer"
        player1_is_human = True
        player2_is_human = False
    elif choice == '3':
        player1_name = "Computer 1"
        player2_name = "Computer 2"
        player1_is_human = False
        player2_is_human = False

    player1 = Player(player1_is_human, "X", player1_name)
    player2 = Player(player2_is_human, "O", player2_name)
    players = [player1, player2]
    board = Board()
    # For player row and colmn input
    options = ('1', '2', '3')

    for player in cycle(players):
        board.display()
        print(f"It's {player.name}'s turn")

        # The actual turn of the player
        while True:
            if player.is_human:
                row = int(get_player_input(options, "Enter row number(1-3): ")) - 1
                colmn = int(get_player_input(options, "Enter column number(1-3): ")) - 1
            else:
                row, colmn = randint(0, 2), randint(0, 2)

            result = board.place_symbol(player, (row, colmn))
            if result is None:
                if player.is_human:
                    print("Enter in a non-full tile")
                continue
            else:
                break

        win = board.check_win()
        if win or board.is_full():
            board.display()
            if win:
                print(f"player {player.name} won")
                player.score += 1
                print(f"current scores:\nPlayer {players[0].name}: {players[0].score}")
                print(f"Player {players[1].name}: {players[1].score}")
            elif board.is_full():
                print("It's a draw!")

            again = input("another game?(y/n)")
            if again == "y":
                board = Board()
                continue
            return


if __name__ == '__main__':
    main()
\$\endgroup\$
  • \$\begingroup\$ Is this python 3.9+? or plan to go for 3.9+? \$\endgroup\$ – hjpotter92 Jul 31 at 15:41
  • \$\begingroup\$ It's in python 3.8.5, and I don't plan to update it to python 3.9. \$\endgroup\$ – Nadi726 Aug 1 at 20:13
5
\$\begingroup\$

Welcome to Code Review!

  1. Having a class for player seems over complicated. A simple namedtuple would be enough.

  2. The main() function is doing most of the heavy lifting. You can have a Game class, which takes the players list (or individual objects) as init parameters, and then implements the game logic.

  3. You can reuse the get_player_input when asking for another game from the user.

  4. When working with random library, it is generally a good practice to seed it at the beginning.

  5. The Board.display cam be made a 1-liner:

     print("\n".join(" ".join(row) for row in self._board))
    
  6. Instead of having a Board.display method, override __str__, and simply print(board).

  7. Alternative implementation of is_full:

     def is_full(self):
         return "-" not in set(chain(*self._board))
    

    where chain is from itertools.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You have some great points, thanks! However, there's a problem with your 5th one: first, I assume you meant row instead of x. Second, it raises an error once there's a player object on the row. I managed to fix it by modifying it to '\n'.join(' '.join([str(tile) for tile in row]) for row in self._board) and overriding str for Player \$\endgroup\$ – Nadi726 Aug 1 at 20:38
  • \$\begingroup\$ Also... I'm not sure I understand the 2nd one completely. if the problem is main() doing most of the heavy lifting, wouldn't it be simpler to just have a game function instead of class? \$\endgroup\$ – Nadi726 Aug 1 at 20:41
  • \$\begingroup\$ @Nadi726 yes. it was supposed to be row. edited now. There is not an issue with having the whole functionality in main(); however, if you plan to have multiple games running in parallel (for stats/multiplayer for eg.) it would become hectic to run the program n-different times. With a Game class, you can maintain multiple independent copies, scores, player information etc. as its own entity. Later, you may for eg. store game results in a centralised db. Just a suggestion for futuristic expansion. Not needed if the console level is its intended use. \$\endgroup\$ – hjpotter92 Aug 1 at 21:45
  • \$\begingroup\$ Even with row instead of x, your code still fails if there's a Player object in the row, as I mentioned in my comment - you can try to run it yourself. And I think I can see your point about main, although I'm not sure about how to implement the Game class without rewriting a lot of the code, but I'll try. \$\endgroup\$ – Nadi726 Aug 1 at 22:04
  • \$\begingroup\$ @Nadi726 Board should not be dealing with player objects at all. When placing a symbol, you only place the symbol. The Game manager will handle which player's symbol was it. Players can also choose their own symbols if needed. \$\endgroup\$ – hjpotter92 Aug 1 at 22:26
3
\$\begingroup\$

Welcome to CodeReview!

You have missed an OO-opportunity.

You have a class Player but you are still "switching on internal data". You do this:

def get_player_input(choices, text=''):
    ...

which is not a method on Player. And later, you do this:

if player.is_human:
    row = int(get_player_input(options, "Enter row number(1-3): ")) - 1
    colmn = int(get_player_input(options, "Enter column number(1-3): ")) - 1
else:
    row, colmn = randint(0, 2), randint(0, 2)

This act of writing if player.is_human: ... else: ... is "switching on internal data". It's "internal data" because you aren't getting it from outside the class. It's "switching" because you are making an exclusive choice.

Switching on internal data is a "code smell" that indicates you might need a new class. In this case, I think you do:

from abc import ABC, abstractmethod

class Player(ABC):
    @abstractmethod
    def get_next_move(self, board: Board) -> Position:
        ...

class PlayerIO(Player):
    def get_next_move(self, board: Board) -> Position:
        """ Read next move from io streams """
        pass

class PlayerRandom(Player):
    def get_next_move(self, board: Board) -> Position:
        """ Randomly generate next move """
        pass

I'll suggest that the IO constructor takes input and output streams, and handles displaying the board and prompting for a new move.

I'll also suggest that you write a TextIO class of some kind, and give it methods like "prompt for input" and "read a string" and "read an integer". This basic set of operations can be the building blocks for your PlayerIO class, and will make it possible to create a mock object for unit testing.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

In check_win:

checks = {True if (len(set(lst)) == 1 and lst[0] != "-") else False for lst in checks}
if True in checks:
    return True
return False

can be rewritten more explicitly as:

return any(len(set(lst)) == 1 and lst[0] != "-" for lst in checks)
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This also has the benefit that any is lazy so it'll stop checking once finding the first True. \$\endgroup\$ – Peilonrayz Aug 2 at 5:32
1
\$\begingroup\$

This looks good and well thought through.

The continue, break and return statements are always a bit tricky, it might be useful to add a comment to them to explain what they continue/break/return from.

Theoretically we could be in an endless loop if the computer player never finds a non-full tile.... , but the AI algorithm you planned will fix that :-)

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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