checkers v2 finished

i have finished version 2 of checkers i have made it a lot smaller (i think) and added some features to it like colouring the selected piece red. I think the function take piece is a cop-out but i cant think of another way that i can make work without wiered behavour that defies logic.

GitHub - cooper1200/checkers-v2

any feedback appreciated
coop

Have you considered using a graph for your board instead of an array?

Think of a linked list, but instead of next/previous you have 4 pointers; something like NorthWest/NorthEast/SouthWest/SouthEast - Any edge is a NULL, and you can have other members of the square struct such as current contents (use an enum for black, white, or empty), last x amount of move's contents, a member for calculating next moves...

I would have done this from the start as a thinking aid, but generally I like to introduce abstractions, e.g.,

Code:

---

typedef struct
{
    char label[MESSAGE];
} Square;

typedef struct
{
    Square squares[COL_MAX + 1][ROW_MAX + 1];
} Board;

---

This would mean that when you think of a board, you think of it as a 2D array of squares, then when you think of a square, it is clear that the square is denoted by a string label. It does mean that your implementation might be a little more verbose than it currently is though, but in the long run it may be more maintainable.

I wonder if you actually need the label to be a string: from what I see, the labels are basically strings of length 1, so perhaps they could just be chars.

I would suggest writing initialize_game like this:

Code:

---

enum player initalize_game(char board[][ROW_MAX +1][MESSAGE])
{
    enum player current_player = PLAYER_WHITE;

    int x_coordinate, y_coordinate;

    for (y_coordinate = ROW_MIN; y_coordinate < 3; y_coordinate++)
    {
        for (x_coordinate = COLUMN_MIN; x_coordinate <= COLUMN_MAX; x_coordinate++)
        {
            update_board_array(board, WHITE_PIECE, x_coordinate, y_coordinate);
        }
    }

    for (y_coordinate = 3; y_coordinate < 5; y_coordinate++)
    {
        for (x_coordinate = COLUMN_MIN; x_coordinate <= COLUMN_MAX; x_coordinate++)
        {
            update_board_array(board, EMPTY_SQUARE, x_coordinate, y_coordinate);
        }
    }

    for (y_coordinate = 5; y_coordinate <= ROW_MAX; y_coordinate++)
    {
        for (x_coordinate = COLUMN_MIN; x_coordinate <= COLUMN_MAX; x_coordinate++)
        {
            update_board_array(board, BLACK_PIECE, x_coordinate, y_coordinate);
        }
    }

    return current_player;
}

---

Instead of int, I used enum player as the return type because you conveniently declared the enum, so you might as well use it. Notice that I turned the current_player parameter into a local variable: you're always setting it to PLAYER_WHITE, so it is useless as a paramter. Then, I wrote three loops instead of one, and got rid of the piece_colour array as you can use the string constants directly (which is good because it avoids repeated copying).

I suggest that you keep your use of the conditional operator to a minimum. Notice that one restriction of the conditional operator is that the second and third operands must have the same, or compatible, types. Consequently, it is usually used when you want to make use of its result, i.e., to conditionally select one of two possible results. So, you might see it frequently used when you want to conditionally assign to a variable, or in other similiar contexts like say in a printf call to conditionally choose a value to print. If you aren't using the result of the conditional operator, I suggest thinking twice. If the operands are verbose, you should also avoid the conditional operator, e.g. in this double whammy:

Code:

---

                    current_player ? printf(" %d *********  %s  *********  %s  *********  %s  *********  %s  *\n",y_coordinate,
                                    temp_array[7-1][7-y_coordinate], temp_array[7-3][7-y_coordinate],
                                    temp_array[7-5][7-y_coordinate], temp_array[7-7][7-y_coordinate]) :
                            printf(" %d *********  %s  *********  %s  *********  %s  *********  %s  *\n",y_coordinate,
board[1][y_coordinate], board[3][y_coordinate], board[5][y_coordinate], board[7][y_coordinate]);

---

which would have been much more readable if it were:

Code:

---

                    if (current_player)
                    {
                        printf(" %d *********  %s  *********  %s  *********  %s  *********  %s  *\n",
                            y_coordinate,
                            temp_array[7-1][7-y_coordinate], temp_array[7-3][7-y_coordinate],
                            temp_array[7-5][7-y_coordinate], temp_array[7-7][7-y_coordinate]);
                    }
                    else
                    {
                        printf(" %d *********  %s  *********  %s  *********  %s  *********  %s  *\n",
                            y_coordinate,
                            board[1][y_coordinate], board[3][y_coordinate],
                            board[5][y_coordinate], board[7][y_coordinate]);
                    }

---

As I noted elsewhere, you really don't need to be so verbose:

Code:

---

return (strcmp(board[x_coordinate][y_coordinate], EMPTY_SQUARE) == 0) ? true : false;

---

just write:

Code:

---

return strcmp(board[x_coordinate][y_coordinate], EMPTY_SQUARE) == 0;

---

The result of the == operator is already a boolean value even though it is of type int, so you don't have to use the conditional operator to decide whether or not to return true or false. If you insist, then I insist that instead of this:

Code:

---

if (off_board(*p_x, *p_y) == false)

---

you should write:

Code:

---

if ((off_board(*p_x, *p_y) == false) ? true : false)

---

absurd, isn't it?

Speaking of which, that should be:

Code:

---

if (!off_board(*p_x, *p_y))

---

Also, don't do this:

Code:

---

scanf(" %1d", &*p_x);

---

just write:

Code:

---

scanf(" %1d", p_x);

---

but remember to check the return value of scanf.

In calculate_move, this:

Code:

---

if (take_piece(board, is_king, piece_coordinate_x, piece_coordinate_y, move_coordinate_x, move_coordinate_y, current_player) == true)
{
    return true;
}
else
{
    return false;
}

---

can be simplified to:

Code:

---

return take_piece(board, is_king, piece_coordinate_x, piece_coordinate_y, move_coordinate_x, move_coordinate_y, current_player);

---

In take_piece, this:

Code:

---

if ((entered_x_coordinate - temp_x == 1 * multiplyer_x)  && (entered_y_coordinate - temp_y == 1 * multiplyer_y))
{
    if (validate_piece(temp_board, piece_to_take, entered_x_coordinate, entered_y_coordinate))
    {
        if (off_board((temp_x + 2 * multiplyer_x), (temp_y + 2 * multiplyer_y)) == false)
        {
            if (valid_square(temp_board, temp_x + 2 * multiplyer_x, temp_y + 2 * multiplyer_y))
            {
                strcpy(temp_board[temp_x + 1 * multiplyer_x][temp_y + 1 * multiplyer_y], TAKEN_PIECE);
                temp_x += 2 * multiplyer_x;
                temp_y += 2 * multiplyer_y;
            }
            else
            {
                break;
            }
        }
        else
        {
            break;
        }
    }
    else
    {
        break;
    }
}
else
{
    break;
}

---

can be simplified to:

Code:

---

if ((entered_x_coordinate - temp_x == 1 * multiplyer_x) &&
    (entered_y_coordinate - temp_y == 1 * multiplyer_y) &&
    validate_piece(temp_board, piece_to_take, entered_x_coordinate, entered_y_coordinate) &&
    !off_board(temp_x + 2 * multiplyer_x, temp_y + 2 * multiplyer_y) &&
    valid_square(temp_board, temp_x + 2 * multiplyer_x, temp_y + 2 * multiplyer_y))
{
    strcpy(temp_board[temp_x + 1 * multiplyer_x][temp_y + 1 * multiplyer_y], TAKEN_PIECE);
    temp_x += 2 * multiplyer_x;
    temp_y += 2 * multiplyer_y;
}
else
{
    break;
}

---

Wow thank you for you for the time you have taken to respond. The reason message is an array is so i can include the escape codes for changing the colour of the piece on the screen, which i guess is a little childish and probably added dozens of lines of code. with the deceleration of the struct board are u declaring an array of the square struct? and would i address the inner member by strcpy(board.square[x][y].label, "hello there").

writing an if clause like if (my_function == true) helps at time of debugging that i am expecting it to return false and remembering what i wanted the function to return when writing the function call. I guess this is a bad habit of mine as your way of using the not operator is just as clear in terms of is it going to return false or true to pass the test.

what is clear from your comments is i need to really think about what each statement is doing your example of me using the ?: operator on a return statement is classic of what i mean. i didnt think about what strcmp was returning so used a conditional operator to determin weather it was true or false. BUT the logic of that use says that strcmp is returning true or false duh!!!
many many thanks for your input
coop

Consider that you may be inclined to read off_board(*p_x, *p_y) == false as "if off_board returns false for the piece at those coordinates", upon which you still have to interpret the significance of that, whereas !off_board(*p_x, *p_y) lends itself to "if the piece at those coordinates is not off the board", i.e., if you have descriptive function names, especially those involving verbs, it can make your code more readable in this way.

sorry meant to ask rather than having the nested if statements in take piece you would join them together using the && operator. would you do this after you were sure the function worked as you wanted or right from the start if so how would you debug it to find out which part was causing the entire clause to fail.

First, test each function to be sure that they work (unit testing). Then test them together (integration testing). Use a debugger to help you figure out which one returned an unexpected value.

i tried ti impliment declaring the player enum in initialize_game and altered the decelerations to suit but now when i run it throws an error saying current_player undeclared

Code:

---


int main()
{
    char board[COLUMN_MAX + 1][ROW_MAX + 1][MESSAGE], code[ESCAPE_CODE], error_message[ERROR_MESSAGE];
    char *p_make_king;
    int x_coordinate_of_piece, y_coordinate_of_piece, x_coordinate_to_move_to, y_coordinate_to_move_to;
    int number_of_white_pieces_left, number_of_black_pieces_left;
    bool is_king = false;

    current_player = initalize_game(board);
    clear_screen();
    draw_board(board, current_player);
    //move_test(board);
    //current_player = PLAYER_WHITE;
    for(;;)

---

coop

That's right. Look in the main function: where did you declare current_player?

Don't say you declared it in initialize_game: yes, you did, but that's a local variable.