Help with refactoring my Tic Tac Toe game

Question

I'm very new to coding and have been diving into the skill with Python as my first language. One of the first programs I wrote for a class was this Tic Tac Toe game. I re-wrote this game using classes and have just recently refactored it looking for different smells. The person who's been teaching the class suggested that I post it here for additional help optimizing it. Any help or suggestions for improvement is much appreciated! Is there anything that smells funky to you?

#Written in Python 3.3.0, adapted for Python 2.7.3
#Tic Tac Toe Part 2
class Board(object):
 """Represents tic tac toe board"""
 def __init__(self):
 self.move_names = '012345678'
 self.Free_move = ' '
 self.Player_X = 'x'
 self.Player_O = 'o'
 self.moves = [self.Free_move]*9
 def winner(self):
 """Wining combinations. Returns Player or None"""
 winning_rows = [[0,1,2],[3,4,5],[6,7,8], # up and down
 [0,3,6],[1,4,7],[2,5,8], # across
 [0,4,8],[2,4,6]] # diagonal
 for row in winning_rows:
 if self.moves[row[0]] != self.Free_move and self.allEqual([self.moves[i] for i in row]):
 return self.moves[row[0]]
 def allEqual(self, list):
 """returns True if all the elements in a list are equal, or if the list is empty."""
 return not list or list == [list[0]] * len(list)
 def getValidMoves(self):
 """Returns list of valid moves"""
 return [pos for pos in range(9) if self.moves[pos] == self.Free_move]
 def gameOver(self):
 return bool(self.winner()) or not self.getValidMoves()
 def startover(self):
 """Clears board"""
 self.moves = [self.Free_move]*9
class Player(object):
 """ Represents player and his/her moves"""
 def __init__(self, moves):
 self.moves = moves
 self.whoseturn = 1
 def makeMove(self, move, player):
 """Plays a move. Note: this doesn't check if the move is legal!"""
 self.moves[move] = player
class PlayerX(Player):
 """ Player X's moves"""
 def __init__(self, moves):
 self.moves = moves
 def xmove(self): 
 print("Move Player X")
 move = int(input())
 if -1 < move < 9:
 for i in self.moves[move]:
 if i == ' ':
 super(PlayerX, self).makeMove(move, "X")
 print(self.moves)
 return 1
 else:
 print("That's not a valid move")
 return 0
 else:
 print("That's not a valid move")
class PlayerO(Player):
 """ Player O's Moves"""
 def __init__(self, moves):
 self.moves = moves
 def ymove(self):
 print("Move Player O")
 move = int(input())
 if -1 < move < 9:
 for i in self.moves[move]:
 if i == ' ':
 super(PlayerO, self).makeMove(move, "O")
 print(self.moves)
 return 1
 else:
 print("That's not a valid move")
 return 0
 else:
 print("That's not a valid move")
def tictac():
 print(welcome_mes)
 b = Board()
 x = PlayerX(b.moves)
 y = PlayerO(b.moves)
 p = Player(b.moves)
 while b.gameOver() == False:
 if p.whoseturn % 2 == 0:
 print("Turn #" + str(p.whoseturn))
 if x.xmove() == 1:
 p.whoseturn += 1 
 else:
 print("Turn #" + str(p.whoseturn))
 if y.ymove() == 1:
 p.whoseturn += 1
 print(b.winner() + " has won the game")
 print("would you like to play again?")
 Yes, yes, YES, yeah, Yeah = "yes","yes", "yes", "yes", "yes"
 answer = input()
 if str(answer) == "yes":
 b.startover()
 tictac()
 else:
 quit()
welcome_mes = """Welcome to Tic Tac Toe!
The Game board is as follows:
 _0_|_1_|_2_
 _3_|_4_|_5_
 6 | 7 | 8 ""

Answer 1

For someone new to coding, there's a lot here that is good. Naming of classes, functions and variables are generally good. Comments follow convention, with simple, concise Docstrings for everything. Methods are generally simple and do only one thing, which is nice.

---

Let's go through a few things that I think can be improved:

In the winner function, every time this is called, winning_rows is recreated. However, this will be the same for any Board, and won't change. Hence, we can make this a class attribute:

class Board(object):
 winning_rows = [[0,1,2],[3,4,5],[6,7,8], # up and down
 [0,3,6],[1,4,7],[2,5,8], # across
 [0,4,8],[2,4,6]] # diagonal

In winner, we simply call it as follows:

def winner(self):
 """Wining combinations. Returns Player or None"""
 for row in Board.winning_rows:
 #Code as before

In fact, the user of Board probably shouldn't change this. Other object-oriented language have the notion of private variables: variables that aren't visible and can't be accessed outside their own class. Python doesn't have this restriction, but it does have a convention that anything prefixed by an underscore should probably be treated as such, hence it should be _winning_rows.

---

Naming is generally good, but just watch out for the few inconsistencies that are lurking around: self.move_names or self.Free_move? It's nice to keep things uniform, either start everything with lower case, or start everything with upper case. Starting everything with lowercase would be "more pythonic" in my eyes, but consistency is key. The same thing applies to method names, e.g. gameOver vs startover.

---

Using Player as a base class and PlayerX, PlayerY as subclasses isn't really the way to go here. Note that xmove and ymove are almost exactly the same. This sort of code duplication is a definite sign that you should reconsider your design. Instead of subclassing for each type of Player, it would be far better to simply create difference instances of Player, each one with perhaps an attribute like self.piece. Hence, it would look something like:

class Player(object):
 """ Represents player and his/her moves"""
 def __init__(self, piece, moves):
 self.moves = moves
 self.piece = piece
 def getMove(self):
 print("Move Player %s" % self.piece)
 #...
 def makeMove(self, move):
 #...
 def __str__(self):
 return "Player " + self.piece

I've modified the names and shuffled things around a bit here, but hopefully you can see what's going on. This cuts down on the repeated code. Note we've also directly created a __str__ method that will be called whenever we call str() on a Player.

---

There are a few bits of leaky abstraction. self.whoseturn = 1 shouldn't live in Player - this doesn't really encapsulate anything about the player themselves, it's instead information about the state of the game - hence it's in the wrong spot. It likely shouldn't be a simple int either. In fact, I think the part of the code that needs the most work is the tictac() function. Instead of simply having a function, there should be another class that encapsulates the idea of a game. This will hold a Board, both the Players, and will co-ordinate whose turn it is, whether the game is finished, and perform cleanup and reset functions for a new game:

class Game(object):
 _welcome_mes = """Welcome to Tic Tac Toe!
The Game board is as follows:
 _0_|_1_|_2_
 _3_|_4_|_5_
 6 | 7 | 8 """
 def __init__(self):
 self.board = Board()
 self.playerX = Player('x', self.board.moves)
 self.playerO = Player('o', self.board.moves)
 self._currentTurn = self.playerX
 def _swapCurrentTurn(self):
 if self._currentTurn == self.playerX:
 self._currentTurn = self.playerO
 else: self._currentTurn = self.playerX
 def start(self):
 print(Game._welcome_mes)
 while not self.isGameOver():
 print("Turn: " + str(self._currentTurn))
 if self._currentTurn.move():
 self._swapCurrentTurn()
 self._atGameEnd()
 def _atGameEnd(self):
 print(self.board.winner() + " has won the game")
 print("Would you like to play again?")
 answer = input()
 if str(answer).lower() in ["yes", "y", "yeah"]:
 self._restart()
 def _restart(self):
 self.board.clear()
 self.playerX = Player('x', self.board.moves)
 self.playerO = Player('o', self.board.moves)
 self._currentTurn = self.playerX 
 def isGameOver(self):
 return self.board.gameOver()

To play the game, all you need to do is make a Game object and call start() on it. This cleans up the logic a bit (and was done a bit hastily, so can probably be further improved. However, it should give you a general idea at least). I've left out any docstrings, which you should add if you decide to go this route.

I've also cleaned up the string checking to see if the user wants to play again: firstly, we convert it to lowercase, so we don't need to worry about dealing with the difference between "yes" and "YES", and then we simply check if it is in a few different options with in.

---

There's another minor thing I would think about fixing/adding: having the board print out in rows of 3, instead of a single flat list. This should be fairly simple.

Answer 2

In addition to what Yuushi wrote:

• It would be better to have Board validate the moves, instead of Player. That way the players would work even if you changed the size of the board. For an invalid move Board could raise an exception that you handle in the main game loop.

• If you moved the handling of user input to the tictac function, it would be easier to understand the whole user experience.