-3
\$\begingroup\$

Please review my current try of creating a class for chess pieces. I not only want to be able to play a game of chess with this but also solve chess puzzles, thats why I want a flexible board size and pieces that can placed and created by hand.

I put many comments in the code and put the modules that were needed together so you can read it easier. It should still work, print the files of the boardmatrix after the initialization of all pieces and give some information of the black kingside rook. Things that I want to change:

1) reset function gets a start option which initializes all pieces.

2) I dont know how to place the pieces while creating them, thats why the placement is in the __init__ and as a seperate place function to move them.

3) I already have functions like kingMove, queenMove, ... . Where do I put them? In the elif parts where I decide which kind of piece the new one is? I feel like this would make the class too big and hard to keep track of.

# I put all modules in one file for the review and changed the variables from modulename.name to name
# board.py
# the board and game settings are supposed to be here
from string import ascii_uppercase

# I want the dimensions of the board to be able to be set up to 15*15
width = 8
height = 8

# the leading zeros are there to calculate more intuitive: files[2] = B, not C.
files = "0" + ascii_uppercase[:width]
ranks = range(0, height + 1)

# status.py
# contains the boardmatrix which holds the information of how the board currently looks

boardmatrix = [["  " for x in range(1, height + 1)]
               for y in range(1, width + 1)]


def reset(setting):
        # function to control the boardmatrix. i had to use the "global" to be able to change
        # the matrix with the function
    global boardmatrix
    if setting == "empty":
        boardmatrix = [["  " for x in range(1, height + 1)]
                       for x in range(1, width + 1)]
    # elif setting == "start":
        # initialize all pieces
    else:
        print("Something failed.")

# classpiece.py
# class that holds all information about a piece: color, piece, position, its short handle,
# whether rook or king can be used to castle etc.
#


class piece:
    # Summons piece of given color and position
    def __init__(self, color, piece, position):

        # color
        if color == "w":
            self.color = "white"
        elif color == "b":
            self.color = "black"
        else:
            self.color = "none"

        # piece
        if piece == "p":
            self.piece = "pawn"
            self.short = color + "P"
        elif piece == "n":
            self.piece = "knight"
            self.short = color + "N"
        elif piece == "b":
            self.piece = "bishop"
            self.short = color + "B"
        elif piece == "r":
            self.piece = "rook"
            self.short = color + "R"
            self.canCastle = True
        elif piece == "q":
            self.piece = "queen"
            self.short = color + "Q"
        elif piece == "k":
            self.piece = "king"
            self.short = color + "K"
            self.canCastle = True

        # position and place
        self.position = position.upper()
        file = files.index(((self.position[0]).upper()))
        rank = self.position[1:]
        boardmatrix[int(file) - 1][int(rank) - 1] = self.short

    def place(self, position=None):
        # remove old
        oldfile = files.index(((self.position[0]).upper()))
        oldrank = self.position[1:]
        boardmatrix[int(oldfile) - 1][int(oldrank) - 1] = "  "
        # place new
        position = position or self.position
        file = files.index(((position[0]).upper()))
        rank = position[1:]
        boardmatrix[int(file) - 1][int(rank) - 1] = self.short
        # update position
        self.position = position


# i gave each piece a unique name, i dont know how else to initialize and later do stuff with it
# w for white, b for black
wApawn = piece("w", "p", "a2")
wBpawn = piece("w", "p", "b2")
wCpawn = piece("w", "p", "c2")
wDpawn = piece("w", "p", "d2")
wEpawn = piece("w", "p", "e2")
wFpawn = piece("w", "p", "f2")
wGpawn = piece("w", "p", "g2")
wHpawn = piece("w", "p", "h2")

bApawn = piece("b", "p", "a7")
bBpawn = piece("b", "p", "b7")
bCpawn = piece("b", "p", "c7")
bDpawn = piece("b", "p", "d7")
bEpawn = piece("b", "p", "e7")
bFpawn = piece("b", "p", "f7")
bGpawn = piece("b", "p", "g7")
bHpawn = piece("b", "p", "h7")

# Q  for queenside, K for Kingside, W for white square, B for black squre
wQrook = piece("w", "r", "a1")
wQknight = piece("w", "n", "b1")
wBbishop = piece("w", "b", "c1")
wQueen = piece("w", "q", "d1")
wKing = piece("w", "k", "e1")
wWbishop = piece("w", "b", "f1")
wKknight = piece("w", "n", "g1")
wKrook = piece("w", "r", "h1")

bQrook = piece("b", "r", "a8")
bQknight = piece("b", "n", "b8")
bBbishop = piece("b", "b", "c8")
bQueen = piece("b", "q", "d8")
bKing = piece("b", "k", "e8")
bWbishop = piece("b", "b", "f8")
bKknight = piece("b", "n", "g8")
bKrook = piece("b", "r", "h8")

for line in boardmatrix:
    print(line)
print(bKrook.position)
print(bKrook.color)
print(bKrook.canCastle)
\$\endgroup\$

closed as off-topic by Mast, Sᴀᴍ Onᴇᴌᴀ, Ludisposed, Mathias Ettinger, alecxe Dec 27 '17 at 18:06

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Mast, Sᴀᴍ Onᴇᴌᴀ, Ludisposed, Mathias Ettinger, alecxe
If this question can be reworded to fit the rules in the help center, please edit the question.

1
\$\begingroup\$

You have made a few mistakes. Some are website mistakes, while others are programming mistakes.

First, don't merge your files for the review. Instead, separate the files so we can see the file structure, and the imports you use between the files. Dependency checking is part of the review process, if need be.

Second, if you have "things you want to change" you need to list those after your code, with an explanation of why you haven't changed them. It's legit to say you don't know how to do something, or make it work. But this isn't a code-writing service, so don't be surprised if nobody writes that code for you.

With that out of the way, let's look at your code:

Concrete Suggestions

  1. I suggest that you adopt the convention of using capitalized names for classes: Board and Piece instead of board and piece.

  2. You don't have a Board class, but you need one. This is because chess pieces have their location relative to a board, and the pieces are going to move relative to a board. It is legitimate to validate a move by checking the maximum and minimum values for a position: but who decides if those values are okay? The board does!

  3. Your board.py and status.py should be combined into the Board class in chess.py or board.py.

  4. You are trying to use algebraic notation, which is fine until someone creates a 100x100 board. I suggest that you assume that the position information you are given can be decoded into integer-like objects that are comparable and support add and subtract of small integers. Let the Board object handle the decoding. Let the Board object define min and max values for both dimensions. You can write a class that acts like an integer and stringifies to a letter when you need to. Or you can write a Board method that handles conversion of a (rank, file) pair to algebraic form.

  5. You haven't tagged this as Python 2 or Python 3. Both editions support enumerations, so I'll suggest that you either (a) define your colors using enum, or (b) don't define your colors at all, and simply treat them as opaque types. Unfortunately, some pieces need to know direction of move, which is usually color dependent, so ...

    Let's look at a revised __init__ method:

    # chess.py
    
    # class Board: ...
    
    class Piece:
        """Base class for chess pieces."""
    
        def __init__(self, board, shortname=None, *, color=None, position=None, rank=None, file=None):
            if board is None:
                raise TypeError("board may not be None")
    
            if not shortname:
                raise TypeError("shortname must be non-empty")
    
            self.board = board
            self.can_castle = False
            self.color = color
            self.shortname = shortname
    
            if position is None:
                self.rank = rank
                self.file = file
            else:
                self.rank, self.file = board.parse_position(position)
    
            board.place_piece(self.rank, self.file, self)
    

    Notice that the purpose of an __init__ function is that it be an initializer. That is, you use it to set starting values. Either those values are valid, in which case things just work, or you have made a mistake.

    I "outsourced" the parsing of position data to the Board class. That way, a board can implement whatever mechanics it likes for positioning. Similarly, I apply the axiom "tell, don't ask" to get the piece placed on the board. This avoids coupling the Piece and the Board class via knowledge of internal representation or data storage.

    Finally, since Piece is the base class in this system, I set .can_castle to False unilaterally. I'm not actually sure what you mean by this - did you mean "this kind of piece is able to castle under certain circumstances" or did you mean "this particular piece can castle right now if requested?" Regardless, the answer is no at the base level.

  6. With the base class defined, you should use subclasses to define custom behaviors that are different. For example, the possible moves of different pieces, or the .can_castle boolean.

    class Rook(Piece):
        def __init__(self, board, shortname=None, **kwargs):
            shortname = shortname if shortname else "rook"
            super().__init__(board, shortname, **kwargs)
            self.can_castle = True
    

Speculative Suggestions

With the concrete suggestions taken care of, I'll speculate a bit.

  1. You haven't provided any kind of interface for moving, nor checking moves. So I'm going to suggest a .valid_moves() generator. Since moving is unique to the piece, this will be a subclass-specific method.

  2. The Piece class cannot represent any actual piece, since they all have unique moves. That means Piece is an abstract base class and should be marked as such. (Via the abc module.)

  3. Many of the special moves are one-use-only. That means pieces need to know when they move, and how they move, so they can track their own internal state. I suggest adding a .never_moved attribute to the Piece class since this a widespread requirement (pawn, rook, king) for special moves.

  4. I suggest that a .move_piece(piece, rank, file) method on the Board class should handle adding and removing threats.

  5. I suggest a .threatened_by(color) method on the Board class can be used by the pieces to evaluate moves.

Here's some sample code:

from abc import ABC, abstractmethod

class IntBoard:
    """Chess board using integers for rank/file"""

    Colors = ("black", "white") # even = black, odd = white

    Rank = int   # class constructor for ranks
    File = int   # class constructor for files

    class Square:
        def __init__(self, color):
            self.color = color
            self.piece = None
            self.threats = []

        def threaten(self, piece):
            self.threats.append(piece)

        def threatened_by(self, color):
            return any(piece.color == color for piece in threats)

        def unthreaten(self, piece):
            try:
                self.threats.remove(piece)
            except ValueError:
                # Wasn't a threat. Nevermind.
                pass


    def __init__(self, ranks, files=None, *, minrank=None, minfile=None):
        ...

        self.squares = [[self.Square(self.Colors[(r+f)%2])
                         for f in range(files)]
                         for r in range(ranks)]

    def threatened_by(self, color, rank, file):
        return self.squares[rank][file].threatened_by(color)

    def move_piece(self, piece, rank, file):
        """Move piece to a new location. Handle capturing other pieces, updating threats, etc."""
        ...

    def piece_at(self, rank, file):
        """Return piece at given location, or None."""
        return self.squares[rank][file].piece

    def place_piece(self, piece, rank, file):
        """Place piece on the board. Check for collisions, update threats, etc."""
        ...

class Piece(ABC):
    """Base class for chess pieces."""
    def __init__(self):
        ... # as before

    def move_to(self, rank, file):
        self.validate_move(rank, file)
        self.never_moved = False
        self.rank = rank
        self.file = file

    @abstractmethod
    def valid_moves(self):
        """Generate all valid moves for this piece from this location.
        This value *should* be cached, since this method will likely 
        called several times in succession (for threats, unthreats,
        move, etc.).

        """
        for pair in []:
            yield pair

    @abstractmethod
    def validate_move(self, rank, file):
        """Raise an exception if the proposed move is invalid, else pass"""
        pass

class Pawn(Piece):
    """Simple Pawn class. Does not know about en-passant captures
    because that would require some kind of last-move caching, which
    will depend on where you want to go with this code. Uses `dir`
    attribute to indicate direction of travel.

    """
    def __init__(self, *args, dir=1, **kwargs):
        super().__init__(*args, **kwargs)
        self.dir = -1 if dir < 0 else +1

    def valid_moves(self):
        """Yield all valid moves from this location."""

        piece_at = self.board.piece_at
        color = self.color
        file = self.file

        dir = self.dir
        r1 = self.rank + dir

        # Captures
        for f1 in (file-1, file+1):
            if piece_at(r1, f1).color != color:
                yield (r1, f1)

        # Non-captures
        if not piece_at(r1, file):
            yield (r1, file)

            # SPECIAL: Double move if done first 
            r2 = r1 + dir

            if self.never_moved and not piece_at(r2, file):
                yield (r2, file)
\$\endgroup\$

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