3
\$\begingroup\$

I know barely anything about Python, I just jumped into this project as I've already coded it in different languages and it's my go-to when figuring things out in a new one, any feedback will be appreciated.

I deliberately tried to avoid making things too object-oriented with private attributes and whatnot.

This runs slower than the Java / C++ versions I made, despite having already improved the performance by doing a lightweight clone instead of a deep copy.

Board.py

from enum import Enum
class Board:
 #the three states each cell can be in
 class State(Enum): 
 empty = 0
 x = 1
 o = -1
 
 def __init__(self):
 self.reset()
 
 #resets the board
 def reset(self):
 self.cells = [self.State.empty] * 9
 self.turn = self.State.x
 self.currentTurn = 0
 self.winner = self.State.empty
 self.gameOver = False
 
 #applies a move based on the current turn 
 def applyMove(self, move):
 if self.gameOver == False and self.cells[move] == self.State.empty:
 self.cells[move] = self.turn;
 self.update()
 
 #updates the turn
 def endTurn(self):
 if self.turn == self.State.x:
 self.turn = self.State.o
 else: self.turn = self.State.x
 
 self.currentTurn += 1
 
 def getTurn(self):
 return self.turn
 
 def xWon(self):
 self.winner = self.State.x
 self.gameOver = True
 
 def oWon(self):
 self.winner = self.State.o
 self.gameOver = True
 
 def draw(self):
 self.gameOver = True
 
 def getWinner(self):
 return self.winner.value
 
 # returns a list of the empty cells
 def getMoves(self):
 moves = []
 for i in range(len(self.cells)):
 if self.cells[i] == self.State.empty:
 moves.append(i)
 return moves 
 
 #checks for a winner/draw 
 def update(self): 
 
 self.endTurn()
 for i in range(3):
 
 xWin = True;
 oWin = True;
 for j in range(i*3, i*3+3):
 if self.cells[j] != self.State.x:
 xWin = False
 if self.cells[j] != self.State.o:
 oWin = False;
 
 if xWin:
 self.xWon()
 return
 if oWin:
 self.oWon()
 return
 
 xWin = True
 oWin = True
 for j in range(i, i+7, 3): 
 
 if self.cells[j] != self.State.x:
 xWin = False
 if self.cells[j] != self.State.o:
 oWin = False
 
 
 if xWin: 
 self.xWon()
 return 
 if oWin:
 self.oWon()
 return
 step = 4
 for i in range(0, 3, 2):
 
 xWin = True
 oWin = True
 for j in range(3):
 
 if self.cells[i+(j*step)] != self.State.x:
 xWin = False
 if self.cells[i+(j*step)] != self.State.o:
 oWin = False
 if xWin: 
 self.xWon()
 return
 if oWin: 
 self.oWon()
 return
 step = 2;
 if self.currentTurn == 9:
 self.draw()

Brain.py (wrapper for the minimax algorithm)

import copy
from Board import *
class Brain:
 def __init__(self, board):
 self.originalBoard = board
 self.count = 0
 
 #returns the best move according to the minimax method
 def bestMove(self):
 if self.originalBoard.gameOver:
 return
 self.count = 0
 scores = []
 moves = self.originalBoard.getMoves()
 
 for m in moves:
 scores.append(self.miniMax(self.originalBoard, m))
 print(self.count)
 
 if (self.originalBoard.getTurn() == self.originalBoard.State.x):
 return moves[self.max(scores)]
 else:
 return moves[self.min(scores)]
 
 #classic minimax with alphabeta pruning 
 def miniMax(self, board, move, alpha = -1, beta = 1):
 
 self.count += 1
 clone = copy.deepcopy(board) >now replaced as I said in the intro<
 clone.applyMove(move)
 
 if clone.gameOver:
 return clone.getWinner()
 
 availableMoves = board.getMoves() 
 if clone.getTurn() == clone.State.x:
 for m in availableMoves:
 score = self.miniMax(clone, m, alpha, beta) 
 if score > alpha:
 alpha = score 
 if alpha >= beta:
 break
 return alpha 
 
 else:
 for m in availableMoves:
 score = self.miniMax(clone, m, alpha, beta)
 if score < beta:
 beta = score
 if alpha >= beta:
 break
 return beta 
 
 #helper method that returns the index of the highest number in a list
 def max(self, list):
 max = -1
 index = 0
 if list.__len__() == 1:
 return index
 
 for i in range (len(list)):
 if (list[i] > max):
 max = list[i]
 index = i
 return index
 
 #helper method that returns the index of the lowest number in a list
 def min(self, list):
 min = 1
 index = 0
 if list.__len__() == 1:
 return index
 
 for i in range (len(list)):
 if (list[i] < min):
 min = list[i]
 index = i
 return index

TicTacToe.py (main file)

from tkinter import *
from Board import *
from Brain import *
from math import floor as floor
CELLS_ROW = 3
WIDTH = 400
HEIGHT = 400
CELLWIDTH = WIDTH / CELLS_ROW
CELLHEIGHT = HEIGHT / CELLS_ROW
tk = Tk()
tk.title("Tic Tac Toe")
board = Board()
brain = Brain(board)
# Draws the background
def drawBG(): 
 def getBGColor(i, j):
 return "#222222" if i % 2 == j % 2 else "#444444"
 for i in range(0, CELLS_ROW):
 for j in range(0, CELLS_ROW) :
 canvas.create_rectangle(i*CELLWIDTH, j*CELLHEIGHT, CELLWIDTH+i*CELLWIDTH, CELLHEIGHT+j*CELLHEIGHT, fill=getBGColor(i, j))
#Draws the naughts and crosses
def drawMarks():
 thickness = 20
 def drawMark(x, y, mark):
 if mark == board.State.x:
 canvas.create_line(x*CELLWIDTH+thickness, y*CELLHEIGHT+CELLHEIGHT-thickness, x*CELLWIDTH+CELLWIDTH-thickness, y*CELLHEIGHT+thickness, fill="#d32f2f", width = thickness, capstyle = ROUND)
 canvas.create_line(x*CELLWIDTH+thickness, y*CELLHEIGHT+thickness, x*CELLWIDTH+CELLWIDTH-thickness, y*CELLHEIGHT+CELLHEIGHT-thickness, fill="#d32f2f", width = thickness, capstyle = ROUND)
 else: canvas.create_oval(x*CELLWIDTH+thickness, y*CELLHEIGHT+thickness, x*CELLWIDTH+CELLWIDTH-thickness, y*CELLHEIGHT+CELLHEIGHT-thickness, outline="#5a64c8", width = thickness)
 
 for i in range(3):
 for j in range(3):
 if board.cells[i*3+j] != board.State.empty:
 drawMark(j, i, board.cells[i*3+j])
 
#Applies a move based on the mouse position 
def click(event):
 def translateCoords(_x, _y):
 x = floor(_x / CELLWIDTH)
 y = floor(_y / CELLHEIGHT)
 return x + y * CELLS_ROW
 board.applyMove(translateCoords(event.x, event.y))
 updateCanvas()
#Resets the board
def reset(event):
 board.reset()
 updateCanvas()
#Asks the MiniMax algorithm for the best move and applies is
def requestMove(event):
 if board.gameOver == False:
 board.applyMove(brain.bestMove())
 updateCanvas()
#Calls the functions that draw the window
def updateCanvas():
 drawBG()
 drawMarks()
 tk.update()
canvas = Canvas(tk, width=WIDTH, height=HEIGHT)
canvas.bind("<Button-1>", click)
canvas.bind("<Button-2>", reset)
canvas.bind("<Button-3>", requestMove)
updateCanvas()
canvas.pack()
tk.mainloop()
Mast
13.8k12 gold badges57 silver badges127 bronze badges
asked Apr 26, 2017 at 21:57
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Your program looks pretty good for a beginner. A couple of things could make the code easier to work with though, and thus reduce the risk of bugs and extra maintenance should you ever pick this up again.

State

You set a winner as follows:

def xWon(self):
 self.winner = self.State.x
 self.gameOver = True
def oWon(self):
 self.winner = self.State.o
 self.gameOver = True
def draw(self):
 self.gameOver = True

winner is a State and the gameOver is a separate variable. gameOver can't be part of State, because State is actually being abused for something it wasn't supposed to do:

#the three states each cell can be in
class State(Enum): 
 empty = 0
 x = 1
 o = -1

You're not using it just for the cell state. You're using it for the cell state, the board state (who has won) and even the turn state. Apparently it's acceptable to have a board without winner and without next-player (board state and turn state can be set to empty). If you set either of those to empty, I'd assume the game to be over as well and thus there's no need for a gameOver state. But there's an even better way.

You've tried to reduce duplication by using the same class twice. However, by doing so, you've introduced duplication elsewhere in a way that makes less sense than when you'd have allowed the duplication (making either 2 separate constructs to hold state or make 1 that's configurable).

Holding game-state in an Enum is a great idea. Holding cell-state in an Enum is too. But their possible states are not the same.

A board/game state for Tic Tac Toe can be the following:

  • Incomplete (ongoing)
  • Complete (finished, game over)

To keep things simple, we can replace the latter by 3 more specific states:

  • X won
  • Y won
  • Draw

A cell state can be the following:

  • X
  • Y
  • Empty

Do we have to check for those 3 specific states now to check whether a game is finished? No. Just check whether the board is unfinished. If it's not unfinished, it must be finished. Because those are the only states defined. This would simplify some of the checks you have later on.

For example:

if xWin:
 self.xWon()
 return
if oWin:
 self.oWon()
 return

Note: this construct is used 3 times in the same function (update(self)), which indicates the function is either poorly structured or doing too much. Or both.

If the winner is part of the game state, you can return it straight away and not bother with a double if checking temporary variables. The only question at this point is whether there should be returned at all. This could look like the following:

if gameState.xWon or gameState.yWon:
 return

This:

xWin = True
oWin = True
for j in range(3):
 if self.cells[i+(j*step)] != self.State.x:
 xWin = False
 if self.cells[i+(j*step)] != self.State.o:
 oWin = False

Completely defies the whole point of working with states in the first place. Why have states at all when at one point you set the game to assume two winners? One false step in those next lines of code and you've broken an otherwise perfectly safe program.

Comments near definitions

You like to state the purpose of what happens near a definition:

#the three states each cell can be in
class State(Enum):
#applies a move based on the current turn 
def applyMove(self, move):
#Resets the board
def reset(event):
#Asks the MiniMax algorithm for the best move and applies is
def requestMove(event):
#Calls the functions that draw the window
def updateCanvas():

This is commendable. Did you know Python has an in-built feature that makes this even better? It's called docstrings.

Consider PEP 257 – Docstring Conventions.

Docstrings provide a convenient way of putting human-readable documentation in the code itself that's easily extracted by other scripts/programs. They can be used on modules, functions, classes and methods.

A straightforward conversion would result in: class State(Enum): """The three states each cell can be in."""

def applyMove(self, move):
"""Applies a move based on the current turn."""
def reset(event):
"""Resets the board."""
def requestMove(event):
"""Asks the MiniMax algorithm for the best move and applies it."""
def updateCanvas():
"""Calls the functions that draw the window."""

Minor note: if requestMove fails to perform a move, it will not provide feedback in any way and there was a typo in the comment.

Implementing docstrings allows other programs, the Python interpreter and most IDEs to provide guidance on the functions you're using (with IntelliSense). You can then extent this even further using typing (assuming Python 3.5+) and verify whether everything that needs a docstring has one with doctest. This can be very useful when using code defined in a different file/module/library.

Readability

Most of your code is quite easy to read. Whenever it's hard to follow, that's usually due to how you play around with states. See above. Other parts of your code are easily improved.

For example:

def drawMark(x, y, mark):
 if mark == board.State.x:
 canvas.create_line(x*CELLWIDTH+thickness, y*CELLHEIGHT+CELLHEIGHT-thickness, x*CELLWIDTH+CELLWIDTH-thickness, y*CELLHEIGHT+thickness, fill="#d32f2f", width = thickness, capstyle = ROUND)
 canvas.create_line(x*CELLWIDTH+thickness, y*CELLHEIGHT+thickness, x*CELLWIDTH+CELLWIDTH-thickness, y*CELLHEIGHT+CELLHEIGHT-thickness, fill="#d32f2f", width = thickness, capstyle = ROUND)
 else: canvas.create_oval(x*CELLWIDTH+thickness, y*CELLHEIGHT+thickness, x*CELLWIDTH+CELLWIDTH-thickness, y*CELLHEIGHT+CELLHEIGHT-thickness, outline="#5a64c8", width = thickness)

I think you'll agree with me that the following is much easier on the eyes:

def drawMark(x, y, mark):
 if mark == board.State.x:
 canvas.create_line(
 x * CELLWIDTH + thickness,
 y * CELLHEIGHT + CELLHEIGHT - thickness,
 x * CELLWIDTH + CELLWIDTH - thickness,
 y * CELLHEIGHT + thickness,
 fill = "#d32f2f",
 width = thickness,
 capstyle = ROUND
 )
 canvas.create_line(
 x * CELLWIDTH + thickness,
 y * CELLHEIGHT + thickness,
 x * CELLWIDTH + CELLWIDTH - thickness,
 y * CELLHEIGHT + CELLHEIGHT - thickness,
 fill = "#d32f2f",
 width = thickness,
 capstyle = ROUND
 )
 else:
 canvas.create_oval(
 x * CELLWIDTH + thickness,
 y * CELLHEIGHT + thickness,
 x * CELLWIDTH + CELLWIDTH - thickness,
 y * CELLHEIGHT + CELLHEIGHT - thickness,
 outline = "#5a64c8",
 width = thickness
 )

Now the differences and similarities between the canvas calls are clear at a single glance (don't forget to update the canvas.create_rectangle call as well). You're only using canvas.create_line twice, but if you'd use it more often I'd consider writing a new function for it as a wrapper. It might be a bit overkill at this point.

Give your operators some breathing room, don't make your lines too long and be consistent with whitespace.

There are a couple of magic numbers in your code. Some of the numbers you've turned into a pseudo-constant, like CELLS_ROW = 3. But there are a lot of 3 remaining in your code and all colours are hardcoded. If you put a value into a constant, make sure it's properly named.

For example:

for i in range(0, CELLS_ROW):
 for j in range(0, CELLS_ROW):

One of those is going to be a column, not a row.

Consider PEP 8 - Style Guide for Python Code.

answered Nov 13, 2022 at 12:16
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Hey, thank you for taking the time to answer to a 5 and a half year old question. I especially agree with the part about code readablility. \$\endgroup\$ Commented Nov 14, 2022 at 16:15

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.