Jump to content
  • Advertisement
Sign in to follow this  
orcfan32

Constructive Criticism: Tic-Tac-Toe

This topic is 4849 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hey all! This is my very first game, and yes, I'm very excited. This code is GUARANTEED to run on any Windows XP system using Dev-C++. I would like some criticism as to improve my future games and make them more efficient.
#include <iostream>     // For printing text
#include <windows.h>    // For Sleep() and Abort() functions
using namespace std;    // Made so that it don't have to keep putting std::<command>

// Declaration of variables
char BoardSpaces[10] = {'0','1','2','3','4','5','6','7','8','9'};
char CurrentPlayer = 'X';
char Winner = 'a';
int CMove = 0;
int Moves = 0;
bool Win = false;

void Check()             // The heart of the game: checking for a winner
{                        // ALL possible moves are here, I figured them all
    if ( Moves >= 5 )    // with pen and paper.
    {
       if ( BoardSpaces[1] == 'X' && BoardSpaces[2] == 'X' && BoardSpaces[3] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[1] == 'X' && BoardSpaces[4] == 'X' && BoardSpaces[7] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[3] == 'X' && BoardSpaces[6] == 'X' && BoardSpaces[9] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[7] == 'X' && BoardSpaces[8] == 'X' && BoardSpaces[9] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[2] == 'X' && BoardSpaces[5] == 'X' && BoardSpaces[8] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[4] == 'X' && BoardSpaces[5] == 'X' && BoardSpaces[6] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[1] == 'X' && BoardSpaces[5] == 'X' && BoardSpaces[9] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[3] == 'X' && BoardSpaces[5] == 'X' && BoardSpaces[7] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
    }
    
    if ( Moves >= 5 )
    {
       if ( BoardSpaces[1] == 'O' && BoardSpaces[2] == 'O' && BoardSpaces[3] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[1] == 'O' && BoardSpaces[4] == 'O' && BoardSpaces[7] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[3] == 'O' && BoardSpaces[6] == 'O' && BoardSpaces[9] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[7] == 'O' && BoardSpaces[8] == 'O' && BoardSpaces[9] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[2] == 'O' && BoardSpaces[5] == 'O' && BoardSpaces[8] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[4] == 'O' && BoardSpaces[5] == 'O' && BoardSpaces[6] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[1] == 'O' && BoardSpaces[5] == 'O' && BoardSpaces[9] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[3] == 'O' && BoardSpaces[5] == 'O' && BoardSpaces[7] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
    }
    
    if ( Moves >= 9 )
    {
       if ( BoardSpaces[1] == 'X' && BoardSpaces[2] == 'X' && BoardSpaces[3] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[1] == 'X' && BoardSpaces[4] == 'X' && BoardSpaces[7] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[3] == 'X' && BoardSpaces[6] == 'X' && BoardSpaces[9] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[7] == 'X' && BoardSpaces[8] == 'X' && BoardSpaces[9] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[2] == 'X' && BoardSpaces[5] == 'X' && BoardSpaces[8] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[4] == 'X' && BoardSpaces[5] == 'X' && BoardSpaces[6] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[1] == 'X' && BoardSpaces[5] == 'X' && BoardSpaces[9] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
       else if ( BoardSpaces[3] == 'X' && BoardSpaces[5] == 'X' && BoardSpaces[7] == 'X' )
       {
       Win = true;
       Winner = 'X';
       }
    }   
    if ( Moves >= 9 )
    {
       if ( BoardSpaces[1] == 'O' && BoardSpaces[2] == 'O' && BoardSpaces[3] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[1] == 'O' && BoardSpaces[4] == 'O' && BoardSpaces[7] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[3] == 'O' && BoardSpaces[6] == 'O' && BoardSpaces[9] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[7] == 'O' && BoardSpaces[8] == 'O' && BoardSpaces[9] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[2] == 'O' && BoardSpaces[5] == 'O' && BoardSpaces[8] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[4] == 'O' && BoardSpaces[5] == 'O' && BoardSpaces[6] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[1] == 'O' && BoardSpaces[5] == 'O' && BoardSpaces[9] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else if ( BoardSpaces[3] == 'O' && BoardSpaces[5] == 'O' && BoardSpaces[7] == 'O' )
       {
       Win = true;
       Winner = 'O';
       }
       else
       {
           cout<<"The winner is...";           // Prints text
           Sleep(1000);                        // Suspense
           cout<<"orcfan32?!";                 // Prints orcfan32...
           Sleep(1000);                        // Suspense
           cout<<"\nIt was a tie, so I won!";  // Hey! It's good for my reputation! (Or not...)
           Sleep(2000);                        // Keep the window open for 2 seconds
           abort();                            // Abort (exit) program
       }
    }
    
}

void DrawBoard()                               // Basically draws the playing board.
{
    system("CLS");                             // Clears all text on the console window
    // Set up the board:
    cout<<"\n\n   "<<BoardSpaces[1]<<" | "<<BoardSpaces[2]<<" | "<<BoardSpaces[3]<<"\n";
    cout<<"  ---+---+---\n";
    cout<<"   "<<BoardSpaces[4]<<" | "<<BoardSpaces[5]<<" | "<<BoardSpaces[6]<<"\n";
    cout<<"  ---+---+---\n";
    cout<<"   "<<BoardSpaces[7]<<" | "<<BoardSpaces[8]<<" | "<<BoardSpaces[9]<<"\n\n";
}

int Move(int SpaceMove)                        // A function for Moving.
{                                              // Takes the space and places the appropriate
    if ( CurrentPlayer == 'X' )                // letter.
    {
         BoardSpaces[SpaceMove] = 'X';         // Set the appropriate space to the appropriate player
         cout<<"\n\n";                         // Need to go down 2 lines
         Moves = Moves + 1;                    // Set the move + 1
         CurrentPlayer = 'O';                  // Set O as the next player
    }
          
    else if ( CurrentPlayer == 'O' )
    {
         BoardSpaces[SpaceMove] = 'O';         // Set the appropriate space to the appropriate player
         cout<<"\n\n";                         // Need to go down 2 lines
         Moves = Moves + 1;                    // Set the moves + 1
         CurrentPlayer = 'X';                  // Set X as the next player
    }         
}

int main()
{
    
/* Main consists of almost no coding. Everything here was done in previous functions. */

    while ( Win != true )
    {
          
          
          DrawBoard();
          cout<<"Player "<<CurrentPlayer<<"'s turn: ";
          cin>>CMove;
          Move(CMove);
          DrawBoard();
          Check();

    }
    cout<<"The winner is..."; // Text to print
    Sleep(1000);              // Some needed suspense....
    cout<<Winner<<"!";        // The winner to print
    Sleep(2000);              // Keep the window open for 2 seconds and close
}

P.S. I would like pointers as to where I should start next.

Share this post


Link to post
Share on other sites
Advertisement
Guest Anonymous Poster
A minor recommendation would be to disregard the number of moves that have been made and just check for 'winner' for the person that just went and use
1 2 3
4 5 6
7 8 9

Where your combinations are [1,2,3], [4,5,6], [7,8,9], [1,4,7], [2,5,8], [3,6,9], [1,5,9], [3,5,7] all of the same character rather than repeating the check in multiple areas. If you really want to check then make sure that at least 5 moves have passed

A recommendation for the next step would be to add graphics to it! Or if you don't want graphics then some dos card game / number guessing game.

Best of luck!

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
You define your Move() funcion as int Move(int SpaceMove)
{
...
} with no return. You should either have it return something or re-declare/define it as void. I haven't tested this with Dev-C++, but in VC++ 2003 I get an error because it doesn't return a value.

Share this post


Link to post
Share on other sites
Comments inline.


#include <iostream> // For printing text
#include <windows.h> // For Sleep() and Abort() functions
using namespace std; // Made so that it don't have to keep putting std::<command>

// Declaration of variables
char BoardSpaces[10] = {'0','1','2','3','4','5','6','7','8','9'};
// Ugh. Please, please learn to work with 0-based indexing rather than
// against it. You should have 9 elements (indices 0 to 8).
char CurrentPlayer = 'X';
char Winner = 'a';
int CMove = 0;
int Moves = 0;
bool Win = false;

// Instead of communicating with global variables, you should pass the board
// in as a parameter, and return the "win status" (Win and Winner) as the
// result. Also note that Win and Winner are redundant.
void Check() // The heart of the game: checking for a winner
{ // ALL possible moves are here, I figured them all
if ( Moves >= 5 ) // with pen and paper.
{
// Don't bother with that "Moves" check. It's not as though any of this is
// time-critical, so don't complicate your code.
if ( BoardSpaces[1] == 'X' && BoardSpaces[2] == 'X' && BoardSpaces[3] == 'X' )
{
Win = true;
Winner = 'X';
}
// Ed.: Snipped all the other cases here.
// Repeating yourself is BAD. Don't do it. Here, the cases could be combined
// using logical OR (||), or iterated over by using DDD techniques (have
// an array that holds data for the sets of indices to check).


if ( Moves >= 5 )
{
if ( BoardSpaces[1] == 'O' && BoardSpaces[2] == 'O' && BoardSpaces[3] == 'O' )
{
Win = true;
Winner = 'O';
}
// Ed.: Snipped more cases. There's repetition here as well: instead of
// checking for an X or O win separately, just check for a win (a row
// where the values are the same), and report the winner as the symbol
// that is used (e.g. in the first square):
// if (BoardSpaces[1] == BoardSpaces[2] && BoardSpaces[2] == BoardSpaces[3])
// for example.
}

if ( Moves >= 9 )
// Ed. : snipped repetition of all of the above checking code.
// This is completely unnecessary: if Moves >= 9, then it must be true that
// Moves >= 5; therefore you have already done the check.

if ( Moves >= 9 )
// Similarly for O's: completely unneeded.

else
{
// AGH BAD BAD BAD
// This logic isn't part of the checking procedure. What you should do
// in the case (no win detected, Moves >= 9) is just return a value
// indicating "tie game"; in the case (no win detected, Moves < 9), return
// "game still on". You can indicate the four possible game states (X win,
// O win, tie, still on) with an enumeration, for example.

cout<<"The winner is..."; // Prints text
Sleep(1000); // Suspense
cout<<"orcfan32?!"; // Prints orcfan32...
Sleep(1000); // Suspense
cout<<"\nIt was a tie, so I won!"; // Hey! It's good for my reputation! (Or not...)
Sleep(2000); // Keep the window open for 2 seconds
abort(); // Abort (exit) program
// And definitely don't use hacks like abort() except in very, very
// exceptional conditions (program is completely SNAFU and can't continue).
// What's proper is to return and let the message be handled in the same
// place where you indicate a winner (when there is one).
}
}

}

// Again, the board should be passed as a parameter here.
void DrawBoard() // Basically draws the playing board.
{
system("CLS"); // Clears all text on the console window
// Guh guh guh, don't rely on system calls. It's starting up a whole other
// program each time, and it's utterly non-portable. Just output a bunch
// of blank lines, or else look into real terminal control libraries.
// Set up the board:
cout<<"\n\n "<<BoardSpaces[1]<<" | "<<BoardSpaces[2]<<" | "<<BoardSpaces[3]<<"\n";
cout<<" ---+---+---\n";
cout<<" "<<BoardSpaces[4]<<" | "<<BoardSpaces[5]<<" | "<<BoardSpaces[6]<<"\n";
cout<<" ---+---+---\n";
cout<<" "<<BoardSpaces[7]<<" | "<<BoardSpaces[8]<<" | "<<BoardSpaces[9]<<"\n\n";
// You can make use of a for loop to simplify this.
// output leading space;
// for 3 rows:
// output leading space for line;
// output current row (do some math to figure out which indices to use);
// if not the last row, output a separator line;
}

// Again, you'd need to pass in the board here. Oh, and the return should
// be void, since there isn't anything to return. Your compiler should
// warn you for that.
int Move(int SpaceMove) // A function for Moving.
// Why "SpaceMove"? I think you want something like "Location" :/
{ // Takes the space and places the appropriate
if ( CurrentPlayer == 'X' ) // letter.
{
BoardSpaces[SpaceMove] = 'X'; // Set the appropriate space to the appropriate player
cout<<"\n\n"; // Need to go down 2 lines
Moves = Moves + 1; // Set the move + 1
CurrentPlayer = 'O'; // Set O as the next player
}
// Again, notice the repetition here. The only thing that's different in
// the two cases (the only two possible cases) is the character that's
// played (which matches the CurrentPlayer, so you don't need to branch
// for that), and the new CurrentPlayer setting (which you can handle
// easily with the ternary op, which is appropriate here - we basically
// have "conditional data" rather than "conditional code":

// BoardSpaces[SpaceMove] = CurrentPlayer;
// cout<<"\n\n"; <-- this is sketchy anyway - the drawing and/or
// input procedures should handle it instead
// Moves += 1; <-- this is more idiomatic
// CurrentPlayer = (CurrentPlayer == 'X') ? 'O' : 'X';
// Alternately, a slick hack:
// CurrentPlayer ^= ('O' ^ 'X');
// This works because 'O' ^ 'O' ^ 'X' == 'X', and 'X' ^ 'O' ^ 'X' == 'O'
// (XOR of something with itself zeroes it out).

// Ed.: Snip
}

int main()
{

/* Main consists of almost no coding. Everything here was done in previous functions. */
// Good! But it needs a bit of work... :)

// Declare globals here instead
while ( Win != true ) // Avoid writing boolean literals explicitly;
// it's overcomplicating things. Here, "while (!Win)" suffices.
// Although with a proper enumeration, you'd want to do something like
// "while (GameStatus == GAME_STILL_ON)".
{
DrawBoard(); // pass in the board
cout<<"Player "<<CurrentPlayer<<"'s turn: ";
cin>>CMove;
// This is hugely not robust: it will not check for a square being
// already occupied, and it will *totally crash and burn* if the
// user types in some garbage (try it). There are ways to fix this;
// I'll go into detail later.
Move(CMove); // also pass in the board
DrawBoard(); // Ack, the board should only be drawn once per loop!
Check(); // pass in the board, and *set the game status to the result*.
// That lets us avoid the icky globals and instead use functions the
// way they are intended to be. Generally you want to restrict things
// in scope as much as possible, and pass them around to where they
// are needed.
}
// Then here, the game status will be either of X_WIN, O_WIN or TIE_GAME
// (or whatever you call the constants/enumeration values/whatever), and
// you can handle it appropriately. Then there's no need for a messy
// abort(); all the control flows logically to the end. :)
cout<<"The winner is..."; // Text to print
Sleep(1000); // Some needed suspense....
cout<<Winner<<"!"; // The winner to print
Sleep(2000); // Keep the window open for 2 seconds and close
}

Share this post


Link to post
Share on other sites
The book i am currently reading has a tic-tac-toe game as an example i can show your the code if you want so you can see how the person who wrote the book did it.

Share this post


Link to post
Share on other sites
Ok, thanks for the replys. I think that from what Zahlman said, I'm going to work on it some more.
Also, Whoever Anonomous Poser #2 is, I never put returns. =P

Share this post


Link to post
Share on other sites
Quote:
Original post by orcfan32
Also, Whoever Anonomous Poser #2 is, I never put returns. =P


That's a bad practice, not to mention it is not accepted by many compilers.

If you're not returning a value, make the return type "void" (i.e. void Move(int SpaceMove)).

Share this post


Link to post
Share on other sites
Another suggestion would be that rather doing all of those if statements for checking the winner simply make an array of integers 24 elements long and loop through it in groups of 3s using those numbers to index into the board, then test for position.


int lines[] = { 0,1,2 , 3,4,5 , 6,7,8 , 0,3,6 , 1,4,7 , 2,5,8 , 0,4,8 , 2,4,6 };

for ( unsigned int iter = 0;
iter < 9;
iter += 3 )
{
if ( ( board[iter] == board[iter+1] ) && ( board[iter] == board[iter+2] ) }
{
if ( board[iter] = 'x' ) printf("player 1 wins!");

if ( board[iter] = 'o' ) printf("player 2 wins!");
}
}



It's much more condensed.

ace

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!