Help with refactoring my Tic Tac Toe game

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

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 le

StackExchange Code Review Q#24934, answer score: 7