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.
2 Answers 2
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.
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.
-
\$\begingroup\$ I'd clarify that point about booleans w/ this: programiz.com/python-programming/methods/built-in/bool Part of my hope was that the OP would catch on to most of these optimizations (that expound on what I target), but oh well :p \$\endgroup\$T145– T1452019年11月04日 03:20:03 +00:00Commented Nov 4, 2019 at 3:20
Explore related questions
See similar questions with these tags.