Chess game function

Question

I wrote a little C++ program for playing Chess:

Now I check the valid moves by filling an 8x8 board with different flags called cBoard.

• fNONE = none type
• canPASS = the piece can go there
• cannotPASS = the piece is blocked
• canEAT = the piece can go there and will eat a piece

The Piece has 2 properties:

• id = ROOK,PAWN,...,NONE
• color = BLACK or WHITE

void ChessMoves::fillcBoardPawn(int row, int col){
    //reset cBoard
    resetcBoard();

    //WHITE PAWN
    //get pieceMaster
    Piece const& pieceMaster = refBoard.getPieceAt(row,col);
    if(pieceMaster.getColor() == WHITE){
        //check if it can go fw
        //  |r-1c-1|r-1|r-1c+1|
        Piece actualPiece = refBoard.getPieceAt(row-1,col);
        if(actualPiece.getId() != NONE){
            cBoard[row-1][col] = cannotPASS;
        }
        else{
            cBoard[row-1][col] = canPASS;
        }
        //check if can eat
        actualPiece = refBoard.getPieceAt(row-1,col-1);
        if(actualPiece.getId() != NONE){
            if(actualPiece.getColor() != pieceMaster.getColor()){
                    //different color can eat
                    cBoard[row-1][col-1] = canEAT;
            }
        }
        actualPiece = refBoard.getPieceAt(row-1,col+1);
        if(actualPiece.getId() != NONE){
            if(actualPiece.getColor() != pieceMaster.getColor()){
                    //different color can eat
                    cBoard[row-1][col+1] = canEAT;
            }
        }

        if(row == 2){
            //check if in initial position, then can make two jump
            actualPiece = refBoard.getPieceAt(row-2, col);
            if (actualPiece.getId() != NONE){
                    cBoard[row-2][col] = cannotPASS;
            }
            else{
                        cBoard[row-2][col] = canPASS;
            }
        }
    }
    //BLACK PAWN
    //the black pawn has everything the same but is going down instead of up
}

This is the function that checks the moves of the pawn. As you can see, this is a really big function and I wrote only half of it. How could I write this function in a more concise way?

Answer 1

Main Comments:

• I think (by the name of your function) you are missing out on polymorphism (see at bottom)

• Make sure your code is correctly indented.
  I think you had tabs mixed in with your spaces and the last half was uneven
  (I fixed it so I could read it).

• Personally I like the { and } to line up (but it is personal taste)
  But in this case it would have made it easier to see why the code was not lining up correctly.

• Don't write comments that mimic the code. Comments are supposed to explain stuff in the code that is not obvious and documents the code. The worst thing that can happen is that the code and comments will get out of sync over time. By explaining what is supposed to happen rather than how it whould happen your comments are more likely to stay good.

This comment is completely useless.

    //reset cBoard
    resetcBoard();

• Unless you really want to copy the piece always get a reference to them:

Here

// This is making a copy of a piece
Piece actualPiece = refBoard.getPieceAt(row + direction, col);

// You probably ment:
Piece const& actualPiece = refBoard.getPieceAt(row + direction, col);

// Note you can't change a reference once seated so for other squares you
// need other variables. Don't worry about this it will not cost extra space
// in the executable.

Stuff to make it more concise:

The same code can be used for both black and white.
You just need to set a direction variable:

int direction = pieceMaster.getColor() == WHITE ? -1 : +1;

Then wherever you use:

cBoard[row-1][col]

// instead use:

cBoard[row + direction][col]

Rather then write if test that assign the same variable in both branches use a ternary expression:

if (test)
{
     state = result1;
}
else
{
     state = result2;
}

// Instead you can use:

state = test ? result1 : result2;

Rather than doing 2 consecutive tests with no other options bring them into a single test:

if (test1)
{
    if (test2)
    {
        action;
    }
}

// can be simplified too:

if (test1 && test2)
{
    action;
}

Simplified too:

void ChessMoves::fillcBoardPawn(int row, int col)
{
    resetcBoard();

    Piece const& pieceMaster = refBoard.getPieceAt(row,col);
    int   const  direction   = (pieceMaster.getColor() == WHITE) ? -1 : +1;

    //check if it can go forward
    Piece const& actualPieceAhead = refBoard.getPieceAt(row + direction, col);

    cBoard[row + direction ][col] = actualPieceAhead.getId() == NONE ? canPASS :cannotPASS;

    //check if can eat (take a piece)
    for(int side = -1; side <= 1; side += 2)
    {
        Piece const& actualPieceTake = refBoard.getPieceAt(row + direction, col + side);
        if ((actualPieceTake.getId() != NONE) && (actualPieceTake.getColor() != pieceMaster.getColor()))
        {
            cBoard[row + direction][col + side] = canEAT;
        }
    }

    //check if in initial position, then can make two jump
    if(row == 2)
    {
        Piece const& actualPiece2Ahead = refBoard.getPieceAt(row + 2*direction, col);
        cBoard[row+ 2*direction][col] = actualPiece2Ahead.getId() == NONE ? canPASS : cannotPASS;
    }
}

Polymorphism:

It looks like all this work is done on the board. But really you should be using polymorphism to mark the board. ie the piece knows its own type and can mark the board appropriately.

class ChessPiece
{
    public:
        virtual ~ChessPiece() {}
        virtual void fillcBoard(ChessMoves& board) = 0;
}

class ChessPiecePawn: public ChessPiece
{
    public:
        virtual void fillcBoard(ChessMoves& board)
        {
            // Code As above
        }
};

Now a piece on a board can fill in the cBoard without the user of the piece actually knowing what the piece is:

board[1][1].fillcBoard(board);

Answer 2

Each piece id (PAWN, etc.) should have its own class with a method that calculates its legal moves. The "constants" that take on different values for different colors (direction of pawn movement (+1 or -1), location of starting rows (1 or 8) should be variables that are set by either indexing a static const vector by a color enum or by referencing properties of a piece's associated Color object.

The detailed behavior for each piece id should be encoded into a few virtual functions originally defined on an abstract "PieceId" class (PieceId as opposed to Piece which is something that has a Color, PieceId, position, and isAlive flag) and overridden in a class for each specific piece id (PAWN, ROOK, etc).

If you are going to use row and col as vector indexes, you should follow the 0-based indexing idiom, using values 0 through 7 in 8-element vectors rather than using values 1 through 8 requiring 9-element vectors with unused [0] slots.

It's not really clear how you are using this fillcBoard -- I imagine that you are using it when a piece is selected to identify legal moves (whether internally or as a visual hint for learners). Depending on usage, it might be simpler NOT to distinguish canPass and canEat in this function. Since canEat really means the same as "canPass && the refBoard position is not empty", which might be simpler to test later as you need to.

You might want to make your actualPiece local variable a const Piece* so you don't have to keep copying an object. An alternative would be to use a simpler function than getPieceAt like getColorAt that returned a pointer to the occupying piece's color or null for an empty space -- or just return a Color but add a NO_COLOR enum value that can apply only to board locations but not to pieces.