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!!!
return 'Tie'
vsif user_piece == "TIE"
\$\endgroup\$