3
\$\begingroup\$

I'm a beginner to intermediate in programming, and created a Tic-tac-toe game in python. I plan to add an AI player and a GUI using pygame.

Any improvements I could make to the code, especially with the logic, or just formatting in general?

my_board = [
    ["1", "2", "X"],
    ["4", "5", "6"],
    ["7", "8", "9"]
]


class Tictactoe():
  def __init__(self, board):
    self.board = board
  
  def print_board(self):
    print("|-------------|")
    print("| Tic Tac Toe |")
    print("|-------------|")
    print("|             |")
    print("|    " + self.board[0][0] + " " + self.board[0][1] + " " + self.board[0][2] + "    |")
    print("|    " + self.board[1][0] + " " + self.board[1][1] + " " + self.board[1][2] + "    |")
    print("|    " + self.board[2][0] + " " + self.board[2][1] + " " + self.board[2][2] + "    |")
    print("|             |")
    print("|-------------|")
    print()

  def available_moves(self):
    moves = []
    for row in self.board:
      for col in row:
        if col != "X" and col != "O":
          moves.append(int(col))
    return moves

  def select_space(self, move, turn):
    row = (move-1)//3
    col = (move-1)%3
    self.board[row][col] = "{}".format(turn)

  def has_won(self, player):
    for row in self.board:
        if row.count(player) == 3:
            return True
    for i in range(3):
        if self.board[0][i] == player and self.board[1][i] == player and self.board[2][i] == player:
            return True
    if self.board[0][0] == player and self.board[1][1] == player and self.board[2][2] == player:
        return True
    if self.board[0][2] == player and self.board[1][1] == player and board[2][0] == player:
        return True
    return False
  
  def game_over(self):
    self.available_moves()
    if self.has_won("X") or self.has_won("O") or len(self.available_moves()) ==0:
      return True
    else:
      return False

  def get_player_move(self, turn):
    move = int(input("Player {player} make your turn: ".format(player=turn)))
    self.available_moves()
    while move not in range (1,10) or move not in self.available_moves():
      move = int(input("Invalid move, try again: "))
    return move
      

     
def game(board):
  tictactoe = Tictactoe(my_board)
  while not tictactoe.game_over():
    tictactoe.print_board()
    x_move = tictactoe.get_player_move("X")
    tictactoe.select_space(x_move,"X")
    tictactoe.print_board()
    if tictactoe.game_over():
      break

    o_move = tictactoe.get_player_move("O")
    tictactoe.select_space(o_move,"O")
  if tictactoe.has_won("X"):
    print("X wins")
  elif tictactoe.has_won("O"):
    print("O wins")
  else:
    print("tie")



game(my_board)
\$\endgroup\$
4
\$\begingroup\$

Indents

PEP 8: Use 4 spaces per indentation level.


Main game loop

The way your game loop is set up now is a bit unelegant. You already implemented all the methods to be able to handle both players, there's only one piece missing:

from itertools import cycle

players = cycle(['X', 'O'])

cycle will provide a generator that infinitely repeats the iterable you pass it. This makes the loop quite a bit cleaner:

while not tictactoe.game_over():
    tictactoe.print_board()

    player = next(players)
    move = tictactoe.get_player_move(player)
    tictactoe.select_space(move, player)

I'd also suggest renaming select_space to something like do_move. Makes it a bit clearer what's going on.


Redundant calls to Tictactoe.available_moves()

In game_over and get_player_move you call self.available_moves() once without using the return value in any way. You can simply delete those lines.


game_over

The pattern

if self.has_won("X") or self.has_won("O") or len(self.available_moves()) ==0:
    return True
else:
    return False

can be simplified to

return self.has_won("X") or self.has_won("O") or len(self.available_moves()) == 0

What you were doing is basically manually returning the resulting bool value of a condition. We can instead simply return the condition directly.


print_board

Python lets you easily avoid an unelegant pattern

print("|    " + self.board[0][0] + " " + self.board[0][1] + " " + self.board[0][2] + "    |")

by using the * operator for unpacking an iterable

print("|   ", *self.board[0], "   |")

Since we need to do this for all rows in self.board we should let Python handle it for us as well:

for row in self.board:
    print("|   ", *row, "   |")

has_won

For a cleaner implementation of this method I would suggest implementing helper functions that provide lists of the rows, columns and diagonals of the board. They might also be useful for future feature additions.

def rows(self):
    return self.board[:]  # creates a mutable copy of the rows

def columns(self):
    return [[row[index] for row in self.board] for index in range(3)]

def diagonals(self):
    first = [self.board[index][index] for index in range(3)]
    second = [self.board[index][len(self.board[index]) - index - 1] for index in range(3)]
    return [first, second]

These methods are also ready to be used for boards of different sizes. Simply replace the 3s by a variable.

has_won is now a lot simpler and more readable:

def has_won(self, player):
    for line in self.rows() + self.columns() + self.diagonals():
        symbols = set(line)
        if player in symbols and len(symbols) == 1:
            return True

    return False

We can take this a step further (as pointed out in the comments) and change has_won to get_winner and return the winning symbol. This will also affect some other parts of your code:

class Tictactoe:

def get_winner(self):
    for line in self.rows() + self.columns() + self.diagonals():
        symbols = set(line)
        if len(symbols) == 1:
            return symbols.pop()

    return None


def game_over(self):
    return self.get_winner() is not None or len(self.available_moves()) == 0

Printing our winner in game:

def game(...):
    
    # main gameplay here
    
    if (winner := self.get_winner()) is not None:
        print(f"{winner} wins")
    else:
        print("tie")
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Plenty of good advice here. In addition, the OP could consider another simplification of has_won(): unless I've overlooked something, it doesn't need the player argument: if all symbols are the same (and not empty), someone has one; just return who it is. \$\endgroup\$
    – FMc
    Apr 21 at 22:55
  • \$\begingroup\$ Very good suggestion, I added it to my answer! \$\endgroup\$ Apr 22 at 15:29
2
\$\begingroup\$

one performance improvement suggestion

available_moves

Instead of iterating through all the moves create a set with all the moves in the beginning and as each player makes a move remove the corresponding move from the set.

In your code both iterating through all moves to find the valid moves(available_moves function) and then the checking if the move given is valid (move not in self.available_moves()) both of this are o(n) operations. But using set I believe both these operation could average to o(1).

Might not be a huge difference in performance considering tic tac toe only has 9 moves. But might be a good way to tackle the problems in other similar situations.

\$\endgroup\$

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.