Enumerating Chess Piece Moves

There was a question asked recently here that went through enumerating moves for a chess piece. I was going to write an answer for this question, but while I was bashing out some code to throw into an answer, I figured why not build it out completely instead and have it critiqued. Note that this doesn't implement en-passant or castling, as these are a bit of a pain.

```
'''
chess.py

A very simple implementation of a chess board and pieces, mainly
for the purposes of enumerating all possible moves a piece can
make from a given board state.
'''

from __future__ import print_function

from itertools import chain, imap, takewhile

board_size = range(8)
BLACK = 'black'
WHITE = 'white'

class InvalidMoveError(Exception):
'''Exception that is raised when an invalid move is
attempted.
'''

def __init__(self, msg):
super(InvalidMoveError, self).__init__(msg)

class Square(object):
'''Defines a board square. This is given an x and y co-ordinate,
and can optionally be populated with a piece. The co-ordinates
go from (0, 0) to (7, 7), where (0, 0) is the top left corner
and (7, 7) the bottom right. The first value indicates the row,
the second the column.
'''

def __init__(self, x, y, piece=None):
self._x = x
self._y = y
self._piece = piece

@property
def piece(self):
return self._piece

@property
def x(self):
return self._x

@property
def y(self):
return self._y

@property
def coordinates(self):
return (self._x, self._y)

def is_empty(self):
'''Returns True if this Square is not populated with,
True otherwise.
'''
return self._piece is None

def can_move_to(self, piece):
'''Checks if the piece passed as an argument can move to
this square. A piece may move to a square that is empty or
that contains a piece of the opposing colour.
Returns True if the piece can make t

I answered the other chess question I think you're referring to, and I'm going to have a go at answering yours; I'm still not a chess player, though, or an very experienced object oriented coder, so I'm speaking more to the lines of code, naming, readability and possible bugs than the big-picture design. It's nit-picky, but I hope with some merit.

Overall, I'm not loving the interaction between board, squares and pieces. Pieces don't know where they are, so to list the moves you give a piece its coordinates, the piece asks the board for squares in its move pattern, the board returns some squares, and the piece asks the squares if it can move to them. The piece is proxying all the data through it, but not adding much to it.

Then squares can enumerate the moves of the piece on them, which feels like a shortcut to avoid asking a square for a piece, then asking the piece for its moves; why build lots of objects to keep things separate, then mix up their responsibilities like that?

Pieces could have move_dir and start_row as instance properties calculated once in init; calculating the start_row of a piece every time it enumerate_moves() for the entire game, feels like the wrong place to put that code.

Presumably you read the answers in the other chess question from other people, about how enumerating the moves piece by piece, turn by turn is very costly, and pre-computing the moves for all pieces before the game is the only way to get fast enough performance for a chess engine to try many possible moves? That looks like it applies to your code (e.g. no caching), but it might not be relevant.

Okay, from the top...

def can_move_to(self, piece):
    '''Checks if the piece passed as an argument can move to
    this square.

The square doesn't move, so 'can_move_to' feels misleading. a_square.is_valid_destination_for(piece) or a_square.is_available_to(piece) might make the intent more clear. But I think the piece should be responsible for the check, asking the square if it is empty, or what colour the piece on the square is, then deciding if it can move there or not.

What is the wider intent for being able to compare squares with eq? Is it right that two squares are considered equal if they have the same co-ordinates, but one is empty and one has a piece on it?

Can this:

if other is not None and isinstance(other, Square):

become:

if isinstance(other, Square):

? isinstance(None, Square) is False, so the first test seems unnecessary.

In English: "an empty square" - empty is an adjective, which suggests it might be a better fit for a property (square.empty) rather than a method (square.is_empty()). How a square determines whether it is empty (calculation vs. storing it as a bool) is an implementation detail and irrelevant to the outside world.

In the Pawn class, American spelling of color typo:

def opponent_piece_exists(square):
        if not square.is_empty():
            return (square != InvalidSquare() and 
                    square.piece.color != self.colour)

In the Knight class, the Knight adds eight squares to its move list, you exclude squares which are off the board, but what if one of the squares in the move list has a same-colour piece on it? (I think the board will move the Knight onto it, and the piece in to_square disappears from the game).

indices = but it's not index-plural, it's squares, or possible move destinations.

Bishops. not_invalid could be called valid.

If the bishop is at 0,0 then board_size[1:] moves the bishop.x 0+1, 0+2, 0+3... ok. But if the bishop is at 7,7 then board_size[1:] moves the bishop.x 7+1, 7+2, ... so the [1:] in four places makes the code slightly harder to read, to save checking if the bishop can move onto itself. (Which it can't, because of the can_move colour check).

Rooks. Same comment as bishops.

Kings. Same comment as knights - they can move on top of other same-colour pieces.

(I can deal with 0,0 being in the top left corner, but using x to move vertically and y to move horizontally keeps tripping me up. Is that a Chess thing? A common game or graphics thing?)

I almost flagged up the use of itertools.takewhile as a bug, until I realised it constrains the possible moves at the edge of the board, and also cutting off when other pieces are involved. That's neat.

Slightly misleading docstring:

class Board(object):
    '''A Board defines an object that encapsulates the 64 squares of
    an 8x8 chess board. This is initialized to a default starting
    board.
    '''

But it doesn't do that, it defines an object that encapsulates the ?? squares of a len(board_size)*len(board_size) chess board.

And board_size is taken from the module(?) scope, instead of being a parameter to the board init, that seems risky.

Is board_size ever going to be non-consecutive numbers? Can you replace:

```
if all((x >= board_size[0], y >= board_size[0],
x <= board_size[-1], y <= board_size[-1]))

StackExchange Code Review Q#95255, answer score: 4