I have created a game called 'runes' in C. It is similar to Chess, but has a bigger board and different pieces. I would like to know how I can improve the code, for example, by adding proper error checking and by fixing any bugs. (This is probably not called 'broken code' because the main functions work fine.) Also, there is a large amount of spaghetti code in the middle and I don't know how to fix it./* * runes.c The Game of Runes, similar to chess. * * RULES * Pieces: Wizard, Peasant, Knight, Emperor, Archer, Dragon. * * Attack Abilities: * Wizard can attack wizard, peasant, knight, emperor, archer. * Peasant can attack peasant, knight. * Knight can attack peasant, knight, emperor. * Emperor cannot attack. * Archer can attack anything with up to 2 squares between it and the target. * Dragon can attack anything. * * Movement Abilities: * All pieces can move 1 square in any direction except for the following: * Knight. It moves in the L shape, as in chess. It can jump over pieces. * Dragon. Each turn, it can move 3 squares in any direction. It can jump *         over pieces. However, it cannot hover above a piece. * * Misc Abilities: * The Wizard can protect all friendly pieces right next to him. * * Capturing Pieces: * All pieces can kill enemy pieces if they can walk over them, with two * exceptions: * Archer. It can kill enemy pieces with 2 squares between them, 1 if *         diagonal. 1 piece cannot block the arrow, but 2 pieces can. * Wizard. Same as Archer. * Dragon. It can kill enemy pieces with up to 1 square between them, 1 if *         diagonal * * The Wizard, Dragon and Emperor are major pieces. * The Peasant, Knight, and Archer are minor pieces. * In general, major pieces are more valuable than minor pieces. * * Each turn, you can move up to 9 pieces, but you must move at least one. * This is so you can move either of your four battalions at once (see * diagram below). * * TECHNICAL INFORMATION * There is an array of 256 elements which describes the position of the * pieces on the board. Starting at the top left. *//* INITIAL PIECE POSITIONS  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+16|   |   |   |   |   |   |   | d | e |   |   |   |   |   |   |   |     <-- Black Side  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+15| p | p | p |   | p | p | p |   |   | p | p | p |   | p | p | p |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+14| p | w | p |   | p | w | p |   |   | p | w | p |   | p | w | p |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+13| p | p | p |   | p | p | p |   |   | p | p | p |   | p | p | p |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+12|   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+11| k | a | k | a | k | a | k | a | a | k | a | k | a | k | a | k |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+10|   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+9 |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+8 |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+7 |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+6 | K | A | K | A | K | A | K | A | A | K | A | K | A | K | A | K |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+5 |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+4 | P | P | P |   | P | P | P |   |   | P | P | P |   | P | P | P |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+3 | P | W | P |   | P | W | P |   |   | P | W | P |   | P | W | P |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+2 | P | P | P |   | P | P | P |   |   | P | P | P |   | P | P | P |  +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+1 |   |   |   |   |   |   |   | E | D |   |   |   |   |   |   |   |     <-- White SideY +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ X  1   2   3   4   5   6   7   8   9  10  11  12  13  14  15  16*/#include <stdio.h>#include <stdlib.h>#include <string.h>/* Don't use constants, it will be very messy and they're not defined yet * anyway. We initialise the board so we can render it later. */char board[256] = {' ',' ',' ',' ',' ',' ',' ','d','e',' ',' ',' ',' ',' ',' ',' ','p','p','p',' ','p','p','p',' ',' ','p','p','p',' ','p','p','p','p','w','p',' ','p','w','p',' ',' ','p','w','p',' ','p','w','p','p','p','p',' ','p','p','p',' ',' ','p','p','p',' ','p','p','p',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ','k','a','k','a','k','a','k','a','a','k','a','k','a','k','a','k',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ','K','A','K','A','K','A','K','A','A','K','A','K','A','K','A','K',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ','P','P','P',' ','P','P','P',' ',' ','P','P','P',' ','P','P','P','P','W','P',' ','P','W','P',' ',' ','P','W','P',' ','P','W','P','P','P','P',' ','P','P','P',' ',' ','P','P','P',' ','P','P','P',' ',' ',' ',' ',' ',' ',' ','E','D',' ',' ',' ',' ',' ',' ',' '};/* Global variable saying which player you are. */char current_player = 'w';/* Define constants for pieces, white pieces capitalised */#define BWIZARD     'w'#define BPEASANT    'p'#define BKNIGHT     'k'#define BEMPEROR    'e'#define BDRAGON     'd'#define BARCHER     'a'#define WWIZARD     'W'#define WPEASANT    'P'#define WKNIGHT     'K'#define WEMPEROR    'E'#define WDRAGON     'D'#define WARCHER     'A'#define EMPTY       ' '/* * This function gets the value of the piece on the board if any. Otherwise it * returns EMPTY which is defined above. The x/y coordinates are on the board * diagram above. Remember x=horizontal y=vertical. However, due to simplicity * the x and y values are inverted, i.e. x=3,y=3 would be board[51]. Also, * x=0,y=0 would be the top right. */char get_piece_on_board(unsigned short int x, unsigned short int y){    return board[x+(y*16)];}/* * Set a piece to a value. No error checking done. */void set_piece(unsigned short int x, unsigned short int y, char piece){    board[x+(y*16)] = piece;    return;}/* * This should be obvious, it renders the board onto the screen. */void render_board(void){    unsigned short int x;    unsigned short int y;    printf(+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+\n);    for (y = 0; y < 16; y++) {        for (x = 0; x < 16; x++) {            printf(| %c , get_piece_on_board(x, y));        }        printf(|\n+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+\n);    }    return;}/* * This function returns 1 or 0 depending on whether the specified piece * belongs to white or black. 1 if it belongs to the current player, * 0 otherwise. -1 if it is blank. */short int piece_belongs_to_current_player(unsigned short int x, unsigned short int y){    switch (get_piece_on_board(x, y)) {    case BWIZARD: /* Square belongs to black */    case BPEASANT:    case BKNIGHT:    case BEMPEROR:    case BDRAGON:    case BARCHER:        if (current_player == 'w') {            return 0;        } else if (current_player == 'b') {            return 1;        }        break;    case WWIZARD: /* Square belongs to white */    case WPEASANT:    case WKNIGHT:    case WEMPEROR:    case WDRAGON:    case WARCHER:        if (current_player == 'w') {            return 1;        } else if (current_player == 'b') {            return 0;        }        break;    case EMPTY: /* There is nothing in that square */        return -1;        break;    }    return 0;}/* * Check if a p1 is stronger than p2, i.e. can piece1 capture piece2? If * you read this, get ready for spaghetti... */unsigned short int is_stronger_than(unsigned short int p1x, unsigned short int p1y, unsigned short int p2x, unsigned short int p2y){    char p1;    char p2;    p1 = get_piece_on_board(p1x, p1y);    p2 = get_piece_on_board(p2x, p2y);    /* Worst spaghetti code ever! Well, at least I tried... */    if ((p1 == WDRAGON || p1 == BDRAGON) || ((p1 == WWIZARD || p1 == BWIZARD) && (p2 != BDRAGON && p2 != WDRAGON)) || ((p1 == WPEASANT || p1 == BPEASANT) && (p2 == BKNIGHT || p2 == WKNIGHT || p2 == WPEASANT || p2 == BPEASANT)) || ((p1 == WKNIGHT || p1 == BKNIGHT) && (p2 == BPEASANT || p2 == WPEASANT || p2 == BKNIGHT || p2 == WKNIGHT || p2 == WEMPEROR || p2 == BEMPEROR)) || (p1 == BARCHER || p1 == WARCHER)) {        return 1;    }    return 0;}/* * This function moves a piece. If you try to move the opponent's piece, it * will display an error. If you try to move your piece into another of your * pieces, it will also display an error. If you move your piece into the * piece of an opponent, it will check if your piece is powerful enough to * capture that piece. If so, the function will call set_piece() to capture * your opponent's piece. Much of the logic in this program is in here. */void move_piece(unsigned short int oldx, unsigned short int oldy, unsigned short int newx, unsigned short int newy){    /* Checks to make sure insane things are not done */    if (piece_belongs_to_current_player(oldx, oldy) == 0) {        fprintf(stderr, Cannot move piece belonging to other player\n);        return;    } else if (piece_belongs_to_current_player(newx, newy) == 1) {        fprintf(stderr, Cannot capture own piece\n);        return;    } else if (get_piece_on_board(oldx, oldy) == EMPTY) {        fprintf(stderr, Cannot move empty square\n);        return;    } else if (is_stronger_than(oldx, oldy, newx, newy)) {        fprintf(stderr, Piece is too strong to capture);        return;    }    set_piece(newx, newy, get_piece_on_board(oldx, oldy));    set_piece(oldx, oldy, EMPTY); /* clear out old piece */    return;}/* * This function prints out the title in ASCII art. */void print_header(void){    printf(\n);    printf( 8888888  88   88  8888   88  8888888   8888888\n        88   88  88   88  88 88  88  88       88\n        8888888  88   88  88 88  88  8888888   88888\n        88 88    88   88  88  88 88  88           8888\n        88  88   88   88  88  88 88  88             88\n        88   88  8888888  88   8888  8888888   888888\n);    printf(\n);    return;}/* * Print out the instructions for Runes */void print_instructions(void){    printf( Runes: Instructions\n        ===================\n        Runes is a strategy game similar to chess. The key differences are the names\n        of the pieces, how they move, and that the board size is doubled. This aims\n        to create a more interesting game.\n        \n        Pieces\n        ------\n        There are 6 types of pieces: Peasant, Knight, Wizard, Emperor, Dragon, Archer.\n        All pieces can move 1 square in 8 directions, including diagonals, except for\n        the Dragon and the Knight. The Knight moves in the same L-shape as in chess,\n        and the Dragon can move 3 squares in a turn. The Knight and the Dragon can fly\n        or jump/teleport over pieces.\n        \n        Abilities\n        ---------\n        The Dragon can breathe fire on a piece and destroy it if it is up to 1 square\n        away from it. This includes being right next to it, and being seperated by a\n        square. The Archer can shoot arrows at pieces if they are seperated by 2 squares\n        with at most 1 other piece between them. The Wizard does the same as the Archer,\n        except that the Wizard also shields ally pieces from any kind of damage if they\n        are right next to him. The only way to destroy a Battalion (a formation with a\n        Wizard in the center of other pieces) is to use a Dragon, Archer or another\n        Wizard.\n        \n        Piece Strength\n        --------------\n        A piece can only destroy other pieces if it is 'stronger' than that piece.\n        Here is a list of pieces and their point value:\n        Dragon and Archer can attack anything.\n        Wizard can attack anything except for Dragon.\n        Peasant can attack Peasant, Knight and Emperor.\n        Knight can attack Peasant, Knight, Emperor and Wizard.\n        Emperor cannot attack.\n);    return;}/* * Asks the user what to do */void print_menu(void){    char choice;    printf(1. Start Game\n);    printf(2. Instructions\n);    printf(3. Exit\n);    choice = getchar();    switch (choice) {    case '1':        render_board();        return;    case '2':        print_instructions();        return;    case '3':        exit(EXIT_SUCCESS);    default:        fprintf(stderr, \nInvalid entry\n);        exit(EXIT_FAILURE);        break;    }    return;}/* * Read a line from stdin for runes commands such as move. */char *read_command_line(void){    char *line = NULL;    size_t bufsize = 0;    if (getline(&line, &bufsize, stdin) == -1) {        perror(getline);    }    return line;}/* * Split the line into tokens. */char **split_line(char *line){    const short int const_bufsize = 64;    const char delim[] =  \t\r\n\a;    int bufsize = const_bufsize;    int position;    char **tokens = malloc(bufsize * sizeof(char*));    char *token;    if (!tokens) {        perror(malloc);        exit(EXIT_FAILURE);    }    token = strtok(line, delim);    for (position = 0; token != NULL; position++) {        tokens[position] = token;        if (position >= bufsize) {            bufsize += const_bufsize;            tokens = realloc(tokens, bufsize * sizeof(char*));            if (!tokens) {                perror(malloc);                exit(EXIT_FAILURE);            }        }        token = strtok(NULL, delim);    }    tokens[position] = NULL;    return tokens;}/* * Run runes commands: move, exit, surrender, pass, flee, help * This function returns 1 if a turn was not used, 0 otherwise. */unsigned short int run_runes_commands(char **args){    /* The pass command means skip current turn */    if (args[0] == NULL || strcmp(args[0], pass) == 0) {        return 0; /* Turn was used */    } else if (strcmp(args[0], surrender) == 0 || strcmp(args[0], flee) == 0) { /* the other player wins */        if (current_player == 'w') {            printf(White has surrendered, Black wins\n);        } else if (current_player == 'b') {            printf(Black has surrendered, White wins\n);        }        exit(EXIT_SUCCESS);    } else if (strcmp(args[0], exit) == 0) { /* unconditional exit */        exit(EXIT_SUCCESS);    } else if (strcmp(args[0], move) == 0) {        if (args[1] == NULL || args[2] == NULL || args[3] == NULL || args[4] == NULL) {            printf(usage: move <oldx> <oldy> <newx> <newy>\n);            return 1;        }        move_piece(atoi(args[1])-1, atoi(args[2])-1, atoi(args[3])-1, atoi(args[4])-1);        render_board();    } else if (strcmp(args[0], help) == 0) {        printf( Runes Commands Help:\n            help - Show this help\n            surrender - lose, other player wins\n            flee - same as surrender\n            move - move a piece\n            pass - skip this move\n            exit - unconditional exit\n);        return 1;    } else {        printf(Invalid Command = pass\n);        return 1;    }    return 0;}/* * The aim is to put most of the code into the other functions, and keep main * as neat as possible. */int main(void){    char *line;    char **args;    unsigned short int i;    print_header();    print_menu();    /* Main loop */    for (;;) {        for (i = 0; i <= 9; i++) {            printf(runes %c> , current_player);            line = read_command_line();            args = split_line(line);            if (run_runes_commands(args)) {                i--;            }        }        /* Switch player */        if (current_player == 'w') {            current_player = 'b';        } else if (current_player == 'b') {            current_player = 'w';        }    }    return 0;}
Game of Runes: a similar concept to chess
c;game;chess
First, let me say that the quality of this code is higher than the usual C posted here, so well done!  It compiles without errors or warnings, is formatted nicely and has useful and informative comments.  There are still some things I noticed that may help you improve your code.Don't leak memoryBoth malloc and getline allocate memory but this code does not contain any calls to free.  This is easily addressed by modifying main:for (i = 0; i <= 9; i++) {    printf(runes %c> , current_player);    line = read_command_line();    args = split_line(line);    if (run_runes_commands(args)) {        i--;    }    free(line);    free(args);}Avoid global variablesThe current_player and board variables would be better as variables within the scope of main instead of global variables.  Then pass them to the functions that need them.  This would make the linkage among the functions explicit rather than implicit and leads to more robust, more maintainable code.  In fact, I'd probably wrap both of these in a structure, neatly encapsulating the game state.Create a strength tableBased on the text description of the game, some pieces are stronger than others, and we can create a chart:  D A W P K ED 1 1 1 1 1 1 = 6A 1 1 1 1 1 1 = 6W   1 1 1 1 1 = 5K     1 1 1 1 = 4 P       1 1 1 = 3E             = 0On the left side of the chart is the attacking piece and the columns represent the piece that is attacked.  We can now simplify the code by using a function that returns the strength of a piece:unsigned short get_strength(char p){    switch (toupper(p)) {        case 'D':        case 'A':            return 6;            break;        case 'W':            return 5;            break;        case 'K':            return 4;            break;        case 'P':            return 3;            break;        default:            return 0;    }}Now your spaghetti becomes this very neat function:unsigned short int is_stronger_than(unsigned short int p1x, unsigned short int p1y, unsigned short int p2x, unsigned short int p2y){    unsigned short s1 = get_strength(get_piece_on_board(p1x, p1y));    unsigned short s2 = get_strength(get_piece_on_board(p2x, p2y));    return s1 > 0 && s1 >= s2;}Reduce duplication of effort in the codeIf we look at a function such as move_piece, it takes the old and new coordinates and calls a number of functions to check the legality of the move.  This is fine, but the index calculation and the lookup of the piece is done repetitively.  Instead, I think it would make more sense to look up the pieces at each coordinate pair once and then make decisions based on the value of the pieces. That is, if the first two lines of that function were these:char oldpiece = get_piece_on_board(oldx, oldy);char newpiece = get_piece_on_board(newx, newy);I think it would be easy to see how the rest of the code could be redone.Use functions to track attributesIn the piece_belongs_to_current_player there are a number of things I'd suggest changing.  First, pass in the value of the piece rather than the coordinates.  Second, rather than comparing to the global variable current_player, why not simply make the function return the corresponding player?  If we do that, it can be rewritten much more clearly and simply like this:char color(char piece){    if (piece == EMPTY)        return EMPTY;    return islower(piece) ? 'b' : 'w';}Reduce the scope of variablesWithin main the variable i only has meaning with the inner for loop, so it should instead be declared within the for loop:for (unsigned short i = 0; i <= 9; i++) {Eliminate magic numbersAs with the function above, there are a number of places in the code where 'b' signifies the black player and 'w' signifies the white player.  Unfortunately, other places in the code 'w' signifies a black wizard. Since you already have a #define for the pieces, and since the letters 'b' and 'w' are only shown to the player in the context of main, I'd recommend eliminating these and using this instead:#define BLACK 0#define WHITE 1Now main could look like this:int main(void){    char *line;    char **args;    const char players[] = bw;    print_header();    print_menu();    /* Main loop */    for (char current_player = WHITE; ; current_player = WHITE - current_player) {        for (unsigned short i = 0; i <= 9; i++) {            printf(runes %c> , players[(int)current_player]);            line = read_command_line();            args = split_line(line);            if (run_runes_commands(args, current_player)) {                i--;            }            free(line);            free(args);        }    }}Obviously, I have altered current_player (but not yet board) to be local, and have changed the corresponding functions to use BLACK and WHITE instead of the letters. Eliminate return 0 at the end of mainSince C99, the compiler automatically generates the code corresponding to return 0 at the end of main so there is no need to explicitly write it.Think of the userDuring the course of the game the player might reasonably want to re-read the rules or to display the current game state, however these things are not provided for during the course of the game.  Add these as commands.  Also, when the board is printed, it should show the numbers around the outside edges of the board to make it much easier to enter moves.Validate movesNothing in the code prevents a player from moving a powerful piece, such as the dragon, anywhere in the board, even though the rules say that they have limitations in how far they can move.  Moves should be validated according to the stated limitations of each piece.Detect winning or losing conditionsIt's not stated, but I'd imagine that losing all of your pieces would mean that you've lost the game.  The code doesn't actually detect that condition, though, and instead relies on a player to surrender.  It would be better to both clarify the criteria for winning and to encode that into the game.