Thread: checkers v2 finished

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;
}

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.

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.

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.