Finished Tic Tac Toe in C - Feedback on my source?

[Achithyn]

Greetings everyone,
I would first like to thank everyone who has been helping me get back into programming. All the book suggestions, project suggestions, etc. has really helped me define a good path to tread down. :)

Well, I programmed my first completed C game, which is Tic Tac Toe. I know everyone is different, and thus, programs differently, though I feel like everyone will laugh when they see my source... lol. It's 575 lines, which seems long for a tic tac toe game.

I made the AI beatable, or it was at one point. Haven't been able to beat it for a while. I tried to put quality into the game, even though it runs in command line. There may be a bug here and there, though nothing that I have found over the last few look overs.

Well, I just wanted to get some feedback on my code. I just started to learn C, so the code may not look very professional. >< Any suggestions on how to make the code better would be great - it would help me learn a great deal.

Thanks for your time!

Source:
http://www.arcane-artistry.net/programming/tictactoe.zip

~Achi

PS. This is the first time I have zipped my source and presented it for download - let me know if I did it right. :)
PPS. I hope this is the right place to post this - I have the hardest time figuring out what goes where... >< Let me know if I posted in the wrong area, and I won't make the mistake again. :)

[Blacktiger]

DISCLAIMER:All of the following are suggestions based on my personal programming style. Take what suits you. ;)

Opening the source seems to work fine. I'll look closer at the code later, but it seems pretty good for your first game. One thing that would probably help shorten your code and perhaps make things a bit more clear would be to store the tic-tac-toe board in a matrix instead of three arrays.

Instead of this:

char topBoard[3][2] = { "1", "2", "3" };
char midBoard[3][2] = { "4", "5", "6" };
char botBoard[3][2] = { "7", "8", "9" };


Try this:

char board[3][3];


Your setup board function would change to a display board function:

void displayBoard()
{
printf( "\n/------Play Against-------\\\n" );
printf( ".-----------CPU-----------.\n" );
printf( "/\\ /-------\\ /\\\n" );
printf( "-- TIC --| %s %s %s |-- TIC --\n", board[0][0], board[0][1], board[0][2] );
printf( "|| TAC | %s %s %s | TAC ||\n", board[1][0], board[1][1], board[1][2] );
printf( "-- TOE --| %s %s %s |-- TOE --\n", board[2][0], board[2][1], board[2][2] );
printf( "\\/ \\-------/ \\/\n" );
printf( "\t%s = X || CPU = O\n\n", userName );
}


You would need a new setup board function that took no arguments
(or possibly a board argument):

void setupBoard()
{
board = { '1', '2', '3',
'4', '5', '6',
'7', '8', '9'
};
}

Then to test whether a cats game has occurred you can use two loops.

void checkCAT()
{
bool cats = true;
for(int row = 0; row < 3; row++)
{
for(int col = 0; col < 3; col++)
{
if(board[row][col] != 'X' && board[row][col] != 'O')
// This space is unused
cats = false;
}
}
}

(I can actually think of a couple other ways to do this. For example, you could code the cats game check to stop once it found spot that wasn't taken yet.)

I also noticed that you put the keyword void in functions that have no arguments. While that is perfectly valid C, why not just remove the keyword completely as its less typing and is just as clear.

I subscribe to the idea that code should be written in such a way that its intent is obvious, but that's not always possible or desirable. I do suggest that whenever you create a function you put not only a description of what the function does above it, but also what assumptions are made about the state of the program. It will help a lot when you have to go back and debug.

[ThemsAllTook]

Indeed, pretty nice for a first try. It's really easy to read through the code and see what's going on. There were a few formatting/indenting issues, but nothing stuck out to me as particularly egregious.

In addition to Blacktiger's post, one suggestion I'd make is to try to use loops whenever they can reduce code duplication. Certain functions in your code (PlayerTurn, CheckWin, and CPUAI in particular) have what looks like an unrolled loop written in them - largely the same block of code duplicated several times with different values in each. If you need to change the logic, you'd have to make your change 9 times instead of just one. You also have a bit more potential for error than if your logic was all in one place.

With practice, you'll undoubtedly figure out what works best on your own. It definitely looks like you're on the right track. Keep it up! :)

[unknown]

Totaly super!
Very finished, and a formidable computer opponent.

[Zekaric]

void displayBoard()
{
printf( "\n/------Play Against-------\\\n" );
printf( ".-----------CPU-----------.\n" );
printf( "/\\ /-------\\ /\\\n" );
printf( "-- TIC --| %s %s %s |-- TIC --\n", board[0][0], board[0][1], board[0][2] );
printf( "|| TAC | %s %s %s | TAC ||\n", board[1][0], board[1][1], board[1][2] );
printf( "-- TOE --| %s %s %s |-- TOE --\n", board[2][0], board[2][1], board[2][2] );
printf( "\\/ \\-------/ \\/\n" );
printf( "\t%s = X || CPU = O\n\n", userName );
}
I'm not sure how many people know this but this can be rewritten to be just one call instead of the 8 that is used. void displayBoard()
{
printf(
"\n" /* Strings get concatenated by the compiler. */
"/------Play Against-------\\\n"
".-----------CPU-----------.\n"
"/\\ /-------\\ /\\\n"
"-- TIC --| %s %s %s |-- TIC --\n"
"|| TAC | %s %s %s | TAC ||\n"
"-- TOE --| %s %s %s |-- TOE --\n"
"\\/ \\-------/ \\/\n"
"\t%s = X || CPU = O\n\n",
board[0][0], board[0][1], board[0][2],
board[1][0], board[1][1], board[1][2],
board[2][0], board[2][1], board[2][2],
userName);
}
Sorry, I haven't looked at the code yet. Maybe I'll get a chance tonight.

[imikedaman]

I think he did that just to have the text lined up nicely in the source, although technically your way is lined up too. Meh...

[OneSadCookie]

I also noticed that you put the keyword void in functions that have no arguments. While that is perfectly valid C, why not just remove the keyword completely as its less typing and is just as clear.

That would be incorrect. This is an area of difference between C and C++:

// C
void foo(void); // foo takes no arguments
void foo(...); // foo takes a variable number of arguments
void foo(); // foo takes a variable number of arguments (bad style)


// C++
void foo(void); // foo takes no arguments (bad style)
void foo(...); // foo takes a variable number of arguments
void foo(); // foo takes no arguments

[Zekaric]

Just to nit pick. For variable argument list functions you need at least one parameter. Unless that has changed in C++ or a more modern C.void foo(int FirstArg, ...);This is because the functions/macros to get to the variable portion needs something to start with. Nit pick mode off.

[OneSadCookie]

You do need at least one argument if you want to use stdarg to extract the values, but this is (unfortunately) perfectly legal:

// header
void foo(...);

//implementation
void foo(int arg0, double arg2)
{
// ...
}

This will work fine if the function is called as

foo(3, 3.0);

but blow up horribly somehow if it's called as

foo(3, 3);

Make sure your prototypes match your definitions ;)

[Achithyn]

Greetings all,
Thank you all for your feedback and suggestions. :) I'm going to comment on everything up to post #7, since you all kind of lose me at that point. lol. I added the extra ( void ); only because that is how my book does it. :p It doesn't take long to type it in, so no worries there.

@Blacktiger
I didn't know I could setup the board with one char array. If I did, I would have done that. >< I thought that setupBoard[3][3] would be three lines of text that were three characters long, with the trailing \0. So, if I called setupBoard[3][0], would it need an 'end of line' marker \0? [3][3] is setting up to reference 9 characters, though only has three \0's - I guess that is what I feel is the hardest for me to understand... what \0 actually does, and when you need to pay attention to it, and when you don't.

I'm not sure, though would the following code actually work? Is 'board' a pointer? I just thought that you couldn't assign char's like that, less it was when the char was defined and initialized. So, something like userName = "Foo"; wouldn't work, right?

void setupBoard(
{
board = { '1', '2', '3',
'4', '5', '6',
'7', '8', '9'
};
}
I think your cat loop is very well thought out. However, it doesn't save too many lines of code... and (to me) :sneaky: can be kind of confusing to follow instead of what I had:
void CheckCat( void )
{
if ((strcmp(topBoard[0], "1") != 0 ) &&
(strcmp(topBoard[1], "2") != 0 ) &&
(strcmp(topBoard[2], "3") != 0 ) &&
(strcmp(midBoard[0], "4") != 0 ) &&
(strcmp(midBoard[1], "5") != 0 ) &&
(strcmp(midBoard[2], "6") != 0 ) &&
(strcmp(botBoard[0], "7") != 0 ) &&
(strcmp(botBoard[1], "8") != 0 ) &&
(strcmp(botBoard[2], "9") != 0 ))
{
printf( "CATS Game!\n");
gameStatus = false;
}
}I can see how a loop, such as what you had, could work better in some cases though. I am sure that later on, as I grow more and more used to syntax and the use of it, the loop in your example would be more ideal. Honestly, I don't really understand why my code works... can someone win by taking the last place on a tic tac toe board? Or are all victories and defeats determined before all places are taken up? If the last move was played in the last place available on the board, and that play would have constituted as a win, then would the program show: Winner! 'or' Cats! (?)

@ThemsAllTook
Thank you for your feedback. :) I'm glad the code was easy to read... perhaps that is because I need it to be easy to read for me to understand how it works. :p As for loops, I agree. I need to try to understand how I can use loops more to benefit my code. I know all about loops, though I don't know how to effectively and efficiently use them. I'll work on that. : )

@Unknown
Lol, I'm glad you liked it. :p It's funny, I have a member on my forum named Unknown... or was before I reset the site a few months ago. Perhaps your name is common on the net... less you enjoy creative writing?

@Zekaric
I did not know I could have done all that with use one printf function... blah! I think your revision of the code is very nice. Next time I am tempted with using several printf's in a row, I will remember what you said - and your good example. Thanks!

@imikedaman
Lol, unfortunately I did not know about the printf trick. ><

Well, thank you all for your feedback! It was really helpful. I keep coming here and asking for help - is there anything I can do to help the community? Well, let me know! Have a good one!

~Achi

[unknown]

haha Achithyn, I love creative writing! I only read it though, My english is not good enough :\

[Achithyn]

Ah, sorry to hear... >< Though, if you are a programmer, I am sure you can do anything. :p A good grammar book and a lot of writing will do wonders. :) Do as you want in life, for you can only live it once - that's kind of my motto at the moment... :)

~Achi

[Blacktiger]

That would be incorrect. This is an area of difference between C and C++:

// C
void foo(void); // foo takes no arguments
void foo(...); // foo takes a variable number of arguments
void foo(); // foo takes a variable number of arguments (bad style)


// C++
void foo(void); // foo takes no arguments (bad style)
void foo(...); // foo takes a variable number of arguments
void foo(); // foo takes no arguments

Your code ninja skills got me there! :ninja:

[Zekaric]

You wrote...char topBoard[3][2] = { "1", "2", "3" };
char midBoard[3][2] = { "4", "5", "6" };
char botBoard[3][2] = { "7", "8", "9" };
...
if ( (marker == 1) && (strcmp(topBoard[0], "1") == 0))
{
...
As someone pointed out this is a bit clumsy. You could have done...char topBoard[] = "123";
char midBoard[] = "456";
char botBoard[] = "789";
...
if ((marker == 1) && (topBoard[0] == '1'))
{
...strcmp is slow. This way we are dealing with 3 arrays of chars. Comparing char values which in short is comparing very small integers instead of strings. Much faster (not that it matters here but you avoid the strcmp() function call which can trip people up.) A better way would be to use one array as someone else pointed out. char board[] = "123456789";
...
if (marker == 1 && board[0] == '1')
{
...Similarly the players could be changed to just be chars... char playerOne = 'X';


You wrote... if ( (marker == 1) && (strcmp(topBoard[0], "1") == 0))
...
if ( (marker == 2) && (strcmp(topBoard[1], "2") == 0))
...
if ( (marker == 3) && (strcmp(topBoard[2], "3") == 0))
...
if ( (marker == 4) && (strcmp(midBoard[0], "4") == 0))
...
if ( (marker == 5) && (strcmp(midBoard[1], "5") == 0))
...
if ( (marker == 6) && (strcmp(midBoard[2], "6") == 0))
...
if ( (marker == 7) && (strcmp(botBoard[0], "7") == 0))
...
if ( (marker == 8) && (strcmp(botBoard[1], "8") == 0))
...
if ( (marker == 9) && (strcmp(botBoard[2], "9") == 0))
{
strcpy( botBoard[2], "X" );
return 0;
}Which can be compacted down to just...(using the last code in the above section)if (1 <= marker && marker <= 9 && board[marker - 1] == '0' + marker)
{
board[marker - 1] = 'X';
return 0;
}Similarly for the replay() functionfor (i = 0; i < 9; i++) {
board[i] = '1' + i;
}


For the most part you shouldn't have too much trouble but usually when passing in arrays of chars.../* Compiler may generate warnings if you up the warning level.*/
void function(char array[20]);

/* No warnings */
void function(char *array);


I'd stay away from "Magic Numbers". Like return 0; which was used above. Start using...#define FALSE 0
#define TRUE 1And enums to avoid them. Basically give them meaning for future sanity sake.


You can cause a buffer overflow if you type in a name that is larger than 49 characters but that is just being fussy.

[Blacktiger]

I didn't know I could setup the board with one char array. If I did, I would have done that. >< I thought that setupBoard[3][3] would be three lines of text that were three characters long, with the trailing \0. So, if I called setupBoard[3][0], would it need an 'end of line' marker \0? [3][3] is setting up to reference 9 characters, though only has three \0's - I guess that is what I feel is the hardest for me to understand... what \0 actually does, and when you need to pay attention to it, and when you don't.

Ah, let me help you with the \0 thing. First, what I created was a character array (or an array of characters depending on how you want to think of it...). Since we know what the size of the board is at all times, we can just use the size to correctly index any position. \0 is the null character and is used when working with character strings (c-strings). To create a character you use single quotes ('c') and you get back an integer value that represents that character (basically). To create a character string you use double quotes ("string") and you get back an array of characters that is exactly as long as the string plus one more for the null character.

{ s, t, r, i, n, g, \0 }


I'm not sure, though would the following code actually work? Is 'board' a pointer? I just thought that you couldn't assign char's like that, less it was when the char was defined and initialized. So, something like userName = "Foo"; wouldn't work, right?


Quick warning: This part might be more advanced than you are ready for but since you asked...

Actually you are closer to the answer than you think... arrays are actually pointers!

You may have noticed that C will let you access elements 'outside' an array. I believe this is called a buffer overflow error. When you declare an array, space is allocated on your computer for exactly the size of the array, but since an array is a pointer, C has no way of knowing how large the array actually is. In fact, if you want you can do dangerous stuff like:

while(true)
{
anArray++;
printf("%s", anArray[0])
}


So my setupBoard function actually creates a new array and then uses our character array pointer to point to it. Actually I think %s with the characters might break... %s expects a null terminated string. %c is probably the one you should use if you are using characters. (Sorry about the sloppy code on my part, I haven't used C/C++ in almost a year! My current classes have used Java, Scheme, and Perl.)


I think your cat loop is very well thought out. However, it doesn't save too many lines of code... and (to me) :sneaky: can be kind of confusing to follow instead of what I had:
I can see how a loop, such as what you had, could work better in some cases though. I am sure that later on, as I grow more and more used to syntax and the use of it, the loop in your example would be more ideal.


Largely I wanted to put that there to show you how to loop through a 2d array safely as you will probably find other places where that is more useful.


Honestly, I don't really understand why my code works... can someone win by taking the last place on a tic tac toe board? Or are all victories and defeats determined before all places are taken up? If the last move was played in the last place available on the board, and that play would have constituted as a win, then would the program show: Winner! 'or' Cats! (?)


I think someone can win by taking the last place, but its very rare since a smart opponent will force you to block usually. Looking at your code, it appears that if the last piece was placed on the board and it won you should see "Winner!"; but you would then see "Cats!".

There are a probably a bunch of ways to prevent this (although if there is no such game it doesn't matter); the easiest one I see is to change the CheckWin function to return a boolean value of true if the player being tested won. Then in your main function do something like:

if( CheckWin(playerOne) )
printf("You won!");
else
CheckCat();


Whew, I think that's enough for now. :zzz:

[Zekaric]

I would also mention that, even though this is such a tiny program, one file, it could have been compartmentalized better. If you code a more difficult game, this stream of consciousness style of coding will soon cause you a great deal of pain. My suggestion would be to isolate areas of the program into logical modules/units/engines/whatever so that modification of the game at a later date becomes oh so much easier. Debugging and enhancements to the game become much more trivial to make. Less and less copy code will result which mean less and less errors. Bugs are easier to find as you will most likely know where the bug is happening (Ok not always but usually it'll give you a focus on where to look.)

If I was to refactor the game I would split it up into 3 or 4 modules. (ttt prefix for Tic-Tac-Toe)

ttt - module for data manipulation and management.
tttui - module for user interface handling.
tttdisplay - module for displaying of ttt information.
tttai - module for the artificial intelligence.

In your case, with just the use of console IO, I'd probably combine tttui and tttdisplay since tttdisplay is so very trivial. However in a more complex game, the display of the board may be complicated enough to warrent it's own module, like say, drawing to OpenGL which could possibly be quite unique from the cocoa/carbon ui code.

These modules would look something like this in my world... (Just the headers not the actual code but the idea will hopefully shine through. There would be a matching .c file for each header which you can probably guess what's on the inside of those functions. Also note, I'm assuming a C program not C++ or Obj-C)
/* ttt header */

/* In case this header file is included more than once via via...
** This will ensure that the header is only processed once. It
** avoids compiler warnings and errors.*/
#if !defined(TTT_HEADER)
#define TTT_HEADER

/* In case this module is ever included in a C++ project.*/
/* C++ code.********************************************* ***********/
#ifdef __cplusplus
extern "C" {
#endif
/* C++ code.********************************************* ***********/

/* Constants */
typedef enum {
tttPositionTOP_LEFT,
tttPositionTOP_CENTER,
tttPositionTOP_RIGHT,
tttPositionMID_LEFT,
tttPositionMID_CENTER,
tttPositionMID_RIGHT,
tttPositionBOT_LEFT,
tttPositionBOT_CENTER,
tttPositionBOT_RIGHT,

tttPositionCOUNT
} TttPosition;

typedef enum {
tttValueNONE,
tttValueO_PLAYER,
tttValueX_PLAYER
} TttValue

/* Types */
/* I put the types in the header just so debugging is simple.
** no code outside the c module associated with this header
** will ever touch the guts of the types. Everything done
** via the API to the type. The reason for that is that it
** becomes easier to find out where somethign is changing.
** if internal data is changing, it should only ever be in
** an API call so only 1 break point is ever needed.*/
typedef struct {
char board[tttPositionCOUNT];
} Ttt;

/* (api) Functions */
/* Create a new board. */
Ttt *tttCreate( void);

/* Clean up. */
void tttDestroy( Ttt *ttt);

/* Get the value at a cell.*/
TttValue tttGet( Ttt *ttt, TttPosition pos);

/* Test if there is a winner.*/
TttValue tttIsWon( Ttt *ttt);
/* Test if there is a stalemate/cats */
int tttIsStaleMate(Ttt *ttt);
/* Something I like to do with modules for proper startup.*/
int tttIsStarted( Ttt *ttt);

/* Set the value at a cell.*/
void tttSet( Ttt *ttt, TttPosition pos, TttValue value);
/* Start the ttt module. Set up any internal states if needed.*/
int tttStart( void);
/* Stop the ttt module. Clean up. */
void tttStop( void);

/* Matching the code near the top. */
/* C++ code.********************************************* ***********/
#ifdef __cplusplus
}
#endif
/* C++ code.********************************************* ***********/

/* Matching the #if !defined(TTT_HEADER) above.*/
#endif


/* tttui header */
#if !defined(TTTUI_HEADER)
#define TTTUI_HEADER

/* includes */
/* Needed for the Ttt typedef.*/
#include "ttt.h"

/* C++ code.********************************************* ***********/
#ifdef __cplusplus
extern "C" {
#endif
/* C++ code.********************************************* ***********/

/* Types */
typedef struct {
char name[50];
} Tttui;

/* Functions */
/* I could put down, tttuiCreate and tttuiDestroy but since there will only
** be one ui element ever then might as well just make it a static inside
** the .c module and manipulated via the other functions here.*/

/* Module started state */
int tttuiIsStarted( void);

/* Print the board. */
void tttuiProcessBoard( Ttt *ttt);
/* Get the player's move */
TttPosition tttuiProcessMove( void);
/* Get the player name */
int tttuiProcessName( void);
/* Print the title/welcome message */
void tttuiProcessWelcome( void);

/* Maybe other functions depending on what I missed. */

/* Start up the internal routines. I.E. initialize a Tttui variable.*/
int tttuiStart( void);
/* Clean up the module stuff.*/
void tttuiStop( void);

/* C++ code.********************************************* ***********/
#ifdef __cplusplus
}
#endif
/* C++ code.********************************************* ***********/

#endif


/* tttai header */
#if !defined(TTTAI_HEADER)
#define TTTAI_HEADER

/* includes */
#include "ttt.h"

/* C++ code.********************************************* ***********/
#ifdef __cplusplus
extern "C" {
#endif
/* C++ code.********************************************* ***********/

/* Functions */
/* You get the picture */
int tttaiIsStarted( void);

/* The computer makes a move. */
int tttaiProcess( Ttt *ttt);

int tttaiStart( void);
void tttaiStop( void);

/* C++ code.********************************************* ***********/
#ifdef __cplusplus
}
#endif
/* C++ code.********************************************* ***********/

#endif


Now a C++ and Obj-C solution may use some niceties of their language but in general the idea is the same. Break the program down into logical components. For larger programs it will be a must or else you will find yourself freaking out with it's complexity and unmaintainability.

[backslash]

I'm not sure, though would the following code actually work? Is 'board' a pointer? I just thought that you couldn't assign char's like that, less it was when the char was defined and initialized. So, something like userName = "Foo"; wouldn't work, right?

void setupBoard()
{
board = { '1', '2', '3',
'4', '5', '6',
'7', '8', '9'
};
}
You're absolutely right here. board could be declared like that when it was initialised, but then couldn't be changed later, and would therefore be useless for your purposes. Also although board is a pointer, it was declared as an array, and so cannot be changed to point at another part of memory later (which is what that function would do if it was legal). The function you would need instead is:

void setupBoard(void)
{
memcpy(board, "123456789", 9);
}

Using memcpy() rather than strcpy() means you don't need extra space in board[] to store the '\0' that would be on the end of the string in the function call. Ofcourse, calling a function with only one line of code in it is a bit of a waste of time. ;)

[Achithyn]

Greetings everyone,
You've all blown me away with your help. :) I'm not sure where to comment and where not to... though I can say that I've learned a lot from the examples you've all provided. I'm almost tempted to reprogram my version of Tic Tac Toe, to include the things that I've learned, though I'm kind of sick of TTT now, looking to work on something new.

I'll be working on a new program that records the users thoughts concerning anime and stores them into a file. The only problem is the book I'm reading: Learn C on the Mac by Dave Mark, 10th chapter on files, source code, does not work as it should. I'm sure I can find the correct way in one of my other books (library seems to be my new friend...). :p

Thanks for the detailed source examples you guys cooked up for me. Some of it seems so practical, while other parts seems confusing. Right now I'm having problems understanding structs, with pointers, in... functions... etc. As I've heard, pointers can often be the most confusing. Well, I'm slowly working my way through them. :)

Anyway, I've bookmarked this thread so I can come back and check out the code and comments you've all left. there's a lot of good content here, and will take several times of reading to understand it clearly.

Well, thanks again! I need to be off to get some reading a programming done today. :) So, you all take care.

~Achithyn