3
\$\begingroup\$

I'm new to Python and I just finished a school assignment where I had to make a tictactoe game with focus on OOP. I would love to get some pointers on how I can clean up my code (especially my Tictactoe class) as it's pretty messy right now.

What I've done so far is comment as much as possible to make it understandable, but I feel like my tictactoe class is a mess and I would like to get some pointers on how I can optimize it.

The class below enforces the rules and draws the board:

import os
class Board:

    board = [0,1,2,3,4,5,6,7,8,9]
    win_combinations = [
        (1,2,3),
        (4,5,6),
        (7,8,9),
        (1,5,9),
        (3,5,7),
        (1,4,7),
        (2,5,8),
        (3,6,9),
    ]
    GameOver = False

    #Draws the board
    def drawboard(self): 
        print('=========')
        print(self.board[7], '|', self.board[8], '|', self.board[9])
        print(self.board[4], '|', self.board[5], '|', self.board[6])
        print(self.board[1], '|', self.board[2], '|', self.board[3])
        print('=========')

    #Checks if the move the player just made, made him/she win the game
    def checkIfWon(self, choice):

        for a, b, c in self.win_combinations:
            if self.board[a] == self.board[b] == self.board[c]:
                print('Game over, player ' + choice + ' won the game')
                self.GameOver = True

    #Update the current board 
    def update(self, input, choice):

        self.board[input] = choice
        os.system('clear')
        self.drawboard()
        self.checkIfWon(choice)

    #Resets the board
    def resetBoard(self):
        self.board = [0,1,2,3,4,5,6,7,8,9]

    #Stops the game if tie
    def tie(self):
        list = []
        for x in self.board:
            if type(x) != int:
                list.append(x)

        if len(list) == 9:
            return True

The class below contains the runGame method which starts the game:

import os
from board import Board

class Tictactoe():

    b = Board()
    choicePlayer1 = ''
    choucePlayer2 = ''
    corretChoice = False
    correctPlayer1 = False
    correctPlayer2 = False


    def runGame(self):
        os.system('clear')

        #Resets the game when a new game is started
        #Is necessary if the players wish to play more than 1 game
        resetGame(self)

        #Makes sure the game only starts if player1 picks X or O
        while self.corretChoice == False:

            self.choicePlayer1 = input('Do you want to play X or O? ')
            print()
            if self.choicePlayer1 == 'X':
                self.choicePlayer2 = 'O'
                self.corretChoice = True
                print('Starting player selected X')

            elif self.choicePlayer1 == 'O':
                self.choicePlayer2 = 'X'
                self.corretChoice = True
                print('Starting player selected O')
            else:
                print('ERROR - input has to be either X or O!')
                continue

        os.system('clear')
        self.b.drawboard()

        while self.b.GameOver == False:
            self.correctPlayer1 = False
            self.correctPlayer2 = False

            #For player1
            while self.correctPlayer1 == False:
                while True:
                    try:
                        x = int(input(self.choicePlayer1 + ' Where do you want to place your piece? '))
                        break
                    except:
                        print('Input has to be a number, try again')

                if x > 0 and x < 10 and type(self.b.board[x]) != str:
                    self.b.update(x, self.choicePlayer1)
                    self.correctPlayer1 = True
                elif x == 10:
                    quit()
                else: 
                    print('Spot is taken, try again: ')

                if self.b.GameOver == True:
                    self.correctPlayer2 = True

                if self.b.tie() == True:
                    self.correctPlayer2 = True
                    self.b.GameOver = True
                    print('Game is a tie')
            #For player2
            while self.correctPlayer2 == False:
                while True:
                    try:
                        x = int(input(self.choicePlayer2 + ' Where do you want to place your piece? '))
                        break
                    except:
                        print('Input has to be a number, try again')

                if x > 0 and x < 10 and type(self.b.board[x]) != str:
                    self.b.update(x, self.choicePlayer2)
                    self.correctPlayer2 = True
                elif x == 10:
                    quit()
                else: 
                    print('Spot is taken, try again: ')

                if self.b.tie() == True:
                    self.b.gameOver = True
                    print('Game is a tie')

#Resets the game if the players wishes to play again
def resetGame(self):
    self.b = Board()
    self.choicePlayer1 = ''
    self.choucePlayer2 = ''
    self.corretChoice = False
    self.correctPlayer1 = False
    self.correctPlayer2 = False
    self.b.resetBoard()

The script i run to start the game:

from tictac import Tictactoe

run = Tictactoe()

while True:
    run.runGame()
    if input("Play again? (y/n)") == "n":
        quit()
\$\endgroup\$
3
\$\begingroup\$

Now, as has been said before: this is more of a codereview than a question. Nevertheless:

  1. You're defining class Board: without parantheses and class Tictactoe() with. No big deal reall, but a bit of inconsistency. I personally just put parens on every declaration. Rids you of having to think about it.
  2. The variables board, win_combinations and GameOver are defined as class variables since they're declared in the body of the class definition itself. This means that should you for some reason instantiate two games at once they'll mess with each other. It would be better to put them into a constructor __init__(self) (the actual constructor is __new__ but you hardly need to edit that so init is generally refered to as constructor). That way every instance has their own instances of these variables.
  3. The names of the variables and general format doesn't conform to PEP8. Some things here are: Docstrings for classes to describe what they do etc.; variables named snake_case (except for globals/constants) and classes CamelCase; two lines between top level classes, functions etc.; double quotes rather than single quotes. Also the variables probably aren't supposed to be manipulated from outside the class so you could tell other programmers so by prefixing them with an underscore "_".
  4. Maybe add some additional abstraction. The board in your case isn't only a board but also includes gamelogic like checking for a win etc.
  5. You're using + for string concatenation/interpolation which is deprecated print('Game over, player ' + choice + ' won the game'). The more modern and also more efficient way would be to use an f-string f"Game over, player {choice} won the game".
  6. The block

    def tie(self):
        list = []
        for x in self.board:
            if type(x) != int:
                list.append(x)
    

    could be written as a list comprehension or generator expression: list = [x for x in self.board if type(x) != int]. Or using the filtermethod: list = list(filter(lambda x: type(x) != int, self.board)). You should also rename list to list_ or an actual expressive name saying what it represents here. And in the same function you could just return len(list) == 9 since that already is a boolean expression.

That's it for the board class. Most of the stuff like format, not using class variables etc also applies to the TicTacToe class.

  1. resetGame(self) should probably be a method on the class so you can do self.reset_game() or similar.
  2. Typo in corretChoice -> correct_choice.
  3. b is a really inexpressive variable name for the board. Why don't you name it board? Especially if it's used across the whole class (rather than being just a local variable) that would make the code a lot clearer.
  4. You're catching everything here:

    try:
        x = int(input(self.choicePlayer1 + ' Where do you want to place your piece? '))
        break
    except:
        print('Input has to be a number, try again')
    

    which is really bad style. It will for example also catch stuff like keyboard interrupts. Since you want to catch errors in the conversion what you probably want is except ValueError:.

And lastly not really an error but: if the user inputs anything other than n on the "play again?"-prompt it'll restart.

I also feel like the runGame method is way too large - I'll see if I can come up with a clearer solution and post it up here if I can.

EDIT: I've tried my hand at refactoring you're code:

import os

class Board():
    """Represents the game-board"""
    def __init__(self):
        self.board = [i for i in range(10)]
        self._win_combinations = [
            (1, 2, 3),
            (4, 5, 6),
            (7, 8, 9),
            (1, 5, 9),
            (3, 5, 7),
            (1, 4, 7),
            (2, 5, 8),
            (3, 6, 9)]
        self.game_over = False

    def draw_board(self):
        """Draws the board to the terminal"""
        print("=========")
        print(self.board[7], "|", self.board[8], "|", self.board[9])
        print(self.board[4], "|", self.board[5], "|", self.board[6])
        print(self.board[1], "|", self.board[2], "|", self.board[3])
        print("=========")

    def check_if_won(self, player):
        """Checks if the move the player just made, made him/her win the game"""
        for a, b, c in self._win_combinations:
            if self.board[a] == self.board[b] == self.board[c]:
                print(f"Game over, player {player} won the game")
                self.game_over = True

    def update(self, input, choice):
        """Update the current board"""
        self.board[input] = choice
        os.system("clear")
        self.draw_board()
        self.check_if_won(choice)

    def reset_board(self):
        """Resets the board"""
        self.board = [i for i in range(10)]

    def tie(self):
        """Stops the game if tie"""
        list_ = list(filter(lambda x: type(x) != int, self.board))
        return len(list_) == 9


class TicTacToe():
    def __init__(self):
        os.system("clear")
        self.board = Board()
        self.player_1_char = ""
        self.player_2_char = ""
        self.corret_choice = False
        self.get_player_char()

    def reset(self):
        """Resets the internal state to prepare for a new game"""
        self.player_1_char = ""
        self.player_2_char = ""
        self.board.reset_board()

    def get_player_char(self):
        """Ask the player what character he wants to use and verify choice"""
        while True:
            player_1_char = input("Do you want to play X or O? ")
            print()
            if player_1_char == "X":
                self.player_1_char = "X"
                self.player_2_char = "O"
                print("Starting player selected X")
                break
            elif player_1_char == "O":
                self.player_1_char = "O"
                self.player_2_char = "X"
                print("Starting player selected O")
                break
            else:
                print("ERROR - input has to be either X or O!")
        os.system("clear")

    def get_player_input(self, player_char):
        while True:
            while True:
                x = input(f"{player_char} Where do you want to place your piece?")
                if x.isdigit():
                    x = int(x)
                    break
                else:
                    print("Input has to be a number, try again")

            if x > 0 and x < 10 and type(self.board.board[x]) != str:
                self.board.update(x, player_char)
                break
            elif x == 10:
                quit()
            else: 
                print("Spot is taken, try again: ")

    def check_tie(self):
        if self.board.tie():
            self.board.game_over = True
            print("Game is a tie")
            return True
        return False

    def run(self):
        self.board.draw_board()

        while not self.board.game_over:
            self.correct_player_1 = False
            self.correct_player_2 = False

            self.get_player_input(self.player_1_char)
            if self.board.game_over:
                break
            if self.check_tie():
                break

            self.get_player_input(self.player_2_char)
            if self.board.game_over:
                break
            if self.check_tie():
                break


while True:
    TicTacToe().run()

    user_input = "a"
    while user_input not in "ny":
        user_input = input("Play again? (y/n)").lower()

    if user_input == "y":
        continue
    else:
        break
\$\endgroup\$
  • 1
    \$\begingroup\$ "class Board: is Python 2 syntax" - nope. It's just Python. Parens are optional in either version. \$\endgroup\$ – Reinderien Apr 1 at 13:58
  • \$\begingroup\$ whoops, sorry. Don't know why I had that in mind like this. Still: He defines one class with and one without parens. \$\endgroup\$ – SV-97 Apr 1 at 14:04
  • \$\begingroup\$ It's OK; edit your question to suit and I'll upvote it. Everything else looks good. \$\endgroup\$ – Reinderien Apr 1 at 14:05
  • 1
    \$\begingroup\$ @SV-97 Thank you for writing such a detailed response. Really helpful. I didn't know about PEP8, was a nice read. \$\endgroup\$ – mikkelræv Apr 1 at 14:25
  • 1
    \$\begingroup\$ No problem - I generally enjoy code reviews and profit from them too :D I've just added my version of your code if you want to have a look though I'd of course encourage you to try implementing some of the mentioned things yourself beforehand. \$\endgroup\$ – SV-97 Apr 1 at 14:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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