Tic-Tac-Toe project in Python

Question

Here's a Tic-Tac-Toe program I wrote in Python. How can I improve it (.eg. make it more readable)?

class Gameboard():
 def __init__(self):
 self.gameboard = {1:' ' , 2: ' ', 3: ' ', 4:' ', 5:' ', 6:' ', 7:' ', 8:' ', 9:' '}
 def setPeice(self, user, position, gameboard):
 gameboard[position] = user
 return gameboard
 @property
 def gameBoard(self):
 '''
 need to learn more about properties. They seem to be used primarily as
 getters and setters. ??
 '''
 return self.gameboard
 def clearboard(self):
 self.gameboard = {1:' ' , 2: ' ', 3: ' ', 4:' ', 5:' ', 6:' ', 7:' ', 8:' ', 9:' '}
 def is_place_taken(self, gameboard, index):
 if gameboard[index] != ' ' :
 return True
 def is_board_full(self, gameboard):
 for index in range(1, 10):
 if gameboard[index] == ' ':
 return False
 return True
 def is_game_won(self, gameboard):
 win_conds = ((1,2,3), (4,5,6), (7,8,9), (1,4,7), (2,5,8), (3,6,9), (1,5,9), (3,5,7))
 for win_cond in win_conds:
 if gameboard[win_cond[0]] == gameboard[win_cond[1]] and gameboard[win_cond[1]] == gameboard[win_cond[2]] and gameboard[win_cond[0]]!= ' ':
 return True
 def printBoard(self,gameboard):
 index = 0
 for row in range(1,4):
 for column in range(1,4):
 index += 1
 if column != 3:
 print(gameboard[index], end='')
 print('|', end='')
 else:
 print(gameboard[index])
class Game():
 def on_start(self):
 '''
 Called on initial start and in subsequent restarts
 '''
 self.controlBoard = Gameboard()
 self.gameboard = self.controlBoard.gameBoard
 self.playerOne ='o'
 self.playerTwo = 'x'
 print('Welcome to tic-tac-toe')
 print("What is player one's name?")
 self.player_one = input(' : ')
 print("What is player two's name?")
 self.player_two = input(' : ')
 print('Here is your game board, each place is represented by 1-9, starting from left column each time and movng along the row')
 self.controlBoard.printBoard(self.gameboard)
 self.turn = 1
 def on_end(self):
 #check if a player wants to end the game
 if self.running == False:
 replay = input('Press 0 to quit or 1 to play again: ')
 try:
 if int(replay):
 self.running = True
 self.on_start()
 except:
 print("A number must be entered.")
 self.on_end()
 def takeTurn(self, user, peice):
 print(user + ' choose a place, 1-9')
 try:
 position = int(input(': '))
 if position > 9 or position < 1:
 raise Exception
 except:
 print('Pick a number between 1-9')
 return self.takeTurn(user, peice)
 if self.controlBoard.is_place_taken(self.gameboard, position):
 print("That place is taken")
 self.takeTurn(user,peice)
 else:
 self.controlBoard.setPeice(peice, position, self.gameboard)
 self.controlBoard.printBoard(self.gameboard)
 if self.controlBoard.is_game_won(self.gameboard):
 print(user + " wins.")
 self.running = False
 def main(self,):
 self.running = True
 self.on_start()
 while self.running:
 if self.turn%2 != 0:
 self.takeTurn(self.player_one, 'o')
 else:
 self.takeTurn(self.player_two, 'x')
 if self.controlBoard.is_board_full(self.gameboard):
 print("Its a draw!! You both lose!")
 self.running = False
 self.turn += 1
 if not self.running:
 self.on_end()
if __name__ == '__main__':
 Game().main()

Answer 1

Strive for self-documenting code

Python's any and all are extraordinarily useful, but few people know about them, for example:

def is_board_full(self, gameboard):
 for index in range(1, 10):
 if gameboard[index] == ' ':
 return False
 return True

Is understandable but not obvious, using all it becomes:

def is_board_full(self, gameboard):
 return all(gameboard[index] != ' ' for index in range(1, 10))

Kind of better, now let's use native iteration:

def is_board_full(self, gameboard):
 return all(square != ' ' for square in gameboard.values())

Now we can define a constant:

EMPTY = ' '
...
def is_board_full(self, gameboard):
 return all(square != EMPTY for square in gameboard.values())

That reads as "the board is full if all the squares are not empty", you may prefer "the board is full if not at least one one square is empty"), expressed as:

def is_board_full(self, gameboard):
 return not any(square == EMPTY for square in gameboard.values())

The most self-documenting of all is probably using a helper:

def is_there_at_least_one_empty(self, gameboard):
 return any(square == EMPTY for square in gameboard.values())
def is_board_full(self, gameboard):
 return not self.is_there_at_least_one_empty(gameboard)

The code now looks like a high level description of the problem, not a specific implementation.

@staticmethod

You pass self into your methods, but most do not use it. A method that does not use self is called a static method and the convention is not to pass self to those methods by applying the @staticmethod decorator:

@staticmethod
def is_there_at_least_one_empty(gameboard):
 return any(square == EMPTY for square in gameboard.values())
@staticmethod
def is_board_full(gameboard):
 return not GameBoard.is_there_at_least_one_empty(gameboard)

Given that all the methods in Gameboard are static you may consider using top-level functions, and achieve separation of concerns by putting those in a separate file. Using a class adds complexity and in this case you do not seem to need nor want one (using only static methods defies OOP completely)

Simplify

Do not start by writing a lot of loops and ifs, you start with a description of the problem:

"To print a board, I print each one of its line"

"To print a line, I print the squares joined by |"

"To divide a board into lines, I divide it in chunks of 3"

If you are wondering, dividing a list in chunks of x is present on StackOverflow: https://stackoverflow.com/questions/10364391/how-to-split-python-list-into-chunks-of-equal-size

def split_into_lines(board):
 return zip(*[iter(board.values())]*3)
def _print_line(line):
 print(' | '.join(line))
def print_gameboard(board):
 for line in split_into_lines(board):
 _print_line(line)

This is much clearer and modular then your original version.

Answer 2

Your class Gameboard can't make up its mind whether it is a namespace for some functions (like is_place_taken) that work on a game board passed in, or methods that work on self. In Game.on_start you create the controlboard and then pull out of it the gameboard which is the dictionary of positions and their contents. This is vey confusing, because you then end up passing the gameboard back in to the functions on Gameboard.

It looks to the reader as if there are two boards, but there are not. Have class Gameboard take responsibility for representing the state of the game and do not expose how that is stored to clients of Gameboard (eg Game)

Answer 3

Counting the number of turns taken and testing what that is modulo 2 to find out who's turn it is seems very complex. You could have a boolean variable, for example to say that o should play next and just invert it after each turn