First try at TicTacToe

Started by
14 comments, last by MikeWulf 18 years, 10 months ago
Hey. New here. Great, friendly community. On to buisness... I've been learning C++ as my first language for about a month now and have just finished my first 'game' a dos Tic Tac Toe using dev-c++ compiler. It's got no computer controlled movement. I know my code is pretty bad so you don't have to spell that out. I was just wondering if I have grasped the general idea or if I am going off into some stupid hole that I should have seen comming and any optimisations that are obvious to you (and should be to me). I've commented anything that is causing me problems.

/*
TicTacToe by Wulf :)
The basic idea is the board is updated and redisplayed (as I don't know a way
in which I can clear the screen)
42 or * is the unused spaces
79 is player O
88 is player X
*/

#include <iostream>
using namespace std;

void showboard(); // Shows the gameboard
int getinput(); // Gets valid input
int convert(int coord); // Converts the coordinates into a single value
int makemove(int move); // Makes the move
int wincon(); // Holds winconditions

// I know globals are meant to be bad but in this case arn't they best?
char tic[9];
char player = 79;

int main() {
 int move; // Should this have a pointer?
 char win;
 int i; 

 
 do {
 win = 0;
 
 for(i=0; i<9; i++) tic = 42; // Set all pieces to default
 showboard();
 do {
  do {
   move = getinput();
   if(move == 0) {
    cout << "Thanks for playing!";
    return 0;
   }
   move = convert(move);
   move = makemove(move);
  } while(move == 50);
  win = wincon();
  showboard();
  // Switch the players around
  switch(player) { 
   case 88:
     player = 79;
     break;
    case 79:
     player = 88;
     break;
   }
 } while(win == 0);
 /* switch the players around so that it displays the right person as the
  winner and makes the loser the first to move if you play again */
 switch(player) {
   case 88:
     player = 79;
     break;
    case 79:
     player = 88;
     break;
   }
 cout << "The winner is player " << player << "!\n"; 
 
 cout << "\n\nPlay again? (y n): ";
 cin >> win ;
 } while(win == 'y');
 
 return 0;
}

// ----------------Gets the input here-----------------
int getinput() {
 int input;
 do {
  cout << "\nPlayer - " 
       << player 
       << " - Give your move in the form of 'yx' (0 to quit): ";
  cin >> input;
  // I couldn't work out how to do this the right way or a better way...
  if(input == 0) return 0;
  else if((input == 11) || (input == 12) || (input == 13) || 
            (input == 21) || (input == 22) || (input == 23) || 
            (input == 31) || (input == 32) || (input == 33)) return input;
   else {
   cout << "\nInvalid input!\n";
   showboard();
   input = 0;
  }
 } while(input == 0);
}

// -----------Converts the coordinates to a single value.---------------
// There has to be a better way than this!
int convert(int coord) {
 switch(coord) {
 case 11:
  coord = 0;
  break;
 case 12:
  coord = 1;
  break;
 case 13:
  coord = 2;
  break;
 case 21:
  coord = 3;
  break;
 case 22:
  coord = 4;
  break;
 case 23:
  coord = 5;
  break;
 case 31:
  coord = 6;
  break;
 case 32:
  coord = 7;
  break; 
 case 33:
  coord = 8;
  break;
 default:
  cout << "This can't have happened! ERROR!!!!!!!!!";
  break;
 }
 return coord;
}
    
// -----------------Makes the move-------------------
int makemove(int move) {
 if(tic[move] == 42) tic[move] = player;
 else {
  cout << "\nSpace already taken!\n";
  return 50;
 }
}

// -----------------------Show the board---------------------------
void showboard() {
 cout << "\n 123\n"
      << "1" << tic[0] << tic[1] << tic[2] << "\n"
      << "2" << tic[3] << tic[4] << tic[5] << "\n"
      << "3" << tic[6] << tic[7] << tic[8] << "\n";
}

//------------------------Win conditions------------------------
int wincon(){
 int i;
 for(i=0; i<9; i = i+3) {
  // if((tic == (tic[i+1]) && (tic[i+2])) && ((tic && tic[i+2] && tic[i+2]) != 42)) {
  if(player == tic) if(player == tic[i+1]) if(player == tic[i+2]) {
  return player;
  }
  if(player == tic) if(player == tic[i+3]) if(player == tic[i+6]) {
  return player;
  }
  // Shouldn't be in the loop I know...
  if(player == tic[0]) if(player == tic[4]) if(player == tic[8]) { 
  return player;
  }
  // Yes...I know...
  if(player == tic[2]) if(player == tic[4]) if(player == tic[6]) {
  return player;
  }
 }
 return 0;
}


I really want to move onto a graphical game (maybe pong, eh?) so if you think I am ready, say it, and point me in the right direction. Thanks! <edit> I also seem to get this '[Warning] no newline at end of file' in dev. What is the importance of the newline?</edit>
Advertisement
The cod eitself doesnt look to bad. IMHO though your formatting is horrible to the point of making it entirely too hard to read. You need more white space. Bigger tabs would be nice. Also if your gonna use your braces ilke you do put some whitespace under them. I find that putting braces on lines by them selves help delineate sections of code. So if I had written your code it would look like this:

int main() {    int move; // Should this have a pointer?    char win;    int i;     do     {        win = 0;        /*for(i=0; i<9; i++) tic = 42;*/ //one liners are a bad habit!                 // Set all pieces to default        for(i = 0; i < 9; i++)        {            tic = 42;        }                    showboard();        do        {            do            {                move = getinput();                if(move == 0)                 {                    cout << "Thanks for playing!";                    return 0;                }                                move = convert(move);                move = makemove(move);                            } while(move == 50);                        win = wincon();            showboard();                        // Switch the players around            switch(player)             {                 case 88:                    {                        player = 79;                        break;                    }                    case 79:                    {                        player = 88;                        break;                    }                }                } while(win == 0);         /* switch the players around so that it displays the right person as the        winner and makes the loser the first to move if you play again */        switch(player)         {            case 88:            {                player = 79;                break;            }                case 79:            {                player = 88;                break;            }            }                 cout << "The winner is player " << player << "!\n";          cout << "\n\nPlay again? (y n): ";        cin >> win ;        } while(win == 'y');     return 0;}


Isnt that easier to read? the number of tabs tell you how deep code id in a loop so you can easy tell whats what. Formatting is really upto personal preference though so do whatever your comfortable with. Just remember that alot of coede you write youll go backto one day or another. If its look like what you posted itll confuse the hell outta you. Nevertheless it looks pretty good.

A couple more pointers. using namespace should be avoided. when you need something form a namespace either type namespace::whatever(); or do
a using namespace std::cin; to avoid typing it for cin. All I can really say though is work on readability. For graphics you should check out SDL and allegro. I highly recomend trying to make pong with both them, as one is going to suit your style better than the other and only you can figure that out.
There, I reformatted it with the amazing power of Visual Studio's "Format Selection" command :)

/*TicTacToe by Wulf :)The basic idea is the board is updated and redisplayed (as I don't know a wayin which I can clear the screen)42 or * is the unused spaces79 is player O88 is player X*/#include <iostream>using namespace std;void showboard(); // Shows the gameboardint getinput(); // Gets valid inputint convert(int coord); // Converts the coordinates into a single valueint makemove(int move); // Makes the moveint wincon(); // Holds winconditions// I know globals are meant to be bad but in this case arn't they best?char tic[9];char player = 79;int main() {	int move; // Should this have a pointer?	char win;	int i; 	do {		win = 0;		for(i=0; i<9; i++) tic = 42; // Set all pieces to default		showboard();		do {			do {				move = getinput();				if(move == 0) {					cout << "Thanks for playing!";					return 0;				}				move = convert(move);				move = makemove(move);			} while(move == 50);			win = wincon();			showboard();			// Switch the players around			switch(player) {    case 88:	   player = 79;	   break;   case 79:	   player = 88;	   break;			}		} while(win == 0);		/* switch the players around so that it displays the right person as the		winner and makes the loser the first to move if you play again */		switch(player) {   case 88:	   player = 79;	   break;   case 79:	   player = 88;	   break;		}		cout << "The winner is player " << player << "!\n"; 		cout << "\n\nPlay again? (y n): ";		cin >> win ;	} while(win == 'y');	return 0;}// ----------------Gets the input here-----------------int getinput() {	int input;	do {		cout << "\nPlayer - " 			<< player 			<< " - Give your move in the form of 'yx' (0 to quit): ";		cin >> input;		// I couldn't work out how to do this the right way or a better way...		if(input == 0) return 0;		else if((input == 11) || (input == 12) || (input == 13) || 			(input == 21) || (input == 22) || (input == 23) || 			(input == 31) || (input == 32) || (input == 33)) return input;		else {			cout << "\nInvalid input!\n";			showboard();			input = 0;		}	} while(input == 0);}// -----------Converts the coordinates to a single value.---------------// There has to be a better way than this!int convert(int coord) {	switch(coord) { case 11:	 coord = 0;	 break; case 12:	 coord = 1;	 break; case 13:	 coord = 2;	 break; case 21:	 coord = 3;	 break; case 22:	 coord = 4;	 break; case 23:	 coord = 5;	 break; case 31:	 coord = 6;	 break; case 32:	 coord = 7;	 break;  case 33:	 coord = 8;	 break; default:	 cout << "This can't have happened! ERROR!!!!!!!!!";	 break;	}	return coord;}// -----------------Makes the move-------------------int makemove(int move) {	if(tic[move] == 42) tic[move] = player;	else {		cout << "\nSpace already taken!\n";		return 50;	}}// -----------------------Show the board---------------------------void showboard() {	cout << "\n 123\n"		<< "1" << tic[0] << tic[1] << tic[2] << "\n"		<< "2" << tic[3] << tic[4] << tic[5] << "\n"		<< "3" << tic[6] << tic[7] << tic[8] << "\n";}//------------------------Win conditions------------------------int wincon(){	int i;	for(i=0; i<9; i = i+3) {		// if((tic == (tic[i+1]) && (tic[i+2])) && ((tic && tic[i+2] && tic[i+2]) != 42)) {		if(player == tic) if(player == tic[i+1]) if(player == tic[i+2]) {			return player;		}		if(player == tic) if(player == tic[i+3]) if(player == tic[i+6]) {			return player;		}		// Shouldn't be in the loop I know...		if(player == tic[0]) if(player == tic[4]) if(player == tic[8]) { 			return player;		}		// Yes...I know...		if(player == tic[2]) if(player == tic[4]) if(player == tic[6]) {			return player;		}	}	return 0;}
Cool. I've only read from the same book and it keeps switching between lots of different formatting styles so I kinda just stuck with the lower whitespace. With the namespace thing, why?
<edit> The auto formatting looks alright until I look at the switch's. That is some wacky stuff to me. </edit>

[Edited by - MikeWulf on May 27, 2005 2:09:33 PM]
Name spaces are there so that things dont interfere with eachother. By doing using namespace withjust a namespace you effectivly negate that effect. Granted you probably arent going to name anything cin or cout, but in larger programs that use things from multiple name spaces things would get real messy if you just stripped away all the name spaces(by doing using namespace) many things could even conflict. I doubt this is a good enough explanaition but im sure someone here will clear it up more.


Edit: the auto formatting isnt that bad...And you can get plugins for dev-cpp that do the same thign. But... I really think it ought to be habit that yuo do it right the first time, it'll make spotting error 100 times easier.
To solve you nasty switch statement in convert coords try something like this.

instead of taking in y and x at once you could take them in seperatly and do this:
where y is the row # and x is the column #

int board_width = 3;
int place_on_board = y * board_width + x;

you board would have to be laid out like this:
        c0  c1  c2row 0   0   1   2row 1   3   4   5row 2   6   7   8
For that to work I would need to either ask the user to input the first coord then the second or use something to seperate the two digits into two variables...which I don't know how to do.
I looked over your code. It wasn't bad, but it wasn't good. So, I took the pleasure of rewriting most of it. I didn't go all out, but I tried to make it better, to an extent. Note the code is untested, and incomplete. I didn't redo everything. Those things which I left unaltered I noted in the code, so you'll know.

Also, I tried to comment it well enough so you'd know why I did what I did. Anyway, here it is:

#include <iostream>// It's best not to use entire namespaces, to avoid name clashes.// Rather, use just those componenets which you need:using std::cout;using std::cin;// An easy way of storing coordinates:struct Coord{	Coord() : x( 0 ), y( 0 ) {}	int x, y;		// There's no need to do the huge switch statement.		// Here's why:		//  0 1 2	//  _ _ _	// |_|_|_| 0	// |_|_|_| 1	// |_|_|_| 2		// To convert { column 1, row 2 } to a single int,	// start at the x-offset into the table (column 1), and go down y rows.	// To go down y rows, you simply add the number of cells which	// would make up y rows, in this case y * 3, because a row	// is made up of 3 cells.	// So, the above example would be 1 + 2 * 3 = 7,	// provided the first cell is {0, 0}.		// In code:	const int ToSingleInt() const { return x + ( y * 3 ); }};// Instead of having game data trashed about in the global namespace,// let's encapsulate it into a nice class:class GameBoard{public:	// Instead of using numbers to differentiate between cells,	// use constants:	enum CellContents { // An 'enum' is simply an enumeration of constants,	       // hence the name :)			UNUSED = 0,		PLAYER_X,		PLAYER_O	};	       GameBoard()       {		       // set all cells to unused:	       Reset();       }		// Put the reset logic into it's own function:	void Reset() const	{			for ( int i = 0; i < 9; ++i )			m_boardData = UNUSED; <span class="cpp-comment">// does having constants</span><br>		                                   <span class="cpp-comment">// make it so much easier</span><br>						    <span class="cpp-comment">// to read?</span><br>	}<br>	<br>	<span class="cpp-keyword">void</span> Draw() <span class="cpp-keyword">const</span><br>	{<br>	<br>		<span class="cpp-comment">// you can do better here!!</span><br>		<br>		<span class="cpp-keyword">for</span> ( <span class="cpp-keyword">int</span> col = <span class="cpp-number">0</span>; col &lt; <span class="cpp-number">3</span>; ++col ) {<br>			<br>			cout &lt;&lt; <span class="cpp-literal">" - - - \n"</span>;<br>		<br>			<span class="cpp-keyword">for</span> ( <span class="cpp-keyword">int</span> row = <span class="cpp-number">0</span>; row &lt; <span class="cpp-number">3</span>; ++row ) {<br>				<br>				cout &lt;&lt; <span class="cpp-literal">"|"</span>;<br>				CellContents cell = m_boardData[ col + row * <span class="cpp-number">3</span> ];<br>				<br>				<span class="cpp-keyword">switch</span> ( cell ) {<br>				<br>					<span class="cpp-keyword">case</span> PLAYER_X:<br>						cout &lt;&lt; <span class="cpp-literal">"X"</span>;<br>						<span class="cpp-keyword">break</span>;<br>						<br>					<span class="cpp-keyword">case</span> PLAYER_O:<br>						cout &lt;&lt; <span class="cpp-literal">"O"</span>;<br>						<span class="cpp-keyword">break</span>;<br>						<br>					<span class="cpp-keyword">case</span> UNUSED:<br>						cout &lt;&lt; <span class="cpp-literal">" "</span>;<br>						<span class="cpp-keyword">break</span>;<br>				};<br>			}<br>			<br>			cout &lt;&lt; <span class="cpp-literal">"|\n"</span>;<br>		}<br>		<br>		cout &lt;&lt; <span class="cpp-literal">" - - - \n"</span>;<br>	}<br>	<br>	<span class="cpp-comment">// This method lets the player and computer go</span><br>	<span class="cpp-comment">// Returns false if someone won.</span><br>	<span class="cpp-keyword">const</span> <span class="cpp-keyword">bool</span> DoNextRound() <br>	{<br>	<br>		SetCell( GetPlayerMove(), PLAYER_X );<br>		SetCell( GetCPUMove(), PLAYER_O );<br>		<br>		<span class="cpp-comment">// show the updated board</span><br>		Draw();<br>		<br>		<span class="cpp-keyword">return</span> CheckForWin();<br>	}<br>	<br><span class="cpp-keyword">private</span>:<br>	CellContents m_boardData[ <span class="cpp-number">9</span> ]; <span class="cpp-comment">// Users of this class shouldn't ever</span><br>                               <span class="cpp-comment">// directly modify this data</span><br>	<br>	<span class="cpp-keyword">void</span> SetCell( <span class="cpp-keyword">const</span> Coord&amp; Cell, <span class="cpp-keyword">const</span> CellContents Contents ) <br>	{<br>	<br>		m_boardData[ Cell.ToSingleInt() ] = Contents;<br>	}<br>			      <br>	<span class="cpp-comment">// helper function, pretty much self-explanatory		       </span><br>	<span class="cpp-keyword">const</span> <span class="cpp-keyword">bool</span> IsCellEmpty( <span class="cpp-keyword">const</span> Coord&amp; Cell )<br>	{<br>		<br>		CellContents cell = m_boardData[ Cell.ToSingleInt() ];<br>		<br>		<span class="cpp-keyword">if</span> ( cell == UNUSED )<br>			<span class="cpp-keyword">return</span> <span class="cpp-keyword">true</span>;<br>		<br>		<span class="cpp-keyword">return</span> <span class="cpp-keyword">false</span>;<br>	}<br>			       <br>	<span class="cpp-comment">// the external world shouldn't be calling these</span><br>	<span class="cpp-keyword">const</span> Coord GetPlayerMove() <span class="cpp-keyword">const</span><br>	{<br>	<br>		Coord input;<br>		cout &lt;&lt; <span class="cpp-literal">"\nPlayer: choose column: "</span>;<br>		cin &gt;&gt; input.x;<br>		cout &lt;&lt; <span class="cpp-literal">"\nPlayer: choose row: "</span>;<br>		cin &gt;&gt; input.y;<br>		<br>		<span class="cpp-comment">// check for valid input:</span><br>		<span class="cpp-keyword">int</span> singleInt = input.ToSingleInt();<br>		<span class="cpp-keyword">if</span> ( singleInt &lt; <span class="cpp-number">0</span> || singleInt &gt; <span class="cpp-number">8</span> ) {<br>		<br>			cout &lt;&lt; <span class="cpp-literal">"\nInvalid input!\n"</span>;<br>			<br>			<span class="cpp-comment">// use recursive logic to keep going</span><br>			<span class="cpp-comment">// until they get it right</span><br>			<span class="cpp-keyword">return</span> GetPlayerMove();<br>		}<br>		<br>		<span class="cpp-keyword">return</span> input;<br>	}<br>	<br>	<span class="cpp-keyword">const</span> Coord GetCPUMove() <span class="cpp-keyword">const</span><br>	{<br>	<br>		<span class="cpp-comment">// get random coord:</span><br>		<br>		Coord move = Coord( rand() % <span class="cpp-number">3</span>, rand() % <span class="cpp-number">3</span> );<br>		<br>		<span class="cpp-keyword">if</span> ( !IsCellEmpty( move ) )<br>			<span class="cpp-keyword">return</span> GetCPUMove(); <span class="cpp-comment">// more recursive logic</span><br>		<br>		<span class="cpp-keyword">return</span> move;<br>	}<br>	<br>	<span class="cpp-comment">// did someone win?</span><br>	<span class="cpp-keyword">const</span> <span class="cpp-keyword">bool</span> CheckForWin() <span class="cpp-keyword">const</span><br>	{<br>		<br>		<span class="cpp-comment">// i don't feel like implementing a good, board-size independent</span><br>		<span class="cpp-comment">// algorithm here, so just hardcode in the conditions. I didn't do this</span><br>		<span class="cpp-comment">// because I'm lazy, and you already did that in your code :)</span><br>		<br>		<span class="cpp-keyword">return</span> <span class="cpp-keyword">true</span>; <span class="cpp-comment">// just so it compiles</span><br>	}<br>};<br><br><span class="cpp-keyword">void</span> main()<br>{<br><br>	<span class="cpp-comment">// instantiate a board</span><br>	GameBoard board = GameBoard();<br>	<br>	<span class="cpp-keyword">bool</span> playAgain = <span class="cpp-keyword">true</span>;<br>	<br>	<span class="cpp-keyword">while</span> ( playAgain ) {<br>	<br>		<span class="cpp-comment">// show the initial board</span><br>		board.Draw();<br>		<span class="cpp-keyword">while</span> ( board.DoNextRound() );<br>		<br>		cout &lt;&lt; <span class="cpp-literal">"Play again? "</span>;<br>		cin &gt;&gt; playAgain;<br>	}<br>}<br><br></pre></div><!–ENDSCRIPT–> 
Nooo! Classes in a basic tick tack toe game? There are a time and place for classes, but I don't see this as being one. It is better practice to understand procedural programming before bogging yourself down with classes. They tend to be very hard to follow for beginners (and for none-beginners if they are not done well).
Quote:42 or * is the unused spaces
79 is player O
88 is player X


Using literals directly in code is bad practise, use some consts or #defines instead, viz.:

const int PLAYER_1 = 79

This topic is closed to new replies.

Advertisement