3
\$\begingroup\$

I programmed the tic-tac-toe game in Python using object oriented programming and I wanted to share it here to get some feedback

class TicTacToe:
    def __init__(self):
        self.board = [" "] * 9
        self.spacer = "-" * 50

    @property
    def _select_letter(self):
        computer_side = None
        player_side = None

        while True:
            player_side = input("Choose your side: ").upper()

            if player_side not in ("X", "O"):
                print("You can only choose X or O.")
            else:
                break

        computer_side = "O" if player_side == "X" else "X"
        print(f"The computer has chosen to be {computer_side}.")
        print(f"You will be {player_side}.")
        return player_side, computer_side

    @property
    def _human_goes_first(self):
        go_first = None
        while go_first not in ("y", "n"):
            go_first = input(("Do you require the first move? (y/n): ")).lower()
        return go_first == "y"

    @property
    def _print_board(self):
        return f"{self.board[0]}|{self.board[1]}|{self.board[2]}\n-+-+-\n{self.board[3]}|{self.board[4]}|{self.board[5]}\n-+-+-\n{self.board[6]}|{self.board[7]}|{self.board[8]}"
    
    @property
    def _board_full(self):
        return not " " in self.board

    def _check_win(self, letter):
        win_combs = [letter, letter, letter]
        return self.board[:3] == win_combs or self.board[3:6] == win_combs or self.board[6:] == win_combs or \
               self.board[:7:3] == win_combs or self.board[1:8:3] == win_combs or self.board[2::3] == win_combs or \
               self.board[::4] == win_combs or self.board[2:7:2] == win_combs

    def _computer_move(self, player_letter, computer_letter):
        corners = (2, 4, 6, 8)
        # First, check if we can win in the next move.
        for i in range(8):
            if self.board[i] == " ":
                if self._check_win(computer_letter):
                    self.board[i] = computer_letter
                    return

        # Check if the player could win on their next move, and block them.
        for i in range(8):
            if self.board[i] == " ":
                if self._check_win(player_letter):
                    self.board[i] = computer_letter
                    return

        # Try to take one of the corners, if they are free.
        for i in corners:
            if self.board[i] == " ":
                self.board[i] = computer_letter
                return

        # Try to take the center, if it is free.
        if self.board[4] == " ":
            self.board[4] = computer_letter
            return

    def _player_move(self, letter):
        while True:
            try:
                move = int(input(f"It's your turn {letter}. Enter a number between 1-9: ")) - 1
                if not 0 <= move <= 8:
                    print("The number entered is invalid. Enter a number between 1 and 9.")
                elif self.board[move] != " ":
                    print("This place is already occupied.")
                    print(f"\n{self.spacer}\n")
                else:
                    self.board[move] = letter
                    break
            except ValueError:
                print(f"Enter a number!\n\n{self.spacer}\n")

    def play(self):
        print("Welcome to Tic Tac Toe!")
        human, computer = self._select_letter
        first_player = self._human_goes_first
        print(f"\n{self.spacer}")

        while not self._board_full:
            if first_player:
                print(f"\nNow playing: Human ({human})")
                self._player_move(human)
            else:
                print(f"\nNow playing: Computer ({computer})")
                self._computer_move(human, computer)
            first_player = False if first_player else True
            print(f"\n{self._print_board}")
            print(f"\n{self.spacer}")

            if self._check_win(human):           
                print("\nYou won!")
                print("Thanks for playing!")
                break
            elif self._check_win(computer):
                print("\nComputer won!")
                print("Thanks for playing!")
                break
        else:
            print("\nNobody won, it's a tie.")


if __name__ == '__main__':
    game = TicTacToe()
    game.play()
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Don't scatter magic values everywhere. Values like 'X', 'O', or ' '. Instead, define named constants like X, O, and EMPTY.

Smarter data, simpler code. Your current _check_win() method is repetitive and laid out with long lines are are not easy to scan visually.

# I suppose this is correct ... but I don't want to think so hard.

def _check_win(self, letter):
    win_combs = [letter, letter, letter]
    return self.board[:3] == win_combs or self.board[3:6] == win_combs or self.board[6:] == win_combs or \
           self.board[:7:3] == win_combs or self.board[1:8:3] == win_combs or self.board[2::3] == win_combs or \
           self.board[::4] == win_combs or self.board[2:7:2] == win_combs

To eliminate the repetition, define a data structure of slices -- and lay it out nicely so that the intent and the correctness are obvious at a quick glance.

WIN_SLICES = (
    # Rows.
    slice(0, 3, 1),
    slice(3, 6, 1),
    slice(6, 9, 1),
    # Columns.
    slice(0, 7, 3),
    slice(1, 8, 3),
    slice(2, 9, 3),
    # Diagonals.
    slice(0, 9, 4),
    slice(2, 7, 2),
)

def _check_win(self, letter):
    return any(
        self.board[s] == [letter, letter, letter]
        for s in self.WIN_SLICES
    )

The logic in play() is unnecessarily complicated. You have some duplicated code to handle the difference between human and computer. But that duplication can be eliminated if you think about the problem in terms of data. Human and computer differ in these ways: their labels ("Human" vs "Computer"), their letters, and the methods to be called. Put that data in some kind of structure (something as simple as a pair of tuples would work) and then just toggle between the tuples with each iteration of the while-loop. You could also simplify things quite a bit by changing _check_win() so that it always checks for a win by either player and just returns the winner or None. You have the ability to figure this out. What's missing right now is your instinct to notice the annoying code duplication and determine to do something about it. Thinking in terms of data is often the key to these kinds of issues.

The computer move logic has bugs. Your _computer_move() method isn't doing what you think it's doing. It claims to be looking into the future (for example, one comment says "Check if the player could win on their next move, and block them"). However, the logic is confined to making calls to _check_win() and that method does not examine the future; it examines only the current board state. In order to make this type of forward look you would need to temporarily fill the empty spot, then call _check_win(), and then undo the temporary move (or not) depending on the result. To see this issue (and a separate bug) in action, play as X, move first, and enter the following choices: 7, 5, 6. Notice that on the next computer move ... the computer does nothing. Not only does it fail to block, it fails to play anything at all. Rather than going into the details, I think I'd rather leave fixing such bugs in your hands. You are writing reasonable code and can clearly figure these things out on your own.

A different OO vision: your TicTacToe class is doing too much, at least for my tastes. While learning, it's understandable that you want to experiment with object-oriented programming. I would encourage you to use OO in a more fine-grained manner. Consider your current TicTacToe class. It has only two attributes (board and spacer), and that latter is more properly understood as a constant. In other words, your current class is masquerading as a class that knows how to orchestrate the playing of tic-tac-toe, but in reality it's just a vehicle to store a tic-tac-toe board. And you know what? That would be a better design decision: a Board class that keeps track of the current board and can answer simple question about it. Pull the game orchestration out of that class and put it somewhere else -- either in ordinary functions or in other classes. We can catch a glimpse of why this decomposition might be better by considering _computer_move() again. In spite of its bugs, that method is interesting because it represents a game-playing strategy. Conceivably, you might want to implement multiple strategies and pit them against each other in a tournament. And even if you don't want to go that far, the human player is also a game-playing strategy: one that gets the next move via input(). Under your current design -- where everything is combined under the umbrella of a big class -- it's not easy to experiment with different strategy implementations. Under a more fine-grained OO approach, your finished program might have different classes focused narrowly on specific aspects of the problem: board state; player strategy; overall game orchestration; and so forth. Admittedly, that's a lot to consider for something as simple as tic-tac-toe, but if you're trying to learn how to build more complex programs, these are the kinds of questions you want to be thinking about.

\$\endgroup\$
1

Your Answer

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

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