4
\$\begingroup\$

I am new to Python and would like to get helpful tips and suggestions on this code. Would be awesome if you provide info if anything is fundamentally incorrect, improve the algorithm, and any room for further improvements.

class Game:
  def __init__(self):
    self.board = ["0", "1", "2", "3", "4", "5", "6", "7", "8"]
    self.ai = "X"
    self.human = "O"

  def start_game(self):
    print(" %s | %s | %s \n=========\n %s | %s | %s \n=========\n %s | %s | %s \n" % \
        (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]))
    print("Enter number between [0-8]:")
    while not self.game_over(self.board) and not self.draw(self.board):
      self.get_human_position()
      if not self.game_over(self.board) and not self.draw(self.board):
        self.eval_board()

      print(" %s | %s | %s \n=========\n %s | %s | %s \n=========\n %s | %s | %s \n" % \
          (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]))

    print("Game over") # have to think of way to print Draw when it's a draw

  def get_human_position(self):
    spot = None
    while spot is None:
      spot = int(input())
      if self.board[spot] != "X" and self.board[spot] != "O":
        self.board[spot] = self.human
      else:
        spot = None

  def eval_board(self):
    spot = None
    while spot is None:
      if self.board[4] == "4":
        spot = 4
        self.board[spot] = self.ai
      else:
        spot = self.get_optimal_move(self.board, self.ai)
        if self.board[spot] != "X" and self.board[spot] != "O":
          self.board[spot] = self.ai
        else:
          spot = None

  def get_optimal_move(self, board, next_player, depth = 0, highest_score = {}):
    available_spaces = [s for s in board if s != "X" and s != "O"]
    optimal_move = None

    for available in available_spaces:
      board[int(available)] = self.ai
      if self.game_over(board):
        optimal_move = int(available)
        board[int(available)] = available
        return optimal_move
      else:
        board[int(available)] = self.human
        if self.game_over(board):
          optimal_move = int(available)
          board[int(available)] = available
          return optimal_move
        else:
          board[int(available)] = available

    if optimal_move:
      return optimal_move
    else:
      return int(available_spaces[0])

  def three_in_row(self, *args):
    return args[0] == args[1] == args[2] == "X" or \
        args[0] == args[1] == args[2] == "O"

  def game_over(self, b):
    return self.three_in_row(b[0], b[1], b[2]) == 1 or \
        self.three_in_row(b[3], b[4], b[5]) == 1 or \
        self.three_in_row(b[6], b[7], b[8]) == 1 or \
        self.three_in_row(b[0], b[3], b[6]) == 1 or \
        self.three_in_row(b[1], b[4], b[7]) == 1 or \
        self.three_in_row(b[2], b[5], b[8]) == 1 or \
        self.three_in_row(b[0], b[4], b[8]) == 1 or \
        self.three_in_row(b[2], b[4], b[6]) == 1

  def draw(self, b):
    return len([s for s in b if s == "X" or s == "O"]) == 9

if __name__ == '__main__':
  game = Game()
  game.start_game()

Currently I do want to fix the Draw portion where I would print Draw instead of game-over when it is a draw.

\$\endgroup\$
2
\$\begingroup\$

Design

I'm guessing you're coming from some sort of Object-Oriented Programming Language (OOPL), like Java. Python is an interpreted language, and while treating it like an OOPL isn't bad in many large scale scenarios, something small like this doesn't need that design. If this is for some kind of scholastic exercise or you're just training OOPL design in Python, you can stick to it, but personally I'd avoid it here.

Printing & Strings

Pasting out each array index isn't necessary. Instead of using the modulo operator to format strings, you can either use .format() or f-strings. For the board prompt I'd personally use .format(), like so:

print(' {} | {} | {} \n===+===+===\n {} | {} | {} \n===+===+===\n {} | {} | {} \n'.format(*list(n for n in self.board)))

You use variadic assignment in three_in_row, so I'm assuming you know the four use cases of the asterisk.

I notice you use double quotes a lot in your main code but not in the actual main function, so maybe that was pregenerated by an editor like VSCode? Regardless, you can use single quotes more throughout your code.

Game & Logic

None checks and usage seem to be quite frequent throughout this program. Again, this indicates to me some beginner OOP ideology. We all start somewhere, but as it is in good OOP design to not explicitly use null where possible, the same can apply to None here.

In your eval_board function, you prioritize the fourth, or central, spot for the AI. Why not just check this outside of the loop, and then start it if it fails? This function appears to be used exclusively by the AI as well, so why isn't it named something like ai_move or ai_eval to reflect this? Other functions could benefit from this naming as well. Also, using while loops and checking for a None spot seems like a bad substitution for Boolean functions and checks in my mind.

Conveniently, I've also put up a Tic Tac Toe question, which is here: Back to Basics - Tic Tac Toe

Any comments I'd have on optimizing your game logic would just be incorporating some of the techniques used in my implementation. Granted, I don't have an "AI," but the imagination shouldn't need to flex over the available possibilities.

\$\endgroup\$
1
\$\begingroup\$

WET -vs- DRY

In your start_game() method, you have this code, twice:

print(" %s | %s | %s \n===+===+===\n %s | %s | %s \n===+===+===\n %s | %s | %s \n" % \
    (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]))

You should try to write DRY code (Don’t Repeat Yourself), instead of WET code (Write Everything Twice). This common code can be moved into its own method:

def _print_board(self):
    print(" %s | %s | %s \n===+===+===\n %s | %s | %s \n===+===+===\n %s | %s | %s \n" % self.board)

Note: (self.board[0], self.board[1], ... self.board[8]) just creates a list (tuple, actually) of all the elements of self.board, but self.board is already such a list, so can be use directly as the format argument. Way less typing.

@T145 is suggesting "...".format(*list(n for n in self.board)), which can be simplified in exactly the same way, to "...".format(*self.board).

Other things which appear multiple times, that you might want to create a function for include checking that a spot on the board is neither an "X" nor a "O".

Game Over

Your main loop is looping while not game_over and not draw. This begs the question: won’t the game be over if it was a draw?

You should simply be able to loop while the game is not over.

Perhaps you want another function, to check if a winning pattern exists (the current game_over function), and game_over would call both this new function and the draw function, to determine if the game is in fact over.

True is not 1

You are testing the return value of three_in_row() with 1. However, three_in_row() returns a boolean (True or False), not a integer.

You could simply write:

return (self.three_in_row(b[0], b[1], b[2]) or 
    self.three_in_row(b[3], b[4], b[5]) or
    self.three_in_row(b[6], b[7], b[8]) or
    self.three_in_row(b[0], b[3], b[6]) or
    self.three_in_row(b[1], b[4], b[7]) or
    self.three_in_row(b[2], b[5], b[8]) or
    self.three_in_row(b[0], b[4], b[8]) or
    self.three_in_row(b[2], b[4], b[6]))

Note the use of (...)’s around the entire expression to eliminate the need for backslashes.

You are still repeating yourself quite a bit here. You are calling the same function 8 times with different arguments. Usually, we throw things like that into a loop:

rows = ((0, 1, 2), (3, 4, 5), (6, 7, 8),
        (0, 3, 6), (1, 4, 7), (2, 5, 8),
        (0, 4, 8), (2, 4, 6))

for x, y, z in rows:
    if self.three_in_row(b[x], b[y], b[z]):
        return True
return False

Or, even shorter:

return any(self.three_in_row(b[x], b[y], b[z]) for x, y, z in rows)

Private -vs- Public

Python doesn’t have private variables. But convention (which is obeyed by many IDE’s, and understood by various PEP 8 checkers), is to prefix members which are not supposed to be accessed by code external to the class with a leading underscore.

You may have noticed _print_board(self): member I created above had the leading underscore.

Other members should also have leading underscores, such as _board, _ai, _human. In fact, the only public member would be start_game()!

Input Sanitation

What happens if the user doesn’t enter a legal move? Such as -5, 13 or 1.75 or foobar? Does the program crash? The int(input()) will raise a ValueError exception on 1.75 or foobar, crashing the program. 13 is a valid integer, but will give you an IndexError when you index self.board with it. -5 oddly will work just fine; but should it?

Consider adding:

try:
    spot = int(input())
except ValueError:
    spot = None

to check for nonsense input. And follow that with a bounds check like:

if 0 <= spot <= 8:
    # A valid spot was entered
else:
    # Handle invalid spot

get_optimal_move()

None of the arguments beyond board are used, and can be eliminated.

Moves are considered sequentially, and for each move, if the AI wins, or the player wins, that move is returned, so the AI either wins or blocks the player from winning. But ... if a move that blocks the player from winning occurs in the list before a move that the AI wins on occurs, then instead of winning, the AI will simply block the player, which is suboptimal.

You have 3 places where you have:

board[int(available)] = available

You can DRY this code (avoid this duplication) using a try: ... finally: ... structure:

for available in available_spaces:
    move = int(available)
    try:
        board[move] = self.ai
        if self.game_over(board):
            return move
        else:
            board[move] = self.human
            if self.game_over(board):
                return move
    finally:
        board[move] = available

Even when either of the two return statements cause the function to exit, the code in the finally: block will still execute, restoring the board’s state.

Note the move local variable to avoid the int(available) calls everywhere as well.

\$\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.