5
\$\begingroup\$

Just looking for a review on my code, its currently only player vs player, but I'll be working on the AI soon enough. Functions edit_name, tutorial, and AI have been left out as I haven't started work on them.

class tic_tac_toe:
    board = [" "," "," ",
             " "," "," ",
             " "," "," ",]

    players = [["One", "O", True],["Two", "X", False]]
    winning_combos = [[0,1,2],  # Horizontal
                      [3,4,5],
                      [6,7,8],
                      [0,3,6],  # Vertical
                      [1,4,7],
                      [2,5,8],
                      [0,4,8],  # Diagonal
                      [2,4,6]]
    grid_size = 3

    def __init__(self):
        pass

    def edit_name(self):
        pass

    def occupied(self, position):
        if self.board[self.translate_position(self.grid_size, position)] == " ":
            return False
        else:
            return True


    # Swaps the players turns
    def swap_turns(self):
        # if player 1 is turn is false
        if self.players[0][2]:
            self.players[0][2] = False
            self.players[1][2] = True

        else:
            self.players[0][2] = True
            self.players[1][2] = False

    # Who's turn is it?
    def players_turn(self):
        if self.players[0][2]: return 0
        if self.players[1][2]: return 1

    # Update the board with new plays
    def update_board(self, position, player):
        if not self.occupied(position):
            self.board[
                self.translate_position(self.grid_size, position)
            ] = self.players[player][1]
        else:
            print("You cannot over lap plays!")
            self.main()

    # Translates [x,y] -> 0-8
    @staticmethod
    def translate_position(grid_size, position):
        # y = (x_1 - 1) + n(x_2 - 1)
        # 8 = [3,3][x_1,x_2] where the grid size(n) is 3
        x1 = int(position[0])
        x2 = int(position[1])
        grid = int(grid_size)
        return (x1 - 1) + grid * (x2 - 1)

    # Displays the current board
    def show_board(self):
        print("/-----------------\\ \n"
            "|  {0}  |  {1}  |  {2}  |\n"
            "|-----------------|\n"
            "|  {3}  |  {4}  |  {5}  |\n"
            "|-----------------|\n"
            "|  {6}  |  {7}  |  {8}  |\n"
            "\-----------------/".format(self.board[0], self.board[1], self.board[2],
                                     self.board[3], self.board[4], self.board[5],
                                     self.board[6], self.board[7], self.board[8]))

    # Checks if there is a winner
    @property
    def winner(self):
        for player in range(0,len(self.players)):
            for winning_combo in range(0,len(self.winning_combos)):
                if self.players[player][1] in [self.board[self.winning_combos[winning_combo][0]]]:
                    if self.players[player][1] in [self.board[self.winning_combos[winning_combo][1]]]:
                        if self.players[player][1] in [self.board[self.winning_combos[winning_combo][2]]]:
                            return True

        return False

    def main(self):
        while not self.winner:
            player = self.players_turn()

            print("Player {0}'s turn".format(self.players[player][0]))
            play = []

            x = input("Input the x-coordinate where you would like to place your piece: ")
            y = input("Input the y-coordinate where you would like to place your piece: ")

            try:
                play.append(int(x))
                play.append(int(y))
            except:
                print("Fill in Both values!")
                self.main()

            self.update_board(play, player)
            self.show_board()

            self.swap_turns()

        z = self.players_turn()

        if z == 0: name = self.players[1][0]
        if z == 1: name = self.players[0][0]

        print("player {0} wins!".format(name))

    def tutorial(self):
        pass

    def AI(self):
        pass


if __name__ == "__main__":
    tic_tac_toe = tic_tac_toe()
    tic_tac_toe.main()
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

The first thing I would suggest is to do some input validation. Here's some of my input:

Player One's turn Input the x-coordinate where you would like to place
your piece: 1, 3 Input the y-coordinate where you would like to place
your piece: 4 Fill in Both values!

Player Two's turn Input the x-coordinate where you would like to place
your piece: 7 Input the y-coordinate where you would like to place
your piece: 5

Traceback (most recent call last):
    ... IndexError: list index out of range

You do some input validation, but it doesn't quite account for everything you need, and the error message is not clear or comprehensive. Instead of accepting both answers and then just trying to turn them into ints, you should also check if they fit inside the boundaries of the game grid. You should also abstract it into a function rather than putting it inside main.

def get_input():

    max_value = len(self.board)
    while True:
        try:
            x = int(input("Input the x-coordinate where you would like to place your piece: "))
            y = int(input("Input the y-coordinate where you would like to place your piece: "))
        except ValueError:
            print("Both values must be numbers.")
            continue

        if 0 < x <= max_value and 0 < y <= max_value:
            return [x, y]

        print("Both values must be numbers from 1-{}".format(max_value))

You also have a bug in your win detection, this was my end game:

/-----------------\
|  X  |  O  |  X  |
|-----------------|
|  O  |  X  |  X  |
|-----------------|
|  O  |  O  |  O  |
\-----------------/
player One wins!
/-----------------\
|  X  |  O  |  X  |
|-----------------|
|  O  |  X  |  X  |
|-----------------|
|  O  |  O  |  O  |
\-----------------/
player Two wins!

I can't find this bug though, because you have so many overlapping and interacting functions I can't follow the game flow. For instance, this function:

def players_turn(self):
    if self.players[0][2]: return 0
    if self.players[1][2]: return 1

This is very strange and confusing. Why not just have a value that's current_player which holds a reference to the player. Python's variables are just references, so if you were to do self.current_player = self.players[0] then you're not making a copy, you're just telling Python that self.current_player and self.players[0] both refer to the same thing. Even aside from that, abstracting it into a small unclear function like this is actually counter productive.

Functions shouldn't just abstract things for the sake of only doing one job each. The actual intent is to reduce duplicate code and to make code easier to read and follow. In a sense players_turn makes it easier to check what the turn is, but in reality it masks a fragile data structure. What if something happens that edits a players info? Your function would quickly fall apart. It's also not useful for a user to get the index of the player rather than the player object itself, particularly since the index is not the same as the "Player 1" string that is used for printing.

\$\endgroup\$

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