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