4
\$\begingroup\$

I have written a Tic-Tac-Toe game for two players, where each player uses the same keyboard in Python I'm happy with all kinds of feedback from this community, it is very much appreciated. I am wondering how I can make this code better in performance ? Since I am not up to classes yet, I only have functions

#PseudoCode

# Display welcome and instructions 
# Display board 
# Who gets X or O ?
# While no one has won and it isn't a tie loop 
    # get player's move X or O
    # Update the board with the new move 
    # and check legal move, so move that hasn't been taken
    #Switch Turns into X or O
#Congratulate Winner X or O or say it's a Tie

def display_welcome():
    """Display Welcome to Tic Tac Toe """
    print(
        """
        Hi and welcome to this Tic Tac Toe game_board.
        You can win by haveing three X or 3 O in a row.
        This can be horizontal, vertical or diagonal
    
       """
    )

def display_board(game_board):
    """display the board"""
    for x in range(3):
        print("+---+---+---+" )
        for y in range(3):
            print("|", game_board[x*3+y], end = " ")
        print("|")
    print("+---+---+---+" )

def determine_pieces():
    """Determine who gets X or O"""
    answer = None
    answer = input("Do you wish to start with X or O ").upper()
    while not answer in ['X','O']:
        answer = input("Sorry this needs to be X or O ").upper()
    return answer

def determine_winner(game_board):
    """check if there is a win, draw"""
    # check rows
    for i in range(3):
        if game_board[i*3] == game_board[i*3+1] == game_board[i*3+2] and game_board[i*3] in ('X', 'O'):
            return game_board[i*3]
    # check columns
    for i in range(3):
        if game_board[i] == game_board[i+3] == game_board[i+6] and game_board[i] in ('X', 'O'):
            return game_board[i]
    # check diagonals
    if game_board[0] == game_board[4] == game_board[8] and game_board[0] in ('X', 'O'):
        return game_board[0]
    if game_board[2] == game_board[4] == game_board[6] and game_board[2] in ('X', 'O'):
        return game_board[2]
    # check tie
    if all(x in ('X', 'O') for x in game_board):
        return 'Tie'
    # no winner or tie
    return False

def get_move(game_board):
    """ Get the player's move """
    move = int(input(" Please make a move between 1 and 9 "))
    while not legal_move(game_board,move):
        move = int(input( " That move is not legal try again "))
    return move

def legal_move(game_board,player_move):
    """Check if the move is legal"""
    legal = True
    if game_board[player_move -1] in ['X','O']:
        legal = False
    return legal
    
    
def modify_board(game_board,user_piece,move):
    """ Modify the board positions into X or O"""
    game_board[move - 1] = user_piece
    
def switch_turns(user_piece):
    """ Change player's piece into X or O"""
    if user_piece == 'X':
        user_piece = 'O'
    else:
        user_piece = 'X'
    return user_piece 

def win_or_tie(user_piece):
    """ display win for X or O or a tie"""
    if user_piece == "TIE":
        print("This is a TIE, but feel free to play again")
    else:
        print(f"Yes the player with {user_piece} won !!" )


def main():
    """ game_board starting point """
    game_board = [1,2,3,4,5,6,7,8,9]
    player_turn = None
    display_welcome()
    display_board(game_board)
    player_turn = determine_pieces()
    print(f"ok you start with {player_turn}")

    while not determine_winner(game_board):
        player_move = get_move(game_board)
        modify_board(game_board,player_turn,player_move)
        display_board(game_board)
        if not determine_winner(game_board):
            player_turn = switch_turns(player_turn)

    win_or_tie(player_turn)

        
if __name__ == "__main__":
    main()

\$\endgroup\$
1
  • \$\begingroup\$ Does this actually work? return 'Tie' vs if user_piece == "TIE" \$\endgroup\$ Commented Jul 31, 2023 at 18:03

1 Answer 1

2
\$\begingroup\$

Overall I think you did a very fine job. The only major problem I saw was in function main where you have:

    win_or_tie(player_turn)

You call determine_winner, which returns either 'X', 'O', 'Tie' or False, but you are not saving that return value. It is that value you should be passing to win_or_tie. Also, win_or_tie is erroneously checking for 'TIE' instead of 'Tie'. Finally, in the loop in main you are calling determine_winner twice. You don't really need that second cal; just unconditionally call switch_turns at the end of the loop.

I do have a few recommendations I might offer to make this game even better.

Let the Users Know Whose Turn It Is

I was running your code playing both X and Y. I got called away from the game for a while and when I returned I had forgotten whose turn it was. If your prompt for a move had included who the current player was, this would have solved that problem. Also, consider the following:

You have not written the program so that a single user is playing against the computer. If you had, it makes sense to ask that user whether they wanted to be X or O, but only if you follow tradition where X goes first. Then you would in essence be asking whether the user wants to go first or second when you ask whether they want to be X or O.

But the computer is not playing against a single user. You have to assume that there are two people playing. So who are you asking when you query whether "the user" wants to be X or O? To me what makes more sense is that the two players have already decided who is X and O (i.e. who goes first). Therefore, when the game begins you should be prompting X for their move and then alternating between O and X for subsequent moves.

There are Many Types of Illegal Moves

What if the user when prompted for a move enters 'a'? You will be trying to convert this to an int and a ValueError exception will be raised. What if '10' is entered? It will be converted to an int successfully but when it is used an index into the game board you will get an IndexError exception. Better would be to test for these additional illegal moves and put out the appropriate error.

Over-architected?

This is an area where people will disagree. But my feeling, for what it's worth, is that you have decomposed the problem into too many small functions that do very little. Although functional decomposition is an important technique, if carried to far it becomes a bit more difficult to follow the logic because of the necessity to jump around. This is purely a value judgement as to when someone has gone to far or not. But when you have a function whose body can be reduced to a single line of code and that function is only referenced once (not reused), perhaps it would be better to just inline that code. This is something for you to think about.

As an example, you have:

def switch_turns(user_piece):
    """ Change player's piece into X or O"""
    if user_piece == 'X':
        user_piece = 'O'
    else:
        user_piece = 'X'
    return user_piece 

First, it's a bit more Pythonic to do:

def switch_turns(user_piece):
    """ Change player's piece into X or O"""
    return 'O' if user_piece is 'X' else 'O'

So here we have a function consisting of a single line. Which is clearer, this:

user_piece = 'X'
while True:
    ... # some logic
    user_piece = switch_user()

Or this:

user_piece = 'X'
while True:
    ... # some logic
    user_piece = 'O' if user_piece == 'X' else 'X'

In the first example you might guess correctly what function switch_turns is doing, but you actually have to now look for the function definition to verify what you had guessed is in fact the case. Neither code example is wrong. I am just emphasising that there is nothing wrong with the second example and perhaps it's better.

Would a More Object-Oriented Approach Be Better?

I have written many Python scripts that did not have a single user-defined class. But this program seems to be begging for a more object-oriented approach. For example, I see a class, TicTacDoeBoard, that encapsulates the implementation of the game board you need and a TicTacDoe class, which HAS a TicTacDoeBoard instance as an attribute but does not concern itself with that game board's implementation. The TicTacDoe class has a play method that essentially encapsulates the logic you have in function main but without having to know any details about how a game board is implemented.

The following is an example (I have tried to incorporate some of my previous suggestions for robustness):

A Slightly More Object-Oriented Solution

#PseudoCode

# Display welcome and instructions
# Display board
# Who gets X or O ?
# While no one has won and it isn't a tie loop
    # get player's move X or O
    # Update the board with the new move
    # and check legal move, so move that hasn't been taken
    #Switch Turns into X or O
#Congratulate Winner X or O or say it's a Tie

class TicTacToeBoard:
    """The tic-tack-toe game board."""

    def __init__(self):
        self._game_board = [1, 2, 3, 4, 5, 6, 7, 8, 9]

    def __str__(self):
        """Convert board to a string."""
        s = []
        for x in range(3):
            s.append("+---+---+---+\n")
            for y in range(3):
                s.append(f"| {self._game_board[x*3+y]} ")
            s.append("|\n")
        s.append("+---+---+---+")
        return "".join(s)

    def display(self):
        print(self)

    def get_and_make_move(self, player):
        """Get the player's move
            Return False if no winner or not yet a tie.
            Else return 'X', 'O' or 'Tie'
        """
        while True:
            try:
                move = int(input(f"Player {player}, please make a move between 1 and 9: "))
            except ValueError:
                pass
            else:
                if 1 <= move <= 9 and self._game_board[move - 1] not in ('X', 'O'):
                    """ Modify the board positions into X or O"""
                    self._game_board[move - 1] = player
                    return self._determine_winner()
            print('That move is not legal! Try again ...')

    def _determine_winner(self):
        """check if there is a win, draw"""
        game_board = self._game_board

        # check rows
        for i in range(3):
            if game_board[i*3] == game_board[i*3+1] == game_board[i*3+2] and game_board[i*3] in ('X', 'O'):
                return game_board[i*3]
        # check columns
        for i in range(3):
            if game_board[i] == game_board[i+3] == game_board[i+6] and game_board[i] in ('X', 'O'):
                return game_board[i]
        # check diagonals
        if game_board[0] == game_board[4] == game_board[8] and game_board[0] in ('X', 'O'):
            return game_board[0]
        if game_board[2] == game_board[4] == game_board[6] and game_board[2] in ('X', 'O'):
            return game_board[2]
        # check tie
        if all(move in ('X', 'O') for move in game_board):
            return 'Tie'
        # No winner or tie:
        return False

class TicTacToe:
    """Tic-Tac-Toe Game."""

    def __init__(self):
        self._board = TicTacToeBoard()

    def play(self):
        self._display_welcome()

        player = 'X'
        while True:
            print()
            self._board.display()
            winner = self._board.get_and_make_move(player)
            if winner:
                break
            player = 'O' if player == 'X' else 'X'

        """ display win for X or O or a Tie"""
        if winner == "Tie":
            print("The game has ended in a tie! But feel free to play again!")
        else:
            print(f"Player {winner} has won!!!")

    def _display_welcome(self):
        """Display Welcome to Tic Tac Toe """

        print("""Hi and welcome to this Tic Tac Toe game board.
You can win by having three X's or 3 O's in a row.
This can be horizontally, vertically or diagonally.

X traditionally goes first. So choose between the two
of you who will be X and who will be O."""
        )

if __name__ == "__main__":
    TicTacToe().play()

Sample Output

Hi and welcome to this Tic Tac Toe game board.
You can win by having three X's or 3 O's in a row.
This can be horizontally, vertically or diagonally.

X traditionally goes first. So choose between the two
of you who will be X and who will be O.

+---+---+---+
| 1 | 2 | 3 |
+---+---+---+
| 4 | 5 | 6 |
+---+---+---+
| 7 | 8 | 9 |
+---+---+---+
Player X, please make a move between 1 and 9: 1

+---+---+---+
| X | 2 | 3 |
+---+---+---+
| 4 | 5 | 6 |
+---+---+---+
| 7 | 8 | 9 |
+---+---+---+
Player O, please make a move between 1 and 9: 4

+---+---+---+
| X | 2 | 3 |
+---+---+---+
| O | 5 | 6 |
+---+---+---+
| 7 | 8 | 9 |
+---+---+---+
Player X, please make a move between 1 and 9: a
That move is not legal! Try again ...
Player X, please make a move between 1 and 9: 10
That move is not legal! Try again ...
Player X, please make a move between 1 and 9: 5

+---+---+---+
| X | 2 | 3 |
+---+---+---+
| O | X | 6 |
+---+---+---+
| 7 | 8 | 9 |
+---+---+---+
Player O, please make a move between 1 and 9: 8

+---+---+---+
| X | 2 | 3 |
+---+---+---+
| O | X | 6 |
+---+---+---+
| 7 | O | 9 |
+---+---+---+
Player X, please make a move between 1 and 9: 9

+---+---+---+
| X | 2 | 3 |
+---+---+---+
| O | X | 6 |
+---+---+---+
| 7 | O | X |
+---+---+---+
Player X has won!!!
\$\endgroup\$
1
  • \$\begingroup\$ I am not sure if I can show grattitude with a comment but I thank you very much for this ! It is clear and I will rework my code and when I get to classes use your example code of Tic Tac Toe with a class. The next step would be to let the computer do it and then classes. Thank you! \$\endgroup\$
    – Joris
    Commented Jul 31, 2023 at 22:48

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