Thread: Checkers program feedback wanted - Jumping into C++ exercise

Hi,

I'm learning C++ and am looking for some feedback on my checkers program. It works, but I just want to learn from your wisdom and find out if I have any bad coding habits to fix, tips, shortcuts etc...

The question from the book:

Chapter 10: Arrays

Make a two-player checkers program that allow each player to make a move, and checks for legal moves and whether the game is over. Be sure to support kinging! Feel free to add support for any house rules that you use when you play. Consider allowing the user to choose the type of rules at program startup.

My solution:

Code:

#include <iostream>
#include <cstdlib>
#include <vector>

using namespace std;

void move(vector<vector<char>>& board, int player);
bool isLegal(vector<vector<char>>& board, int player, int x, int y, int j, int i);
bool isKing(vector<vector<char>>& board, int player);
bool isGameOver(vector<vector<char>>& board);
vector<int> findJumpPos(vector<vector<char>>& board, int player);
void jump(vector<vector<char>>& board, vector<int>& pos);
void intialiseBoard(vector<vector<char>>& board);
void displayBoard(vector<vector<char>>& board);

int main()
{
    // Create and deploy the game board
    vector<vector<char>> board(8, vector<char> (8));
    intialiseBoard(board);

    // Declare and initialise the number of players
    int player = 2;

    while(!isGameOver(board)) {
        cout << "CHECKERS" << endl;
        cout << "--------"<< endl;
        if(player == 2)
            player = 1;
        else
            player = 2;

        displayBoard(board);
        move(board, player); // Let the active player make their move
    }

    cout << endl << "Congratulations player " << player << "! You win." << endl;
}

// Allows the given player to make a move
void move(vector<vector<char>>& board, int player)
{
    bool isJump = false;
    vector<int> jumpPos = findJumpPos(board, player);

    // If there's a jump force current player to take it
    while(jumpPos.at(0) != 9) { // 9 indicates no jumps were found
        char jumpedPiece = board[jumpPos[2]][jumpPos[3]];
        isJump = true;
        jump(board, jumpPos);
        // Check for kings
        if((player == 1) && (isKing(board, player)))
            board[jumpPos[4]][jumpPos[5]] = 'K';
        else if((player == 2) && (isKing(board, player)))
            board[jumpPos[4]][jumpPos[5]] = 'Q';
        // Update the board
        system("CLS");
        cout << "CHECKERS" << endl;
        cout << "--------"<< endl;
        cout << endl << "Player " << player << ", you jumped a " << jumpedPiece;
        cout << " piece at " << static_cast<char> (jumpPos[3] + 65) << jumpPos[2] << endl << endl;
        displayBoard(board);
        cout << "Press enter to continue...";
        cin.ignore(256, '\n');
        cin.get();
        // Check if there is a double jump
        jumpPos = findJumpPos(board, player);
    }

    if(isJump)
        return;

    int invalidMove = 0;
    int x, y, j, i;
    char xTmp, jTmp;

    do {
        system("CLS");
        cout << "CHECKERS" << endl;
        cout << "--------"<< endl;

        if(++invalidMove > 1) {
            cout << endl << "Illegal move! Try again..." << endl << endl;
        }

        displayBoard(board);

        cout << endl << "Player " << player << " - " << endl;
        cout << "Choose X position of your piece -> ";
        cin >> xTmp;
        cout<< "Choose Y position of your piece -> ";
        cin >> y;

        x = static_cast<int> (xTmp - 65);

        cout << endl << "Choose X position of your chosen move -> ";
        cin >> jTmp;
        cout<< "Choose Y position of your chosen move -> ";
        cin >> i;

        j = static_cast<int> (jTmp - 65);
    } while(!isLegal(board, player, x, y, j, i));
    // Make the move
    board[i][j] = board[y][x];
    board[y][x] = 0;
    // Check for Kings
    if((player == 1) && (isKing(board, player)))
        board[i][j] = 'K';
    else if((player == 2) && (isKing(board, player)))
        board[i][j] = 'Q';
}

// Returns true when a player move's positioning is legal
bool isLegal(vector<vector<char>>& board, int player, int x, int y, int j, int i)
{
    bool legal = false;

    if((board[y][x] != 0) && (board[i][j] == 0)) {
        if((player == 1) && (board[y][x] == 'R') && ((j == (x + 1)) || j == ((x - 1))) && (i == (y + 1)))
            legal = true;
        else if((player == 2) && (board[y][x] == 'B') && ((j == (x + 1)) || j == ((x - 1))) && (i == (y - 1)))
            legal = true;
        else if((player == 1) && (board[y][x] == 'K') && ((j == (x + 1)) || j == ((x - 1))) && (i == (y + 1) || (i == (y - 1))))
            legal = true;
        else if((player == 2) && (board[y][x] == 'Q') && ((j == (x + 1)) || j == ((x - 1))) && (i == (y - 1) || (i == (y + 1))))
            legal = true;
    }

    return legal;
}

// Returns true when there is a new king
bool isKing(vector<vector<char>>& board, int player)
{
    bool king = false;

    for(int i = 0; i < board.size(); i++) {
        if((player == 1) && (board[7][i] == 'R'))
            king = true;
        else if((player == 2) && (board[0][i] == 'B'))
            king = true;
    }

    return king;
}

// Returns true when a player has won
bool isGameOver(vector<vector<char>>& board)
{
    bool over = false;

    int p1PiecesLeft = 0;
    int p2PiecesLeft = 0;

    for(int i = 0; i < board.size(); i++) {
        for(int j = 0; j < board.size(); j++) {
            if((board[i][j] == 'R') || (board[i][j] == 'K'))
                p1PiecesLeft++;
            else if((board[i][j] == 'B') || (board[i][j] == 'Q'))
                p2PiecesLeft++;
        }
    }

    if(p1PiecesLeft == 0)
        over = true;
    else if(p2PiecesLeft == 0)
        over = true;

    return over;
}

// Returns coordinates of the concerned grid positions when a player is obliged to jump
vector<int> findJumpPos(vector<vector<char>>& board, int player)
{
    vector<int> jumpPos(6, 9);

    for(int i = 0; i < board.size(); i++) {
        for(int j = 0; j < board.size(); j++) {
            if((player == 1) && (board[i][j] == 'R') && (i < 6) && (board[i + 1][j - 1] == 'B' || board[i + 1][j - 1] == 'Q') && (board[i + 2][j - 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i + 1);
                jumpPos[3] = (j - 1);
                jumpPos[4] = (i + 2);
                jumpPos[5] = (j - 2);
            } else if((player == 1) && (board[i][j] == 'R') && (i < 6) && (board[i + 1][j + 1] == 'B' || board[i + 1][j + 1] == 'Q') && (board[i + 2][j + 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i + 1);
                jumpPos[3] = (j + 1);
                jumpPos[4] = (i + 2);
                jumpPos[5] = (j + 2);
            } else if((player == 2) && (board[i][j] == 'B') && (i > 1) && (board[i - 1][j - 1] == 'R' || board[i - 1][j - 1] == 'K') && (board[i - 2][j - 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i - 1);
                jumpPos[3] = (j - 1);
                jumpPos[4] = (i - 2);
                jumpPos[5] = (j - 2);
            } else if((player == 2) && (board[i][j] == 'B') && (i > 1) && (board[i - 1][j + 1] == 'R' || board[i - 1][j + 1] == 'K') && (board[i - 2][j + 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i - 1);
                jumpPos[3] = (j + 1);
                jumpPos[4] = (i - 2);
                jumpPos[5] = (j + 2);
            } else if((player == 1) && (board[i][j] == 'K') && (i > 1) && (board[i - 1][j - 1] == 'B' || board[i - 1][j - 1] == 'Q') && (board[i - 2][j - 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i - 1);
                jumpPos[3] = (j - 1);
                jumpPos[4] = (i - 2);
                jumpPos[5] = (j - 2);
            } else if((player == 1) && (board[i][j] == 'K') && (i > 1) && (board[i - 1][j + 1] == 'B' || board[i - 1][j + 1] == 'Q') && (board[i - 2][j + 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i - 1);
                jumpPos[3] = (j + 1);
                jumpPos[4] = (i - 2);
                jumpPos[5] = (j + 2);
            } else if((player == 1) && (board[i][j] == 'K') && (i < 6) && (board[i + 1][j - 1] == 'B' || board[i + 1][j - 1] == 'Q') && (board[i + 2][j - 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i + 1);
                jumpPos[3] = (j - 1);
                jumpPos[4] = (i + 2);
                jumpPos[5] = (j - 2);
            } else if((player == 1) && (board[i][j] == 'K') && (i < 6) && (board[i + 1][j + 1] == 'B' || board[i + 1][j + 1] == 'Q') && (board[i + 2][j + 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i + 1);
                jumpPos[3] = (j + 1);
                jumpPos[4] = (i + 2);
                jumpPos[5] = (j + 2);
            } else if((player == 2) && (board[i][j] == 'Q') && (i < 6) && (board[i + 1][j - 1] == 'R' || board[i + 1][j - 1] == 'K') && (board[i + 2][j - 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i + 1);
                jumpPos[3] = (j - 1);
                jumpPos[4] = (i + 2);
                jumpPos[5] = (j - 2);
            } else if((player == 2) && (board[i][j] == 'Q') && (i > 1) && (board[i - 1][j - 1] == 'R' || board[i - 1][j - 1] == 'K') && (board[i - 2][j - 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i - 1);
                jumpPos[3] = (j - 1);
                jumpPos[4] = (i - 2);
                jumpPos[5] = (j - 2);
            } else if((player == 2) && (board[i][j] == 'Q') && (i < 6) && (board[i + 1][j + 1] == 'R' || board[i + 1][j + 1] == 'K') && (board[i + 2][j + 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i + 1);
                jumpPos[3] = (j + 1);
                jumpPos[4] = (i + 2);
                jumpPos[5] = (j + 2);
            } else if((player == 2) && (board[i][j] == 'Q') && (i > 1) && (board[i - 1][j + 1] == 'R' || board[i - 1][j + 1] == 'K') && (board[i - 2][j + 2] == 0)) {
                jumpPos[0] = (i);
                jumpPos[1] = (j);
                jumpPos[2] = (i - 1);
                jumpPos[3] = (j + 1);
                jumpPos[4] = (i - 2);
                jumpPos[5] = (j + 2);
            }
        }
    }

    return jumpPos;
}

// Forces a jump if there is one to be made
void jump(vector<vector<char>>& board, vector<int>& pos)
{
    if(board[pos[0]][pos[1]] == 'R') {
        board[pos[0]][pos[1]] = 0;
        board[pos[2]][pos[3]] = 0;
        board[pos[4]][pos[5]] = 'R';
    } else if(board[pos[0]][pos[1]] == 'B') {
        board[pos[0]][pos[1]] = 0;
        board[pos[2]][pos[3]] = 0;
        board[pos[4]][pos[5]] = 'B';
    } else if(board[pos[0]][pos[1]] == 'Q') {
        board[pos[0]][pos[1]] = 0;
        board[pos[2]][pos[3]] = 0;
        board[pos[4]][pos[5]] = 'Q';
    } else if(board[pos[0]][pos[1]] == 'K') {
        board[pos[0]][pos[1]] = 0;
        board[pos[2]][pos[3]] = 0;
        board[pos[4]][pos[5]] = 'K';
    }
}

// Initialises the game board
void intialiseBoard(vector<vector<char>>& board)
{
    // Player 1 side
    for(int i = 0; i < 3; i++) {
        for(int j = 0; j < board.size(); j++) {
            if((j % 2) == 0) {
                if((i % 2) == 0) {
                    board[i][j] = 'R';
                } else {
                    board[i][(j + 1)] = 'R';
                }
            }
        }
    }
    // Player 2 side
    for(int i = 0; i < 3; i++) {
        for(int j = 0; j < board.size(); j++) {
            if((j % 2) == 0) {
                if((i % 2) != 0) {
                    board[(i + 5)][j] = 'B';
                } else {
                    board[(i + 5)][(j + 1)] = 'B';
                }
            }
        }
    }
}

// Prints the board to the screen
void displayBoard(vector<vector<char>>& board)
{
    // X positions
    for(int k = 0; k < board.size(); k++)
        cout << "    " << static_cast<char> (k + 65);

    cout << endl << endl;
    // Y positions
    for(int i = 0; i < board.size(); i++) {
        cout << i << "  ";
        for(int j = 0; j < board.size(); j++) {
            if(board[i][j] != 0)
                cout << "[ " << board[i][j] << " ]";
            else
                cout << "[   ]";
        }
        cout << endl << endl;
    }
}

Thank you!

The biggest thing I notice is that there appears to be a lot of code duplication that could easily be reduced. For example:

Code:

// Forces a jump if there is one to be made
void jump(vector<vector<char>>& board, vector<int>& pos)
{
    if(board[pos[0]][pos[1]] == 'R') {
        board[pos[0]][pos[1]] = 0;
        board[pos[2]][pos[3]] = 0;
        board[pos[4]][pos[5]] = 'R';
    } else if(board[pos[0]][pos[1]] == 'B') {
        board[pos[0]][pos[1]] = 0;
        board[pos[2]][pos[3]] = 0;
        board[pos[4]][pos[5]] = 'B';
    } else if(board[pos[0]][pos[1]] == 'Q') {
        board[pos[0]][pos[1]] = 0;
        board[pos[2]][pos[3]] = 0;
        board[pos[4]][pos[5]] = 'Q';
    } else if(board[pos[0]][pos[1]] == 'K') {
        board[pos[0]][pos[1]] = 0;
        board[pos[2]][pos[3]] = 0;
        board[pos[4]][pos[5]] = 'K';
    }
}

The first two lines in each section appear to be the same so it could possibly be reduced to:

Code:

// Forces a jump if there is one to be made
void jump(vector<vector<char>>& board, vector<int>& pos)
{
    board[pos[0]][pos[1]] = 0;
    board[pos[2]][pos[3]] = 0;

    if(board[pos[0]][pos[1]] == 'R') {
        board[pos[4]][pos[5]] = 'R';
    } else if(board[pos[0]][pos[1]] == 'B') {
        board[pos[0]][pos[1]] = 0;
    } else if(board[pos[0]][pos[1]] == 'Q') {
        board[pos[4]][pos[5]] = 'Q';
    } else if(board[pos[0]][pos[1]] == 'K') {
        board[pos[4]][pos[5]] = 'K';
    }
}

Which could possibly be possibly be reduced to:

Code:

void jump(vector<vector<char>>& board, vector<int>& pos)
{
    board[pos[0]][pos[1]] = 0;
    board[pos[2]][pos[3]] = 0;
    board[pos[4]][pos[5]] = board[pos[0]][pos[1]];

    if(board[pos[0]][pos[1]] == 'B') {
        board[pos[0]][pos[1]] = 0;
}

There are several other places where you could probably reduce the duplication with a little thought.

Jim

I feel like it'd be a lot easier for everyone if you instead wrote about your logic for the program itself. What were you going to design and how were you going to design it? Describe your algorithms and data structures.

Another thing you could do would be to better explain in the code what your magic constants mean. For example, at first I thought jimblumberg's example would break the application because it makes changes even if board[pos[0]][pos[1]] isn't 'R', 'B', 'Q' or 'K'. But then I thought maybe those are the only possible values, so it's "ok". I tried to find what those constants meant in the code and didn't see it, so I don't know if that works or not.

The main thing I will take away from this topic is that I need to comment better and perhaps properly document my program logic/algorithms if I intend to post it on here, in future

I don't know if it's just me or not but I go to a lot of different programming forums and I've gotten, well, lazier in my time.

I typically don't read code unless it's very well-commented or is very short and plays well with its usage of whitespace. Otherwise, you get these really dense segments of code that make perfect sense to the creator but to anyone else, it seems like something they need to trudge through.

Also, always make sure you use self-documenting variable names. No matter how long they are. Me being able to read your code and understanding it is 50x more important than the inconvenience of you having to type a few more characters.

But I advocate that it's easier for people in forums if you just simply write about your approach with something like, "I was hoping to do X, Y and Z using algorithms A, B and data structures C and D."