4
\$\begingroup\$

This is an object-oriented Chess game. So far, I have implemented the pawn and board functionalities and I would love to get a review early on before diving deep into the project.

piece.py

# piece.py

from abc import ABC, abstractmethod

class Piece(ABC):

    def __init__(self, color):
        if color.lower() != 'black' and color.lower() != 'white':
            raise ValueError('Invalid color argument')
        self.color = color.lower()

    def can_move(self):
        pass

pawn.py

# pawn.py

from piece import Piece

class Pawn(Piece):

    def __init__(self, color, co_ordinates):
        super().__init__(color)
        self.co_ordinates = co_ordinates
        self.first_move = True

    def can_move(self, board_list, co_ordinates):
        new_row, new_column = co_ordinates
        
        if self.color == 'black':
            return self.__can_move_black(board_list, new_row, new_column)

        if self.color == 'white':
            return self.__can_move_white(board_list, new_row, new_column)

    def __can_move_black(self, board_list, row, column):       
        location_is_empty = board_list[row][column] == 0
        if (self.co_ordinates[0] + 1 == row) and location_is_empty: 
            return True

        if self.first_move and location_is_empty:
            self.first_move = False
            return self.co_ordinates[0] + 2 == row and self.co_ordinates[1] == column;

        lhs_is_valid = self.co_ordinates[0] + 1 == row and self.co_ordinates[1] - 1 == column
        rhs_is_valid = self.co_ordinates[0] + 1 == row and self.co_ordinates[1] + 1 == column

        if rhs_is_valid or lhs_is_valid:
            if not location_is_empty  and board_list[row][column].color != self.color:
                return True;

        else:
            return False

    def __can_move_white(self, board_list, row, column):
        location_is_empty = board_list[row][column] == 0
        if (self.co_ordinates[0] - 1 == row) and location_is_empty:
            return True

        if self.first_move and location_is_empty:
            self.first_move = False
            return self.co_ordinates[0] - 2 == row and self.co_ordinates[1] == column

        lhs_is_valid = self.co_ordinates[0] - 1 == row and self.co_ordinates[1] - 1 == column
        rhs_is_valid = self.co_ordinates[0] - 1 == row and self.co_ordinates[1] + 1 == column
        
        if rhs_is_valid or lhs_is_valid:
            if not location_is_empty and (board_list[row][column].color != self.color):
                return True;

        else: 
            return False

    def board_repr(self):
        return self.color[0] + 'p'

    def __repr__(self):
        return 'Pawn(%s, %r)' % (self.color, self.co_ordinates)

board.py

# board.py

from pawn import Pawn

class Board:
    def __init__(self):
        self.board_list = [
            [ 0 for i in range(8)] for i in range(8)
        ]
        self.__init_pieces()

    def __init_pieces(self):
        self.__init_pawns()

    def __init_pawns(self):
        row = 1
        for column in range(8):
            self.board_list[row][column] = Pawn('Black', (row, column))
        row = 6
        for column in range(8):
            self.board_list[row][column] = Pawn('White', (row, column))

    def draw(self):
        for row in self.board_list:
            for column in row:
                if column != 0:
                    print(column.board_repr(), end=' ')
                else:
                    print(column, end=' ')
            print()

    def move(self, piece, co_ordinates):
        if piece != 0 and piece.can_move(self.board_list, co_ordinates):
            row, column = co_ordinates

            if self.__can_capture(row, column):
                self.board_list[row][column] = 0

            piece_row, piece_column = piece.co_ordinates
            piece.co_ordinates = co_ordinates

            self.board_list[row][column], self.board_list[piece_row][piece_column] = self.board_list[piece_row][piece_column], self.board_list[row][column]

        else:
            print('Invalid move')

    def __can_capture(self, row, column):
        location_is_empty = self.board_list[row][column] == 0
        if not location_is_empty:
            return True
        else:
            return False

    def __promote(self, piece, new_piece):
        new_piece.co_ordinates = piece.co_ordinates
        row, column = new_piece.co_ordinates
        board_list[row][column] = new_piece


if __name__ == '__main__':
    board = Board();
    board.move(board.board_list[1][0], (3,0))
    board.move(board.board_list[6][1], (4,1))
    board.move(board.board_list[3][0], (4,1))
    board.move(board.board_list[0][0], (4,1))
    #board.move(pawn, (4,1))
    board.draw();

\$\endgroup\$
4
\$\begingroup\$

Abstract classes

can_move is suspicious. Can move where? Chess moves are coordinate-dependent, so I would expect that this method accept the same parameters as your child, i.e. board_list and co_ordinates.

Also, you're not properly using ABC. You need can_move to be an @abstractmethod.

Stringly-typed colours

Not only is using strings for black and white a troubled idea, you're baking some bugs into your application. You do case-sensitive comparison to black and white, but then initialize with Black and White. Instead, represent player with a boolean or an Enum.

Comprehensions

[ 0 for i in range(8)] for i in range(8)

first, you shouldn't reuse an iteration variable at two different levels of nesting. Second, these don't actually need names at all, and can be _.

Don't repeat yourself

    row = 1
    for column in range(8):
        self.board_list[row][column] = Pawn('Black', (row, column))
    row = 6
    for column in range(8):
        self.board_list[row][column] = Pawn('White', (row, column))

can be

for row, colour in ((1, 'Black'), (6, 'White')):
    for column in range(8):
        # ...

Boolean expressions

    if not location_is_empty:
        return True
    else:
        return False

can simply be

return not location_is_empty
\$\endgroup\$
1
  • \$\begingroup\$ "Don't repeat yourself" applies also to can_move_black vs. can_move_white, which may remain but should call a common function for the real work in terms of one or two extra parameters (a sign for direction certainly - and I would use the home row number instead of a first_move flag) \$\endgroup\$ – Hagen von Eitzen Nov 18 '20 at 20:12
4
\$\begingroup\$

Don't encode player turn as a string

turn = "black"
turn = "white"

You're using a string for something which can be represented by a simple boolean value.

if color.lower() == "black":

While the above may seem simple, when performed over and over again it will slow down your program. Comparison of strings is slow and expensive. However, comparing two numbers is much faster, a single computer instruction.

Your best bet here is probably to use an enum, or a plain variable in your class that will represent a boolean / integral value

class Color:
    white = 0 # or False
    black = 1 # or True
Color.white.value
Color.black.value

Code structure

class Pawn(Piece):
    #...

Careful! What you have done now is created your own Pawn class that already has quite a few methods. By doing this you have forced yourself to create five more classes representing a piece.

  • pawn, bishop, knight, rook, queen, king

What you will notice is a loooot of repetition. I can say that for sure because your Pawn class already has two nearly identical methods. Your program will be very slow since your board instead of having simple numbers to represent pieces, it will have huge class objects. Move generation would a nightmare. Re-think your code structure. Do you really have to represent individual pieces using separate classes?

What is it that computers love? Numbers. It's the fastest and simplest way. Each piece gets a certain value. Black pieces would be negative, white pieces would be positive. This will also be very useful for evaluation. Once you have your values assigned, you can simply initialize the board like

board = [
[rook.value, knight.value, ....],
# ...

You now have a direct map of the pieces. Move-generation becomes much easier and faster


Representing moves

Right now since you are starting, A move is just row and col. That doesn't stay, once you begin your move-generator you will realize you need a few more attributes. To list them

  • Old position
  • New position
  • Castling
  • En-passant
  • Promotion

You need to maintain both the old position and new position if a player wants to undo a move. The pawn is evil, en-passant and pawn promotion are two very special kinds of moves since they don't follow any of the usual rules, which requires you to keep an extra attribute. All of this is best represented by a data class

@dataclass
class Move:
    old_position : int
    new_position : int
    ...

chessprogramming.org is an excellent resource for this topic. It has HUGE amounts of information, everything you need for this topic. It will surely help you.

python-chess is a chess library in Python that implements all of these ideas. I don't suggest you directly use it, but seeing its interface can give you nice ideas.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Great review, thanks for the link :-) \$\endgroup\$ – theProgrammer Nov 18 '20 at 20:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.