14
\$\begingroup\$

I have read your rules and in particular the part about bite-sized portions, so I am apprehensive about posting this but want to do it anyway. I have recently begun getting into C and in particular ncurses with the desire to make some games for the sake of historical appreciation (at least a Rogue clone, probably more). Python is the language I've messed with most extensively to this point and I'm far from an expert in that.

If anyone has the time and the desire, here is 715 lines of a complete and working Tic Tac Toe game that I wrote on my Ubuntu Laptop. The AI is no genius and I could definitely make it better but that was not my focus here... my concern was in writing structurally sound code and a portable, good-looking ncurses display. To that end I think I did alright. What I would like to hear are any and all ways that I could improve my coding style to better take advantage of the C language and the ncurses library while making future games. Don't hold back! Every little thing will help me write better games in the near future. I don't need anybody to go over the entire thing unless they really want to, simply giving me style tips is more than helpful.

// The mandatory tic tac toe game to practice ncurses stuff.
// Date Started: 21 DEC 2018

#include <ncurses.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>


// Global Variables
time_t t;
int row, col, x, y, px, py, slen, sx;

// constants to use with the COLOR_PAIR() attribute for easier remembering
const int Xs = 1;
const int Os = 2;
const int BG = 3;

/*
    Board (7 x 7 character cells):
    Line 1 = ------- (7 hyphens)
    Line 2 = | | | | Spaces: 1=(1, 1) 2=(1, 3) 3=(1, 5) 
    Line 3 = -------         
    Line 4 = | | | |         4=(3, 1) 5=(3, 3) 6=(3, 5)  
    Line 5 = -------
    Line 6 = | | | |         7=(5, 1) 8=(5, 3) 9=(5, 5)
    Line 7 = -------
*/
const int line_len = 7;                       // constant int for the length of a board line

char break_lines[] = "-------";               // Strings representing the board lines, minus the pieces
char play_lines[] = "| | | |";

/*
    These spaces are abstractions, since the board itself will need to vary based on the current size of the terminal.
    They represent the current values of the "playable" spaces.
*/
char space_1 = ' ';
char space_2 = ' ';
char space_3 = ' ';
char space_4 = ' ';
char space_5 = ' ';
char space_6 = ' ';
char space_7 = ' ';
char space_8 = ' ';
char space_9 = ' ';
int space_1y, space_1x;
int space_2y, space_2x;
int space_3y, space_3x;
int space_4y, space_4x;
int space_5y, space_5x;
int space_6y, space_6x;
int space_7y, space_7x;
int space_8y, space_8x;
int space_9y, space_9x;

char *space_ptr = NULL;                       // Pointer to one of the playable spaces

// Player's side ('X' or 'O')
char side;                    

int running = 1;                              // Main menu control variable
int playing = 1;                              // Game loop control variable
int turn = 1;                                 // Turn number
int game_over = 0;

// Function Declarations
void f_intro();                               // Print elaborate "animated" intro splash
char f_setup();                               // Prompt player to pick a side or pick random selection, returns char
void f_paint();                               // Paint the board state on a given turn
void f_turn_input();                          // Take player input and determine AI action (includes sub-functions probably)
void f_player_turn();                         // Take player input and place the appropriate mark on the board
void f_AI_turn();                             // Logic (such as it is) and Placement for AI turn 
void f_evaluate_turn();                       // Check for endgame state and either advance the turn or end the game (with sub-functions probably)
int f_AI_picker();                            // Picks random spots until it finds an empty one or tries them all
void f_declare_winner(char symbol);           // Takes the winning character and creates a splash screen declaring victory ('X', 'O', or 'T' for Tie)


// Main Function
int main(){
    srand((unsigned) time(&t));
    initscr();
    clear();
    cbreak();
    keypad(stdscr, 1);
    curs_set(0);
    noecho();
    start_color();
    init_pair(Xs, COLOR_CYAN, COLOR_BLACK);
    init_pair(Os, COLOR_RED, COLOR_BLACK);
    init_pair(BG, COLOR_YELLOW, COLOR_BLACK);

    f_intro();                      // Print intro splash
    getch();

    while(running){
        clear();
        side = f_setup();             // Choose X's, O's, or RANDOM SIDE
        playing = 1;
        while(playing){
            f_paint();                  // Paint the board as it is that turn
            f_turn_input();             // Take player input and if valid determine AI move + sub-functions
            turn++;
        }
        // To-Do, a reset function
    }

    endwin();
    return 0;
}

// Function Definitions
void f_intro(){                   
    // Print elaborate "animated" intro splash
    int which;
    clear();
    getmaxyx(stdscr, row, col);
    // Print the background
    for(y=0;y<=row;y++){
        for(x=0;x<=col;x++){
            which = rand() % 3;
            if(which == 0){
                // Print an "X" in the cell
                attron(COLOR_PAIR(Xs));
                mvprintw(y, x, "X");
                attroff(COLOR_PAIR(Xs));
            }else if(which == 1){
                // Print an "O" in the cell
                attron(COLOR_PAIR(Os));
                mvprintw(y, x, "O");
                attroff(COLOR_PAIR(Os));
            }else if(which == 2){
                // Print a blank black space in the cell
                attron(COLOR_PAIR(BG));
                mvprintw(y, x, " ");
                attroff(COLOR_PAIR(BG));
            }
        }
    }
    // Print the Title
    y = row / 2 - 1;
    char intro_str[] = " NCURSES Tic Tac Toe! ";
    char intro_str_padding[] = "                      "; 
    char intro_str2[] = " any key to continue  "; 
    slen = strlen(intro_str);
    x = col / 2 - slen / 2;
    mvprintw(y++, x, intro_str_padding);
    mvprintw(y++, x, intro_str);
    mvprintw(y++, x, intro_str2);
    mvprintw(y, x, intro_str_padding);

    refresh();
}

char f_setup(){                   
    // Prompt player to pick a side or pick random selection, returns char
    int input;
    clear();
    getmaxyx(stdscr, row, col);
    char setup_str1[] = "Pick a side!";
    char setup_str2[] = "Press 'X', 'O', or 'R' for Random!";
    char *chose_x = "You chose X's! Any key to continue...";
    char *chose_y = "You chose O's! Any key to continue...";
    char *choice_ptr = NULL;
    y = row / 2 - 1;
    slen = strlen(setup_str1);
    x = col / 2 - slen / 2;
    mvprintw(y++, x, setup_str1);
    slen = strlen(setup_str2);
    x = col / 2 - slen / 2;
    mvprintw(y++, x, setup_str2);
    y++;
    refresh();
    input = getch();
    if(input == 'X' || input == 'x'){
        choice_ptr = chose_x;
        slen = strlen(choice_ptr);
        x = col / 2 - slen / 2;
        mvprintw(y, x, choice_ptr);
        refresh();
        getch();
        return 'X';
    }else if(input == 'O' || input == 'o'){
        choice_ptr = chose_y;
        slen = strlen(choice_ptr);
        x = col / 2 - slen / 2;
        mvprintw(y, x, choice_ptr);
        refresh();
        getch();
        return 'O';
    }else if(input == 'R' || input == 'r'){
        int r;
        r = rand() % 2;
        if(r == 0){
            // Pick 'X'
            choice_ptr = chose_x;
            slen = strlen(choice_ptr);
            x = col / 2 - slen / 2;
            mvprintw(y, x, choice_ptr);
            refresh();
            getch();
            return 'X';
        }else if(r == 1){
            // Pick 'O'
            choice_ptr = chose_y;
            slen = strlen(choice_ptr);
            x = col / 2 - slen / 2;
            mvprintw(y, x, choice_ptr);
            refresh();
            getch();
            return 'O';
        }
    }else{
        char err_str[] = "Input error! Any key to continue...";
        slen = strlen(err_str);
        x = col / 2 - slen / 2;
        mvprintw(y, x, err_str);
        refresh();
        getch();
        f_setup();
    }
}

void f_paint(){                   
    // Paint the board state on a given turn
    /*
        1. Clear screen.
        2. Paint blank board.
        3. Paint the contents of each playable cell.
        4. Refresh screen
    */
    clear();                                                    // Clear screen
    getmaxyx(stdscr, row, col);                                 // Get current size of terminal
    y = row / 2 - 3;                                            // Board is 7x7 characters, so (y / 2 - 3) is a decent top edge
    x = col / 2 - 3;                                            // Ditto for (x / 2 - 3) being a decent left edge.
    // Determine the locations of the 9 "playable" cells:
    space_1y = y + 1; space_1x = x + 1;
    space_2y = y + 1; space_2x = x + 3;
    space_3y = y + 1; space_3x = x + 5;
    space_4y = y + 3; space_4x = x + 1;
    space_5y = y + 3; space_5x = x + 3;
    space_6y = y + 3; space_6x = x + 5;
    space_7y = y + 5; space_7x = x + 1;
    space_8y = y + 5; space_8x = x + 3;
    space_9y = y + 5; space_9x = x + 5;
    // Paint the board roughly centered:
    int yy, xx;
    attron(COLOR_PAIR(BG));
    for(yy = 0; yy < line_len; yy++){
        if(yy == 0 || yy % 2 == 0){
            mvprintw(y + yy, x, break_lines);
        }else{
            mvprintw(y + yy, x, play_lines);
        }
    }
    attroff(COLOR_PAIR(BG));
    // Insert appropriate characters into the "playable" cells:
    if(space_1 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_1 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_1y, space_1x, space_1);
    if(space_2 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_2 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_2y, space_2x, space_2);
    if(space_3 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_3 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_3y, space_3x, space_3);
    if(space_4 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_4 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_4y, space_4x, space_4);
    if(space_5 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_5 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_5y, space_5x, space_5);
    if(space_6 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_6 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_6y, space_6x, space_6);
    if(space_7 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_7 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_7y, space_7x, space_7);
    if(space_8 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_8 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_8y, space_8x, space_8);
    if(space_9 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_9 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_9y, space_9x, space_9);
    attroff(COLOR_PAIR(Xs));
    attroff(COLOR_PAIR(Os));
    refresh();
}

void f_turn_input(){              
    // Take player input and determine AI action (includes sub-functions probably)
    /*
        1. Determine who goes first. 
            - Using if/else to divide the function into two halves for each possibility.
        2. Player/AI Takes turn. -> Refresh
        3. Player/AI takes turn. -> Refresh

        Note on AI: No real logic for this version. Just going to randomly pick from the available spaces. 
    */
    if(side == 'X'){
        // if player is 'X':
        f_player_turn();
        f_evaluate_turn();
        if(game_over == 0){
            f_AI_turn();
            f_evaluate_turn();
        }
    }else if(side == 'O'){
        // If player is 'O':
        f_AI_turn();
        f_evaluate_turn();
        if(game_over == 0){
            f_player_turn();
            f_evaluate_turn();
        }
    }
    refresh();
}


void f_player_turn(){             
    // Take player input and place the appropriate mark on the board
    int info_line = y + 10;                                // Determine the line that the info splash will show up on.
    char move_splash[] = "Use arrow keys and press 'P' to place your piece!";
    char done_splash[] = "Good move!";
    char move_err_splash[] = "You can't move that way!";
    char input_err_splash[] = "Invalid input!";
    char full_err_splash[] = "Spot already claimed!";
    slen = strlen(move_splash);
    sx = col / 2 - slen / 2;                               // Center the info splash
    mvprintw(info_line, sx, move_splash);
    curs_set(1);                                           // Enable the cursor for the player
    int pos_y = space_1y;                                  // Y position of the cursor
    int pos_x = space_1x;                                  // X position of the cursor
    move(pos_y, pos_x);                                    // Move it to space 1
    refresh();
    int inputting = 1;
    while(inputting){
        int input;
        char spot;
        int cx;
        input = getch();
        if(input == KEY_LEFT){
            if(!(pos_x == space_1x)){                          
                // If not on the left playable edge
                pos_x -= 2;
                move(pos_y, pos_x);
            }else{
                for(cx = sx; cx <= col; cx++){
                    // Clear the info line
                    mvaddch(info_line, cx, ' ');
                }
                slen = strlen(move_err_splash);
                sx = col / 2 - slen / 2;
                mvprintw(info_line, sx, move_err_splash);
                move(pos_y, pos_x);
            }
        }else if(input == KEY_RIGHT){
            if(!(pos_x == space_3x)){                          
                // If not on the right playable edge
                pos_x += 2;
                move(pos_y, pos_x);
            }else{
                for(cx = sx; cx <= col; cx++){
                    // Clear the info line
                    mvaddch(info_line, cx, ' ');
                }
                slen = strlen(move_err_splash);
                sx = col / 2 - slen / 2;
                mvprintw(info_line, sx, move_err_splash);   
                move(pos_y, pos_x);
            }
        }else if(input == KEY_UP){
            if(!(pos_y == space_1y)){                          
                // If not on the top playable edge
                pos_y -= 2;
                move(pos_y, pos_x);
            }else{
                for(cx = sx; cx <= col; cx++){
                    // Clear the info line
                    mvaddch(info_line, cx, ' ');
                }
                slen = strlen(move_err_splash);
                sx = col / 2 - slen / 2;
                mvprintw(info_line, sx, move_err_splash);   
                move(pos_y, pos_x);                                           
            }
        }else if(input == KEY_DOWN){
            if(!(pos_y == space_9y)){                          
                // If not on the bottom playable edge
                pos_y += 2;
                move(pos_y, pos_x);
            }else{
                for(cx = sx; cx <= col; cx++){
                    // Clear the info line
                    mvaddch(info_line, cx, ' ');
                }
                slen = strlen(move_err_splash);
                sx = col / 2 - slen / 2;
                mvprintw(info_line, sx, move_err_splash);   
                move(pos_y, pos_x);
            }
        }else if(input == 'P' || input == 'p'){
            /*
                1. Read contents of space.
                2. If Empty -> Place player's symbol
                3. Else, try again
            */
            if(pos_y == space_1y && pos_x == space_1x){
                space_ptr = &space_1;   
            }else if(pos_y == space_2y && pos_x == space_2x){
                space_ptr = &space_2;
            }else if(pos_y == space_3y && pos_x == space_3x){
                space_ptr = &space_3;
            }else if(pos_y == space_4y && pos_x == space_4x){
                space_ptr = &space_4;
            }else if(pos_y == space_5y && pos_x == space_5x){
                space_ptr = &space_5;
            }else if(pos_y == space_6y && pos_x == space_6x){
                space_ptr = &space_6;
            }else if(pos_y == space_7y && pos_x == space_7x){
                space_ptr = &space_7;
            }else if(pos_y == space_8y && pos_x == space_8x){
                space_ptr = &space_8;
            }else if(pos_y == space_9y && pos_x == space_9x){
                space_ptr = &space_9;
            }
            if(*space_ptr == ' '){
                if(side == 'X'){
                    *space_ptr = 'X';
                }else{
                    *space_ptr = 'O';
                }
                for(cx = sx; cx <= col; cx++){
                    // Clear the info line
                    mvaddch(info_line, cx, ' ');
                }
                slen = strlen(done_splash);
                sx = col / 2 - slen / 2;
                mvprintw(info_line, sx, done_splash);   
                move(pos_y, pos_x);
                refresh();
                inputting = 0;
            }else{
                for(cx = sx; cx <= col; cx++){
                    // Clear the info line
                    mvaddch(info_line, cx, ' ');
                }
                slen = strlen(full_err_splash);
                sx = col / 2 - slen / 2;
                mvprintw(info_line, sx, full_err_splash);   
                move(pos_y, pos_x);
            }
        }else{
            // If the user presses any other button 
            for(cx = sx; cx <= col; cx++){
                // Clear the info line
                mvaddch(info_line, cx, ' ');
            }
            slen = strlen(input_err_splash);
            sx = col / 2 - slen / 2;
            mvprintw(info_line, sx, input_err_splash);
            move(pos_y, pos_x);
        }
    }
}

int f_AI_picker(){
    /*
        1. Pick a number between 1 and 9
        2. Randomly decide whether to check spaces from 1 to 9 or 9 to 1 for the sake of variety
        3. Check them in the determined order until an open space is found.
        4. Return number of open space.

        Note: This version has no real strategic logic and will be easily beaten by any player.
              Although a quick fix for some added challenge is to make it prioritize the center tile.
    */
    int pick;
    pick = rand() % 9 + 1;
    int order;                                      // 1 = Ascending, 2 = Descending
    order = rand() % 2 + 1;
    if(space_5 == ' '){
        return 5;
    }else{
        if(order == 1){
            if(space_1 == ' '){
                return 1;
            }else if(space_2 == ' '){
                return 2;
            }else if(space_3 == ' '){
                return 3;
            }else if(space_4 == ' '){
                return 4;
            }else if(space_6 == ' '){
                return 6;
            }else if(space_7 == ' '){
                return 7;
            }else if(space_8 == ' '){
                return 8;
            }else if(space_9 == ' '){
                return 9;
            }
        }else if(order == 2){
            if(space_9 == ' '){
                return 9;
            }else if(space_8 == ' '){
                return 8;
            }else if(space_7 == ' '){
                return 7;
            }else if(space_6 == ' '){
                return 6;
            }else if(space_4 == ' '){
                return 4;
            }else if(space_3 == ' '){
                return 3;
            }else if(space_2 == ' '){
                return 2;
            }else if(space_1 == ' '){
                return 1;
            }
        }
    }
}

void f_AI_turn(){                 
    // Logic (such as it is) and Placement for AI turn 
    char AI_char;
    if(side == 'X'){
        AI_char = 'O';
    }else{
        AI_char = 'X';
    }
    int space_to_place;
    space_to_place = f_AI_picker();
    if(space_to_place == 1){
        space_1 = AI_char;
    }else if(space_to_place == 2){
        space_2 = AI_char;
    }else if(space_to_place == 3){
        space_3 = AI_char;
    }else if(space_to_place == 4){
        space_4 = AI_char;
    }else if(space_to_place == 5){
        space_5 = AI_char;
    }else if(space_to_place == 6){
        space_6 = AI_char;
    }else if(space_to_place == 7){
        space_7 = AI_char;
    }else if(space_to_place == 8){
        space_8 = AI_char;
    }else if(space_to_place == 9){
        space_9 = AI_char;
    }
    f_paint();
    refresh();  
}

void f_declare_winner(char symbol){           
    // Takes the winning character and creates a splash screen declaring victory ('X', 'O', or 'T' for Tie)
    char *x_wins = "  X is the winner!  ";
    char *o_wins = "  O is the winner!  ";
    char *tie_game = " The game is a tie! ";
    char padding[] = "                   ";
    char *win_splash_ptr = NULL;
    // Paint background for victory splash:
    if(symbol == 'X'){
      win_splash_ptr = x_wins;
      attron(COLOR_PAIR(Xs));
        for(y = 0; y <= row; y++){
            for(x = 0; x <= col; x++){
                if(x == 0 || x % 2 == 0){
                    mvaddch(y, x, 'X'); 
                }else{
                    mvaddch(y, x, ' ');
                }
            }
        }
      attroff(COLOR_PAIR(Xs));
    }else if(symbol == 'O'){
        win_splash_ptr = o_wins;
      attron(COLOR_PAIR(Os));
        for(y = 0; y <= row; y++){
            for(x = 0; x <= col; x++){
                if(x == 0 || x % 2 == 0){
                    mvaddch(y, x, 'O'); 
                }else{
                mvaddch(y, x, ' ');
                }
            }
        }
      attroff(COLOR_PAIR(Os));
    }else if(symbol == 'T'){
        win_splash_ptr = tie_game;
        for(y = 0; y <= row; y++){
            for(x = 0; x <= col; x++){
                if(x == 0 || x % 2 == 0){
                    attron(COLOR_PAIR(Xs));
                    mvaddch(y, x, 'X'); 
                    attroff(COLOR_PAIR(Xs));
                }else{
                    attron(COLOR_PAIR(Os));
                    mvaddch(y, x, 'O');
                    attroff(COLOR_PAIR(Os));
                }
            }
        }
    }
    //Paint the prompt
    y = row / 2 - 2;
    slen = strlen(win_splash_ptr);
    x = col / 2 - slen / 2;
    mvprintw(y++, x, padding);
    mvprintw(y++, x, win_splash_ptr);
    mvprintw(y, x, padding);
    curs_set(0);
    refresh();
    getch();
    running = 0;
    playing = 0;
}

void f_evaluate_turn(){           
    // Check for endgame state and either advance the turn or end the game (with sub-functions probably)
    /*
        1. Check for each possible victory condition. -> If so, declare appropriate victory.
        2. Check if turn number is high enough to indicate a tie -> If so, declare a tie.
        3. Else, continue.
    */
    int winner;
    winner = 'N';  // For none
    if(space_1 == 'O' && space_2 == 'O' && space_3 == 'O'){
        winner = 'O';
        game_over++;
    }else if(space_4 == 'O' && space_5 == 'O' && space_6 == 'O'){
        winner = 'O';
        game_over++;
    }else if(space_7 == 'O' && space_8 == 'O' && space_9 == 'O'){
        winner = 'O';
        game_over++;
    }else if(space_1 == 'O' && space_4 == 'O' && space_7 == 'O'){
        winner = 'O';
        game_over++;
    }else if(space_2 == 'O' && space_5 == 'O' && space_8 == 'O'){
        winner = 'O';
        game_over++;
    }else if(space_3 == 'O' && space_6 == 'O' && space_9 == 'O'){
        winner = 'O';
        game_over++;
    }else if(space_1 == 'O' && space_5 == 'O' && space_9 == 'O'){
        winner = 'O';
        game_over++;
    }else if(space_3 == 'O' && space_5 == 'O' && space_7 == 'O'){
        winner = 'O';
        game_over++;
    }else if(space_1 == 'X' && space_2 == 'X' && space_3 == 'X'){
        winner = 'X';
        game_over++;
    }else if(space_4 == 'X' && space_5 == 'X' && space_6 == 'X'){
        winner = 'X';
        game_over++;
    }else if(space_7 == 'X' && space_8 == 'X' && space_9 == 'X'){
        winner = 'X';
        game_over++;
    }else if(space_1 == 'X' && space_4 == 'X' && space_7 == 'X'){
        winner = 'X';
        game_over++;
    }else if(space_2 == 'X' && space_5 == 'X' && space_8 == 'X'){
        winner = 'X';
        game_over++;
    }else if(space_3 == 'X' && space_6 == 'X' && space_9 == 'X'){
        winner = 'X';
        game_over++;
    }else if(space_1 == 'X' && space_5 == 'X' && space_9 == 'X'){
        winner = 'X';
        game_over++;
    }else if(space_3 == 'X' && space_5 == 'X' && space_7 == 'X'){
        winner = 'X';
        game_over++;
    }else if(turn >= 5){
        winner = 'T';
        game_over++;
    }
    if(winner != 'N'){
        f_declare_winner(winner);   
    }
}

And of course it goes without saying but any fellow linux users who want to throw this in their home directory and play it themselves are more than welcome to do so! Just be aware that without more strategic thinking the AI is a total pushover. I only went so far as making it prioritize the center square.

\$\endgroup\$
  • 1
    \$\begingroup\$ Long questions are fine: they just tend to lend themselves to less comprehensive reviews (which is not necessarily a bad thing), and more rounds of review if you revise your code using the suggestions given. You can really learn a great deal from improving a larger program. And if you revise your code, it may turn out to need less than 715 lines! \$\endgroup\$ – Graham Dec 23 '18 at 12:42
  • 1
    \$\begingroup\$ I don't really want to do too much more with this. Maybe improve the AI a little bit. Mostly I'm interested in tips that will lend themselves to making better games in the future. Also any ways in which I have obviously mis-used the language or the ncurses library (as I'm sure there are a few). I made this in two marathon sessions and although I'm very proud of it I'm sure it's just full of stylistically bad choices. \$\endgroup\$ – some_guy632 Dec 23 '18 at 12:44
  • \$\begingroup\$ Even if you don't revise this, you'll still get some very useful advice for future projects. The benefit of trying to revise it is that you can work your way through the hiccups of revision and the creation process with a familiar piece of code instead of some code that's not fully-formed (and revision will probably take much less time than composition). But it's your time, and you should choose how to spend it. Also note that this is getting a bit chatty, so a moderator may come and clear this out later, because the main point of comments is to ask clarifying questions. \$\endgroup\$ – Graham Dec 23 '18 at 12:55
  • \$\begingroup\$ Noted! Well I'm all for advice on how to make it shorter as well. In particular I was not sure if it would be appropriate using switch statements instead of if-else chains as it's my understanding that switch statements with variables that change in run-time can be troublesome. That's definitely a clarification I'm after. \$\endgroup\$ – some_guy632 Dec 23 '18 at 13:12
  • 1
    \$\begingroup\$ Using the number pad for move entry is nice. You only ever have to hit one key to play, and the key layout matches the on-screen board. A simple way to create a good AI is to try a move in a copy of the game state, then recurses to see if that leads to a forced win or a possible forced loss. If there's a forced win, play that. Otherwise select randomly from all the moves that don't let the opponent force a win (i.e. lead to a draw). That's what I did for a first-year CS assignment to write a tic-tac-toe game. (But apparently I was the only one in the class to do that. :) \$\endgroup\$ – Peter Cordes Dec 23 '18 at 21:59
13
\$\begingroup\$

Avoid numbered variables

char space_1 = ' ';
char space_2 = ' ';
char space_3 = ' ';
char space_4 = ' ';
char space_5 = ' ';
char space_6 = ' ';
char space_7 = ' ';
char space_8 = ' ';
char space_9 = ' ';
int space_1y, space_1x;
int space_2y, space_2x;
int space_3y, space_3x;
int space_4y, space_4x;
int space_5y, space_5x;
int space_6y, space_6x;
int space_7y, space_7x;
int space_8y, space_8x;
int space_9y, space_9x;

If you find yourself using numbered variables, you almost always should be using an array instead. E.g.

const int CELL_COUNT = 9;

typedef struct {
    int y;
    int x;
} Location;

char cells[CELL_COUNT] = "         ";
Location cell_locations[CELL_COUNT];

Even with the constant and type declaration, this is still shorter than your original. And now you can refer to, e.g. cell_locations[0].x which is more self-documenting. Instead of a naming convention, we have an enforceable type pattern. A Location must have a y and an x. There must be CELL_COUNT (9) cell_locations.

You can move Location into a header file that you can reuse.

Later, instead of

    space_1y = y + 1; space_1x = x + 1;
    space_2y = y + 1; space_2x = x + 3;
    space_3y = y + 1; space_3x = x + 5;
    space_4y = y + 3; space_4x = x + 1;
    space_5y = y + 3; space_5x = x + 3;
    space_6y = y + 3; space_6x = x + 5;
    space_7y = y + 5; space_7x = x + 1;
    space_8y = y + 5; space_8x = x + 3;
    space_9y = y + 5; space_9x = x + 5;

We can say something like

    int i = 0;
    for (int current_y = y + 1, n = y + line_len; current_y < n; current_y += 2) {
        for (int current_x = x + 1, m = x + line_len; current_x < m; x += 2) {
            cell_locations[i].y = current_y;
            cell_locations[i].x = current_x;
            i++;
        }
    }

And instead of nine

    if(space_1 == 'X'){
        attron(COLOR_PAIR(Xs));
    }else if(space_1 == 'O'){
        attron(COLOR_PAIR(Os));
    }
    mvaddch(space_1y, space_1x, space_1);

We can say something like

int determine_color_pair(char mark) {
    if (mark == 'X') {
        return Xs;
    }

    if (mark == 'O') {
        return Os;
    }

    return BG;
} 

and

    for (int j = 0; j < CELL_COUNT; j++) {
         attron(COLOR_PAIR(determine_color_pair(cells[j])));
         mvaddch(cell_locations[j].y, cell_locations[j].x, cells[j]);
    }

Prefer descriptive names to comments

void f_intro();                               // Print elaborate "animated" intro splash

With proper naming, you won't need comments for things like this.

void display_intro();

or even

void display_elaborate_pseudo_animated_intro_splash();

Although I think that's overkill. But for a function that you only call once, that kind of name is possible.

I'm not crazy about using an f_ prefix to indicate a function. Functions should generally have verb names, because they do things. Variables have noun names, because they represent things. It is more common to use prefixes to separate your code from other code that may be linked, e.g. ttt_display_intro. Then if you link your Tic-Tac-Toe game with a text-based RPG (e.g. Zork or Rogue), you can have display_intro functions in both.

In code examples, they often put comments on every variable for didactic purposes. I find this unfortunate, as it gives people an unrealistic view of how comments should be used. Comments should be used to explain why code does things rather than what the code is doing. The code itself should mostly suffice for telling people what the code is doing.

I find the practice of comments after code on the same line obnoxious. I would much rather see

// Takes the winning character and creates a splash screen declaring victory ('X', 'O', or 'T' for Tie)
void f_declare_winner(char symbol);

Note how that gets rid of the scroll bar.

If the comment is not needed to appear in the left column, then you are probably better off without it.

Simplification

    }else if(input == 'R' || input == 'r'){
        int r;
        r = rand() % 2;
        if(r == 0){
            // Pick 'X'
            choice_ptr = chose_x;
            slen = strlen(choice_ptr);
            x = col / 2 - slen / 2;
            mvprintw(y, x, choice_ptr);
            refresh();
            getch();
            return 'X';
        }else if(r == 1){
            // Pick 'O'
            choice_ptr = chose_y;
            slen = strlen(choice_ptr);
            x = col / 2 - slen / 2;
            mvprintw(y, x, choice_ptr);
            refresh();
            getch();
            return 'O';
        }

This whole block can be removed.

Replace

    input = getch();
    if(input == 'X' || input == 'x'){

with

    input = toupper(getch());
    if (input == 'R') {
        int r = rand() % 2;
        input = (r == 0) ? 'X' : 'O';
    }

    if (input == 'X') {

Now you don't have to duplicate the code.

At the end of the function,

        f_setup();

should probably be

        return f_setup();

Or change things to use an infinite loop rather than a recursive call.

void display_prompt_and_wait_for_input(const char *choice_ptr) {
    int slen = strlen(choice_ptr);
    x = col / 2 - slen / 2;
    mvprintw(y, x, choice_ptr);
    refresh();
    getch();
}

// Prompt player to pick a side or pick random selection, returns char
char f_setup() {
    const char setup_str1[] = "Pick a side!";
    const char setup_str2[] = "Press 'X', 'O', or 'R' for Random!";
    char *chose_x = "You chose X's! Any key to continue...";
    char *chose_y = "You chose O's! Any key to continue...";

    for (;;) {
        clear();
        getmaxyx(stdscr, row, col);
        y = row / 2 - 1;
        int slen = strlen(setup_str1);
        x = col / 2 - slen / 2;
        mvprintw(y++, x, setup_str1);
        slen = strlen(setup_str2);
        x = col / 2 - slen / 2;
        mvprintw(y++, x, setup_str2);
        y++;
        refresh();

        int input = toupper(getch());
        if (input == 'R') {
            int r = rand() % 2;
            input = (r == 0) ? 'X' : 'O';
        }

        switch (input) {
            case 'X':
                display_prompt_and_wait_for_input(chose_x);
                return 'X';
            case 'O':
                display_prompt_and_wait_for_input(chose_y);
                return 'O';
            default:
                char *err_str = "Input error! Any key to continue...";
                display_prompt_and_wait_for_input(err_str);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks! I wish I could mark all of these as answers. I will be using all of these tips for my next project shortly. \$\endgroup\$ – some_guy632 Dec 23 '18 at 22:37
12
\$\begingroup\$

Here are a number of things that may help you improve your program.

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. For this program, it would be easy and natural to wrap nearly all of the global variables in a struct to make it clear which things go together. Then all you need is to pass around a pointer to that struct. In some case, such as x, it could simply be put as a local variable into functions that uses that variable. Even better, in the case of t, it can be eliminated entirely because time(NULL) will do what you need.

Eliminate unused variables

This code declares variables px and py but then does nothing with them. Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.

Use more whitespace to enhance readability of the code

Instead of crowding things together like this:

for(y=0;y<=row;y++){
    for(x=0;x<=col;x++){

most people find it more easily readable if you use more space:

for(y = 0; y <= row; y++) {
    for(x = 0;x <= col; x++) {

Don't Repeat Yourself (DRY)

There is a lot of repetitive code in this and some peculiar variables such as space_1 through space_9. This could be greatly improved by simply using an array space[9] and using index variables to step through those spaces. A similar thing could be done with the overly long f_evaluate_turn() function.

Return something useful from functions

The way the functions are currently written, most return void but this prevents simplifying the code. For example, instead of this:

f_player_turn();
f_evaluate_turn();
if(game_over == 0){
    f_AI_turn();
    f_evaluate_turn();
}

You could write this if each turn evaluated itself and returned true if the game is not yet over:

if (f_player_turn()) {
    f_AI_turn();
}

Use better naming

Some of the names, such as game_over and running are good because they are descriptive, but others, such as sx don't give much hint as to what they might mean. Also, using the f_ prefix for all functions is simply annoying and clutters up the code.

Use a better random number generator

You are currently using

which = rand() % 3;

There are a number of problems with this approach. This will generate lower numbers more often than higher ones -- it's not a uniform distribution. Another problem is that the low order bits of the random number generator are not particularly random, so neither is the result. On my machine, there's a slight but measurable bias toward 0 with that. See this answer for details, but I'd recommend changing that to something like those shown in the link. It doesn't matter a great deal in this particular application, but it's good to know generally.

Create and use smaller functions

This code could be much shorter and easier to read, understand and modify if it used smaller functions. For example, using a function like this consistently would greatly improve the readability of the code:

void place(int x, int y, char ch) {
    switch (ch) {
        case ' ':
            attron(COLOR_PAIR(BG));
            mvprintw(y, x, " ");
            attroff(COLOR_PAIR(BG));
            break;
        case 'X':
            attron(COLOR_PAIR(Xs));
            mvprintw(y, x, "X");
            attroff(COLOR_PAIR(Xs));
            break;
        case 'O':
            attron(COLOR_PAIR(Os));
            mvprintw(y, x, "O");
            attroff(COLOR_PAIR(Os));
            break;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Aren't you missing a loop around which = rand() / (RAND_MAX / 3); for it to be correct? (well less biased, although admittedly rand is generally so horrible that you can't save it even if you try. \$\endgroup\$ – Voo Dec 24 '18 at 0:53
  • \$\begingroup\$ @Voo yes, a loop as in the linked answer is the best way to do it. \$\endgroup\$ – Edward Dec 24 '18 at 1:44
  • 1
    \$\begingroup\$ Both which = rand() % 3; and which = rand() / (RAND_MAX / 3); form a similarly slightly skewed distribution. Recommend the 2nd does not add distribution benefit. At least rand() % 3 is clear. \$\endgroup\$ – chux - Reinstate Monica Dec 24 '18 at 3:05
  • \$\begingroup\$ "the low order bits of the random number generator are not particularly random" comes from where? Certainly a quality of implementation issue and not a C specification. \$\endgroup\$ – chux - Reinstate Monica Dec 24 '18 at 3:08
  • 2
    \$\begingroup\$ If you’d like a review of the new code, post it as a new question and link to this one. I’m sure people will review it. \$\endgroup\$ – Edward Dec 28 '18 at 22:42
4
\$\begingroup\$

These:

char space_1 = ' ';
char space_2 = ' ';
char space_3 = ' ';
char space_4 = ' ';
char space_5 = ' ';
char space_6 = ' ';
char space_7 = ' ';
char space_8 = ' ';
char space_9 = ' ';
int space_1y, space_1x;
int space_2y, space_2x;
int space_3y, space_3x;
int space_4y, space_4x;
int space_5y, space_5x;
int space_6y, space_6x;
int space_7y, space_7x;
int space_8y, space_8x;
int space_9y, space_9x;

should certainly be refactored into three arrays. That will allow you to write sane loops and decrease the repetition in your code.

For all of your global variables, as well as all of your functions except main, they should be declared static because they aren't being exported to any other modules.

Your running and playing variables are actually booleans, so you should be using <stdbool.h>.

Having x and y as globals seems ill-advised, especially where you have them being used in loops like this:

for(y=0;y<=row;y++){
    for(x=0;x<=col;x++){

They should probably be kept as locals, and in this case, instantiated in the loop declaration.

Your if(which == 0){ can be replaced by a switch, since you're comparing it three times.

char *chose_x and any other string that doesn't change should be declared const.

This:

if(input == 'O' || input == 'o')

should be:

if (tolower(input) == 'o')

and similarly for similar cases.

This:

x = col / 2 - slen / 2;

can be:

x = (col - slen) / 2;

though, as @PeterCordes correctly notes, you need to be careful about applying this rule generally if working with signed integers. And yes, it is best practice to make variables unsigned if you know that the data will not be negative.

This:

    if(yy == 0 || yy % 2 == 0){
        mvprintw(y + yy, x, break_lines);
    }else{
        mvprintw(y + yy, x, play_lines);
    }

should use an intermediate variable for simplicity, and the first == 0 is redundant:

char *lines;
if (!(yy % 2))
    lines = break_lines;
else
    lines = play_lines;
mvprintw(y + yy, x, lines);

Do a similar temporary variable strategy in your code for Print an "X" in the cell. This is in the name of DRY ("don't repeat yourself").

Since you won't be modifying this:

char done_splash[] = "Good move!";

You should instead declare it as a const char* rather than an array.

\$\endgroup\$
  • 4
    \$\begingroup\$ It's maybe worth mentioning that (col - slen) / 2 is not exactly equivalent to col / 2 - slen / 2. The variables are signed, and (1 - (-1) ) /2 = 2/2 = 1, but 1/2 and -1/2 are both 0 (truncation), and 0 - 0 = 0. It's a safe optimization in this case because neither variable will ever hold a negative number. It might also be better to make both of them unsigned, so the compiler doesn't have to implement signed-division semantics and can just use a right shift. \$\endgroup\$ – Peter Cordes Dec 23 '18 at 21:52
  • \$\begingroup\$ It did not occur to me to use unsigned. Is that a best practice in C for integers that will never be negative? \$\endgroup\$ – some_guy632 Dec 23 '18 at 23:59
  • \$\begingroup\$ @PeterCordes You're right; I've noted as much in an edit. \$\endgroup\$ – Reinderien Dec 24 '18 at 0:54
  • 2
    \$\begingroup\$ @some_guy632: it has some advantages and disadvantages. If you do col - 2 < n, you will get the "unexpected" result that col from 0..1 wraps to UINT_MAX-1..0, so the compare is false. Signed overflow being undefined behaviour also allows compilers to optimize more aggressively in some cases vs. using an unsigned type that's narrower than a pointer. (Which is the case for unsigned on 64-bit machines). See blog.llvm.org/2011/05/what-every-c-programmer-should-know.html. But yes, division by compile-time constants is cheaper for unsigned numbers than signed, power of 2 or not. \$\endgroup\$ – Peter Cordes Dec 24 '18 at 1:06
  • 1
    \$\begingroup\$ @Peter I wouldn't claim that using unsigned ints for numbers you expect to never be negative is a best practice in C though. The exceedingly minor performance advantage in some situations vs. the additional complexities is not that straight forward. Google explicitly says to not use unsigned numbers for "this number should not be negative". \$\endgroup\$ – Voo Dec 24 '18 at 14:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.