Ncurses Tic Tac Toe with simplistic AI












13












$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.










share|improve this question











$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$


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.










share|improve this question











$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












13








13


3



$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.










share|improve this question











$endgroup$




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.







beginner c tic-tac-toe ai curses






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Dec 23 '18 at 13:57









200_success

129k15152415




129k15152415










asked Dec 23 '18 at 12:26









some_guy632some_guy632

1086




1086








  • 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
















  • 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










1




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






$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




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$
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$
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






$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




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






$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












3 Answers
3






active

oldest

votes


















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





share|improve this answer









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





share|improve this answer











$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
    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
    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.






share|improve this answer











$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





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});














draft saved

draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210217%2fncurses-tic-tac-toe-with-simplistic-ai%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























3 Answers
3






active

oldest

votes








3 Answers
3






active

oldest

votes









active

oldest

votes






active

oldest

votes









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





share|improve this answer









$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
















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





share|improve this answer









$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














13












13








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





share|improve this answer









$endgroup$



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






share|improve this answer












share|improve this answer



share|improve this answer










answered Dec 23 '18 at 16:04









mdfst13mdfst13

17.5k62156




17.5k62156












  • $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


















  • $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
















$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




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





share|improve this answer











$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
    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
    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
















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





share|improve this answer











$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
    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
    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














12












12








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





share|improve this answer











$endgroup$



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






share|improve this answer














share|improve this answer



share|improve this answer








edited Dec 24 '18 at 13:07

























answered Dec 23 '18 at 16:33









EdwardEdward

46.7k378212




46.7k378212












  • $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
    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
    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


















  • $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
    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
    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
















$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$
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




$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




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
Dec 24 '18 at 3:05






$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
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
Dec 24 '18 at 3:08






$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
Dec 24 '18 at 3:08






2




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




$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.






share|improve this answer











$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
















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.






share|improve this answer











$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














4












4








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.






share|improve this answer











$endgroup$



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.







share|improve this answer














share|improve this answer



share|improve this answer








edited Dec 24 '18 at 0:53

























answered Dec 23 '18 at 15:07









ReinderienReinderien

4,140822




4,140822








  • 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














  • 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








4




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'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$
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




$begingroup$
@PeterCordes You're right; I've noted as much in an edit.
$endgroup$
– Reinderien
Dec 24 '18 at 0:54




2




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




$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




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




$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


















draft saved

draft discarded




















































Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210217%2fncurses-tic-tac-toe-with-simplistic-ai%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

Plaza Victoria

Puebla de Zaragoza

Musa