Validate Move Pattern Chess

Question

A while ago, I was working on a chess project with a couple other people. In the end, I wrote all but a couple of the lines of code, including these:

class Piece(list):

    def IsValidMovePattern(self, FromCoord, ToCoord):
        print(self, "Validation not done")
        return False


class King(Piece):

    def IsValidMovePattern(self, FromCoord, ToCoord):

        move = [abs(ToCoord[0] - FromCoord[0]), abs(ToCoord[1] - FromCoord[1])]

        # One square in any direction
        if move[0] <= 1 and move[1] <= 1:
            return True

        # Castling - AND has higher priority than OR
        if move[0] == 0 and move[1] == 2:
            if self[1] == Player(1) and FromCoord == (0, 4) or self[1] == Player(2) and FromCoord == (7, 4):
                return True

        return False


class Queen(Piece):

    def IsValidMovePattern(self, FromCoord, ToCoord):

        move = [abs(ToCoord[0] - FromCoord[0]), abs(ToCoord[1] - FromCoord[1])]

        # Diagonal and straight moves
        if move[0] == move[1] or move[0] == 0 or move[1] == 0:
            return True

        return False


class Rook(Piece):

    HasMoved = False

    def IsValidMovePattern(self, FromCoord, ToCoord):

        move = [ToCoord[0] - FromCoord[0], ToCoord[1] - FromCoord[1]]

        if move[0] == 0 or move[1] == 0:
            return True

        return False


class Knight(Piece):

    def IsValidMovePattern(self, FromCoord, ToCoord):

        move = [abs(ToCoord[0] - FromCoord[0]), abs(ToCoord[1] - FromCoord[1])]

        # L-shaped moves
        if move[0] == 1 and move[1] == 2 or move[0] == 2 and move[1] == 1:
            return True

        return False


class Bishop(Piece):

    def IsValidMovePattern(self, FromCoord, ToCoord):

        move = [abs(ToCoord[0] - FromCoord[0]), abs(ToCoord[1] - FromCoord[1])]

        # Can only move diagonal
        if move[0] == move[1]:
            return True

        return False


class Pawn(Piece):

    # Set to True when a pawn moves two squares past
    # Pawn must be on square 3 for PlayerOne or square 4 for PlayerTwo
    EnPassant = False

    def IsValidMovePattern(self, FromCoord, ToCoord):

        move = [ToCoord[0] - FromCoord[0], ToCoord[1] - FromCoord[1]]

        # Player(1) capture
        if self[1] == Player(1) and abs(move[1]) == move[0] == 1:
            return True

        # Player(2) capture
        if self[1] == Player(2) and abs(move[1]) == 1 and move[0] == -1:
            return True

        # Player(1) first move
        if self[1] == Player(1) and FromCoord[0] == 1 and move[1] == 0 and move[0] == 2:
            return True

        # Player(2) first move
        if self[1] == Player(2) and FromCoord[0] == 6 and move[1] == 0 and move[0] == -2:
            return True

        # Player(1) move
        if self[1] == Player(1) and move[1] == 0 and move[0] == 1:
            return True

        # Player(2) move
        if self[1] == Player(2) and move[1] == 0 and move[0] == -1:
            return True

        return False

This is called like this (where self is similar to this in C#; it is being called from the Board class):

if not self.data[FromCoord[0]][FromCoord[1]].IsValidMovePattern(FromCoord, ToCoord):

FromCoord and ToCoord are tuples (similar to immutable arrays) like (FromX, FromY).

Is this a good implementation? Would this be a slow implementation, or is it reasonable? The rest of the code (with Checkmate and the AI incomplete) can be found at Github.

Answer 1

Don't extend list when your class isn't actually a list. It makes every instance that much more complicated when there is no reason to. What does it mean when you call .insert() on a Piece? It breaks all of your code and should not be possible in the first place.

---

Note: I wrote the following section because I thought you were using the base class to represent the coordinate of the specific piece. After being confused at why you where comparing the y coordinate to Player(1), I had to look at your linked code. This showed that you were doing something entirely different. The fact that I got that far with a completely wrong understanding of how your code works is a huge indication that the design is bad.

A list is good for doing list type things. Don't use it as solution to everything and treat it like a bag you can do whatever you want with. There are many different core data structures that do different things. You should learn how they work and when to use them.

I'm going to leave the following section as it still has valid points about giving things names and how classes should be structured.

---

What you want is a Coordinate structure that represent a location on the board. A Coordinate has an x value and a y value for the corresponding axises. Using a collection where the index is significant hides information and requires readers to know that x comes before y. If you don't want to write the small amount of code it takes to create the fields, you can use namedtuple to generate the class for you. I would do this for the tuples you are getting passed in as arguments, as well. from_coord.x is much clearer than from_coord[0].

Taking this a step forward into object oriented design. A chess piece is not a Coordinate. A chess piece has a location on the board, which is a Coordinate. Based on this, it might makes sense for Piece to have a coord instance variable.

---

move is a bad variable name. By the name, it is not clear what it represents. When you look at the code, you see that is change in respective coordinate values between the from coordinate and the to coordinate. Since you are using lists for both your coordinates and these move delta values, things become harder to follow as a specific index is related, but not the same thing. At first glance, it looks like move is just a coordinate.

---

Using Player(1) to access the enum value (code that should have been included in the question directly, not linked to) can be confusing. At first glance it looks like you are creating an instance of a class. Instead Player.One looks like you are referencing a constant. Accessing the value programmatically is supported for:

situations where Color.red won't do because the exact color is not known at program-writing time. Source

This is not that situation. You know exactly which value you wan to be accessing.

---

# Castling - AND has higher priority than OR
if move[0] == 0 and move[1] == 2:
    if self[1] == Player(1) and FromCoord == (0, 4) or self[1] == Player(2) and FromCoord == (7, 4):
        return True

You can use parenthesis to group boolean expressions in order to ensure the indented order of operations. Doing this would mean you wouldn't need a nested if clause.

---

Even though you are familiar with C# conventions, when you are working in Python, you should use Python conventions. In Python, method and variable names are in snake_case, not CapWords. Class names do stay as CapWords. (PEP8: Naming Convetions)

Answer 2

One improvement could be extracting all board geometry operations involving FromCoord and ToCoord into well-named static/helper methods out of your concrete piece classes.

That cleans up your IsValidMovePattern methods a bit as it really describes the actual piece movement / game rules at a higher level and leaves the implementation details (IsInAStraightLine(), IsInADiagonal())to your your helpers to figure out.

The other advantage is that you can avoid code duplication in cases where two pieces can share the same board geometry methods (e.g. a Queen can move like a Rook and a Bishop)