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()
1 Answer 1
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.