Thread: checkers game finally finished

i have finally finished the checkers program. I have attached(i hope) the source code so feel free to have a look and let me know what you think.

Many many thanks to all those on here that have helped me

coop

I suppose we can start with a small function.

Code:

bool toggle_turn(void)
{
    static bool flag;

    if (flag == true)
    {
        flag = false;
    }
    else
    {
        flag = true;
    }

    return flag;
}

First off, it's more succinctly written like this.

Code:

bool toggle_turn(void) {
  static bool flag = false;
  flag = !flag;
  return flag;
}

But the real issue here is clarity. What does this function do? What is flag? What is the return value? Naming is very important, and I think both the names here are bad. In particular the name "flag" is very bad. Variable names should never be quite so generic. If it's a flag, tell me what the flag does in its name.

I'd create an enum of more meaningful symbols that represent the output of this function and give the function itself a better name. Remember that programming is communication, both with the compiler and the reader of the program. You'd successfully communicated what you want to the compiler, but I think the human readability of this function needs improvement. Also, remember that you're communicating with future you. Imagine writing a program that's in use at a company for 5 years before it needs maintenance. If you read this program 5 years from now (or even 5 months from now), are you necessarily going to understand what this function does? Because you're likely not going to remember what was in your head while you wrote it.

Code:

enum { TURN_WHITE=0, TURN_BLACK=1 };
int next_player_turn(void) {
  static int turn = PLAYER_WHITE;
  turn ^= 1;
  return turn;
}

This is the same function. I've defined enum symbols and made sure their values are 0 and 1. I've give the variable a more clear name and made clear that it starts on white.

The ^= 1 trick is the same as toggling the first bit on and off, it's the same as what you're doing in your code, or what I did with the ! operator above. However, to me, this is more clear that it's just toggling the least significant bit. This part is probably a matter of taste, I just prefer it that way.

Overall, I don't think this should be a static variable in a function, either. This hides a part of the state away in a function where it can't be examined without changing it. I think this speaks more to the overall structure and organization of your program in general, and that can't be addressed easily in a short post.

Just an addendum on what @laserlight said:

Did you consider pepole who don't use Code::Blocks? Sure, your project is simple enough to be compiled manually.
Consider learning to create a Makefile.

thanks for the feed back i did think people might not want to download a file. but equally didn't think i should be posting 1000 lines of code in one hit.

here is the link for git hub GitHub - cooper1200/checkers1

Gaxio: I guess as you noticed i tried to write each function in a way that what ever calls it doesn't care how it gets the result (with the exception of get coordinates). for a real world example if i go to a shop and use my debit card to buy something i dont want the shop knowing all my personal info like debits and credits what the balance is or what my pin number is. all i want them to know is that i have the funds to pay for the goods. is this a bad approach to writing functions as far as modular programming goes?

Originally Posted by cooper1200

I guess as you noticed i tried to write each function in a way that what ever calls it doesn't care how it gets the result (with the exception of get coordinates). for a real world example if i go to a shop and use my debit card to buy something i dont want the shop knowing all my personal info like debits and credits what the balance is or what my pin number is. all i want them to know is that i have the funds to pay for the goods. is this a bad approach to writing functions as far as modular programming goes?

Right, this is the concept of encapsulation. However, there are no clear lines between any of files in this program, and this make the entire concept of encapsulation kinda fuzzy here.

I should have looked at how toggle_turn was being called, because it's an even bigger problem. You call it and store its return value in whos_turn. You're duplicating the state of whose turn it is in the whos_turn variable and the static variable hidden inside a function. This is quite confusing. Honestly, I would prefer a very terse implementation of this entire thing.

Code:

enum { PLAYER_WHITE = 0, PLAYER_BLACK = 1 };
int current_player = PLAYER_BLACK;

int next_player(int player) {
  return player ^ 1;
}

That's it. The state is in one place and the logic for who goes next is abstract in a function. The function doesn't update the state of whos_turn, functions with side effects bury state changes and should be avoided where appropriate. So now when you want the next player to go, you just need to say this:

Code:

current_player = next_player(current_player);

It's clear that you want to get the player that should go after this player and it's clear what state is being updated.

Now that you've got the program working, it's time to start making small incremental changes. Figure out what you can do to make the program shorter and more clear, make the improvement, commit the changes. Using git to your advantage, if you ever really screw up you can always just revert to the last commit. I just picked that one function out at random, so let me grab another one that looks fishy.

Code:

int find_piece(int x, int y) {
    int i, found = -1;

    if (whos_turn == false)
    {
        for (i = 0; i < 12; i++)
        {
            if ((white[i].position_x == x) && (white[i].position_y == y))
            {
                found = i; //code this better ie return i
                break;
            }
        }
    }
    else
    {
        for (i = 0; i < 12; i++)
        {
            if ((black[i].position_x == x) && (black[i].position_y == y))
            {
                found = i;
                break;
            }
        }
    }

    return found;
}

This function seems completely unnecessary. It looks to me that you're using the wrong data structure to represent the pieces. The primary action that's done on a board game is to set and get pieces on the board, and if you're constantly having to iterate through these lists to look up pieces then the whole thing becomes a bit convoluted. From your previous posts, I had thought you were storing the board in an 8x8 array, which would render this function unnecessary. The entire function could be replaced with board[y][x]. And on further examination you seem to have a piece_position 8x8 array that's already doing just this. This is duplicating the program state again, which just makes you write more code and introduces bugs when the split state gets out of sync.

The function is also repetitive. Any time you find yourself repeating almost identical code like this, there's probably a better way. Further, you have find_taken_piece, a function that's almost identical to find_piece. At a glance they appear to do the same exact thing. Things like this should be setting off alarm bells when you're writing them. They're a clue that there's probably something wrong, and are referred to as "code smells."

To address this whole issue, I would think about removing piece_struct entirely, as well as white and black piece arrays. That data is already available in piece_position, which I would rename to board. You've made this more complicated than it needs to be by splitting the state. Just looking quickly, I think you could eliminate several functions and about 100 lines of code that are unnecessary. Functions like valid_square and check_if_king just aren't necessary if you store that information on the board. It's a valid square if it's not a '!' character (which you store on the light squares when you initialize the board to indicate you can't move there) and it's a king if isupper(board[y][x]), that kind of thing. These functions can either be eliminated or turned into one-liners.

There are other issues I'm seeing, but to give you an idea of the scale of the problems, I'd expect a checkers game to be about 300 lines of code. If you want to continue working on this project, I think you should be able to get it down into that range. And I know I'm leveling a whole bunch of criticism at your code, but don't get discouraged. This is great for where you are right now. Keep going.

i haven't take your comments as negative your showing me where i have gone wrong and making suggestions as to how i can improve. That is the whole point of the project. There is no point in me sitting here reading a book and doing a few worked examples. Yes that teaches coding to a point with examples giving practice on a particular topic but doesn't encapsulate them or give any real world experience.

I guess some of the issue is that as i wrote this my understanding or ways round things grew. Also thinking out of the box as to how a player could cheat. one of the last things i added was checking that if taking 2 or more pieces that all of the jumps were on the board,

Also from your suggestions i don't seem to be grasping the use of functions. if a function is to do only one thing then it surely has to call other functions to "help" it. For example main just calls functions one after the other. one of which is the function play game(). in the function play game() i have to get co-ordinates of the piece to be moved and coordinates of where it is to be moved to, then have functions that decide how its getting from the first set to the second set of co-ordinates like move_piece or take_piece i also have to have functions that decide on the legality of the move. Kings can move up and down the board i need different functions to check the legality of that move. I did notice last night that the crux of move_king was the same as move_piece i.e. it just moved one square. However i would still need a function to calculate if the move was valid.

from your suggestion rather than having an array of illegal squares (white squares) i could simply have the illegal squares marked with an "!" but from the above understanding of functions i would need a function to check if it was a "!" or an empty square unless of course i add another nested if statement but that would negate the one job rule.

many thanks
coop

many thanks
coop

Originally Posted by laserlight

Avoid magic numbers: this means that your many uses of 8 and 12 should be named constants instead, and likewise that very magic number 99.

i take your point about the magic numbers it would of been more meaning full to have a #define SURRENDER_CODE = 99 and then used that (or any other number above 64 as i needed the numbers 1 to 64 to return 2 co-ordinates in one variable). rather than a comment saying what it is.

Originally Posted by laserlight

The functions declared in a header file form an interface (or part thereof) intended to be called by functions outside of the functions that form the interface, e.g., the main function. So, helper functions should not be declared in a header; declare them in the source file, and you might even declare them as static so that their names have internal linkage.

i seem to be misunderstanding the use of header files and other files therein. The way i understand it is that the compiler/linker needs to know a prototype of each function and where the function is so one has to declare a prototype in the header file and then a separate source file has the function in. hence including moves.h in moves.c. for an example of what i mean if i use a printf statement i haven't told the compiler what printf is. i have to include the appropriate header file that has the prototype of printf in it which in turn is included in a source file somewhere. From your comment it appears that i have this wrong.

Originally Posted by cooper1200

from your suggestion rather than having an array of illegal squares (white squares) i could simply have the illegal squares marked with an "!" but from the above understanding of functions i would need a function to check if it was a "!" or an empty square unless of course i add another nested if statement but that would negate the one job rule.

One thing you'll find yourself doing when writing programs, especially simulations like this, is that they generally follow the same process.

• Data: Select a data structure that represents the state of the simulation. In this case, the 8x8 char array.
• Abstractions: Write a series of functions that operate on that data structure. A lot of these will be short one-liners, like get_piece, set_piece, is_black_square, is_empty_square, etc. Don't go crazy making too many of these, it's easy to get carried away making functions you'll never use. I usually skip to the next step and write these functions as I need them.
• Solution: Write a function or set of functions that uses these functions to solve the problem. For example, your validate_move function uses these functions to systematically check each way in which a move could fail (tried to move to a white square or off the board, tried to move on top of another piece), etc using functions is_black_square and get_piece, etc. Ideally by the time you're writing the functions that actually implement the game, you're not touching the data structures except through the functions from step 2.

It also tends to organize the source code of smaller programs that comfortably fit in one file quite nicely. The constants and types are at the top, then the data, then the functions that operate on the data, then the functions that implement the simulation, then the function that runs the simulation and generates the output, and finally the main function way at the bottom. Note how nothing has to look down, the functions that operate on the data don't need to call the functions that implement the simulation.

It would be good practice to take everything from step 1 and 2 and put them in a file. The game would be separated into board.c and main.c, with a board.h file that has prototypes of all the relevant functions from board.c. As it is right now you have almost everything crammed into moves.c with almost nothing in the other files. It's hard to decide what to put in each file at first, but once you think in terms of abstractions, functions will generally tend to group themselves into tight clusters that work together to solve some smaller problem within the program. These are the functions and data that need to be grouped together in a .c file with a defined interface in the .h file.

im not understanding what your saying i think. if i have a function that deals with moving a piece i have to determine what colour it is (w or b in this case). the operations on that piece would depend if its moving 1 square taking an opponents piece(s) or removing it from the array all dependant on the colour of the piece. Hence the 2 functions i have for find_piece and find_taken_piece both depend on being either white or black. ie find_piece has to look through whites struct array where as find_taken_piece has to look through blacks assuming of course its whites turn.

many thanks
coop

is this any better or am i still being thick??

Code:

#include <stdio.h>
#include <stdlib.h>

#define ROW_MIN 0
#define ROW_MAX 7
#define COLUMN_MIN 0
#define COLUMN_MAX 7

void draw_board(char board[][8]);
void piece_placement(char board[][8], char valid_square, char not_valid_square, int column, int row);

int main()
{
    int column, row;
    char board[COLUMN_MAX + 1][ROW_MAX +1], valid_square, not_valid_square = '!';

    for (row = ROW_MIN; row <= ROW_MAX; row++)
    {
        for (column = COLUMN_MIN; column <= COLUMN_MAX; column++)
        {
            if (row < 3) // row is less than 3 so dealing with white pieces
            {
                valid_square = 'w';
                piece_placement(board, valid_square, not_valid_square, column, row);
            }
            else if ((row > 2) && (row < 5))
            {
                valid_square = ' '; // a space
                piece_placement(board, valid_square, not_valid_square, column, row);
            }
            else // row is 5 or more so dealing with black pieces
            {
                valid_square = 'b';
                piece_placement(board, valid_square, not_valid_square, column, row);
            }
        }
    }
    draw_board(board);
    return 0;
}

void piece_placement(char board[][ROW_MAX + 1], char valid_square, char not_valid_square, int column, int row)
{
    if ((row % 2 == 0) && (column % 2 == 0)) // row and column are even so place a piece
    {
        board[column][row] = valid_square;
    }
    else if ((row % 2 == 0) && (column % 2 != 0)) // row is even but column is odd so illegal square
    {
        board[column][row] = not_valid_square;
    }
    else if ((row % 2 != 0) && (column % 2 == 0)) // row is odd but column is even so illegal square
    {
        board[column][row] = not_valid_square;
    }
    else   //row and column are odd so place a piece
    {
        board[column][row] = valid_square;
    }
}

void draw_board(char board[][ROW_MAX + 1])
{
    int column, row;

    for (row = ROW_MAX; row >= ROW_MIN; row--)
    {
        for (column = COLUMN_MIN; column <= 3; column++)
        {
            if (column == 0)
            {
                printf("   *****************************************************************\n");
            }
            else if (row % 2 == 0)
            {
                if (column % 2 == 0)
                {
                    printf(" %d *   %c   *********   %c   *********   %c   *********   %c   *********\n", row, board[0][row], board[2][row], board[4][row], board[6][row]);
                }
                else
                {
                    printf("   *       *********       *********       *********       *********\n");
                }
            }
            else
            {
                if (column % 2 == 0)
                {
                    printf(" %d *********   %c   *********   %c   *********   %c   *********   %c   *\n", row, board[1][row], board[3][row], board[5][row], board[7][row]);
                }
                else
                {
                    printf("   *********       *********       *********       *********       *\n");
                }
            }
        }
    }
    printf("   *****************************************************************\n");
    printf("       0        1       2      3       4       5       6       7    \n");
}

many thanks
coop

i can see that i can use the piece_placement function for moving pieces around however is there a way of declaring a parameter optional or would i always have to pass a char for the not_valid_square?

never mind i can do a #define as it will never change so don't need to pass it around
coop

Code:

void piece_placement(char board[][ROW_MAX + 1], char valid_square, char not_valid_square, int column, int row)
{
    if ((row % 2 == 0) && (column % 2 == 0)) // row and column are even so place a piece
    {
        board[column][row] = valid_square;
    }
    else if ((row % 2 == 0) && (column % 2 != 0)) // row is even but column is odd so illegal square
    {
        board[column][row] = not_valid_square;
    }
    else if ((row % 2 != 0) && (column % 2 == 0)) // row is odd but column is even so illegal square
    {
        board[column][row] = not_valid_square;
    }
    else   //row and column are odd so place a piece
    {
        board[column][row] = valid_square;
    }
}

So first off, we're back to naming. In general, functions should have verb names. Things like "set_piece" or "is_valid_square" or "init_board". A function name "piece_placement" doesn't tell me what the function does. Does this place a piece or check if a piece's placement is correct? I shouldn't have to read the function to understand on a basic level what it does. It looks like this should just be called "set_piece".

As you discovered, you don't need to pass common representations into the function, it should be in a #define, so that eliminates one of the parameters to this function.

The logic of the function is a bit confusing because you're repeating yourself. This part is a bit difficult to get the hang of, but you can simplify logical statements like this with a bit of work. Look at the board, top left is a white square. It's an even row (row 0) and even column (column 0). On that row, all the even columns are white squares. Next row, an odd row, all the odd columns are white squares. So if the even-ness of the row equals the even-ness of the column, it's a white square so set that. Otherwise, set the piece passed. This could be a one-liner if you used the ternary operator, or 4 liner if you're not comfortable with that. It would also have only one conditional expression. But it's good that you recognized these follow a pattern and a lookup table was unnecessary.

You also need to decide where you want the program to check if things make sense in terms of checkers. Should a function like set_piece even care if the square is black or white or should it just set the pieces it's told? When you're talking about an abstracted data type, it usually just wants to store the data it's told and it's up to the functions that implement the actual checkers game to make sure that makes sense. For example, if you have a function called is_valid_move that makes sure a move is valid, it should be responsible for making sure you're not moving a piece to an invalid square. The set_piece function should be doing bounds checking, though, and probably print a debug message if it was given invalid coordinates because that generally means there's a bug somewhere else in your program.