3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ oh thx for the edit @Loki Astari, why there was that error? I had it formatted correctly... \$\endgroup\$ – Pella86 Jan 14 '12 at 20:28
  • \$\begingroup\$ Include your code block indented by 4 (more) spaces to get correct formatting. \$\endgroup\$ – Paul Martel Jan 14 '12 at 20:29
  • \$\begingroup\$ I did but the first and last line were not parsed correctly and I don't know why... \$\endgroup\$ – Pella86 Jan 14 '12 at 20:37
  • \$\begingroup\$ Assuming they are at top level (with no indentation in your original source code), leaving a blank line before the first line and having exactly 4 spaces before the opening declaration and the closing brace SHOULD trigger the code block formatter. \$\endgroup\$ – Paul Martel Jan 14 '12 at 20:58
  • \$\begingroup\$ fyi, if you want to figure out formatting issues, you can experiment in the formatting sandbox. \$\endgroup\$ – Paul Martel Jan 14 '12 at 21:04
4
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ about the polymorfism, is a design choice (maybe a bad one), I check the board, if it contains a piece or not, and the class piece just store 2 things, id and color (in my logic the piece doesn't know about the board). So I actually stored the refBoard as an array of pieces. After this, in the program I have a function that fill the board automatically, like the last one line you wrote. An other question, do you know in vim if there is a function to change the tab to 4 spaces? \$\endgroup\$ – Pella86 Jan 14 '12 at 22:28
  • \$\begingroup\$ (on four lines of your ~/.vimrc file) add: set tabstop =4 set softtabstop =4 set shiftwidth =4 set expandtab See thorsanvil.com/vimrc \$\endgroup\$ – Martin York Jan 14 '12 at 23:02
  • \$\begingroup\$ Yes polymorhism is a design choice. But it what makes C++ different to C. Without it you are not writing OO but procedural code and thus not really using C++ (We call what you are doing C with classes). In my opinion your code will be much simpler using polymorhism because you will not need to check the type of a piece (each piece knows its type (and thus is known and checked by the compiler) and can then be asked the action it needs to perform). \$\endgroup\$ – Martin York Jan 14 '12 at 23:10
  • \$\begingroup\$ I agree in general that each piece should derive from a ChessPiece class or similar to check for its own legal moves. It will need to see the whole board to determine if a move is legal: for example in the case of a pawn, if it is moving diagonally it needs to know it is capturing, and moving ahead the square must be unoccupied even by enemy pieces. With the bishop we need to know we are not jumping over anything so b2 to d4 may be legal but won't be if there is a piece on c3. Other things that make moves illegal, e.g. leaving yourself in check, could be for a more "master" class to check. \$\endgroup\$ – CashCow Jan 18 '12 at 11:41
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ thank you for he feedback, but as I said to Loki, I think is a matter of taste if I want to use inheritance or not on a piece. I encoded the piece behaviour in a ChessMoves class ( a class that manage the board and the piece and then passed to the game engine or the AI). I'm using the 0 based indexing idiom. Finally fillcBoard will be used by the GE to look for valid moves, and for the AI to look for the best moves. \$\endgroup\$ – Pella86 Jan 15 '12 at 15:52
  • \$\begingroup\$ With 0-based indexing, if (row == 2) tests for pawns on the third row. \$\endgroup\$ – Paul Martel Jan 15 '12 at 19:04
  • \$\begingroup\$ "Best practices" are not simply "matters of taste". You have to decide whether you want better code or just code that you like better. I'm not sure how much this site can help you with the latter, as that's a much more specialized field. \$\endgroup\$ – Paul Martel Jan 15 '12 at 19:40

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.