Need Advice/Suggestions/Critizism

Started by
14 comments, last by Code Fusion 17 years, 8 months ago
Hello, Today I started trying to create a Tic Tac Toe clone. Well, as it turns out, I acually finished the game. I tried creating the game before (like a onth ago i think...), but it took me 3 days (how pathetic...), and it still didnt work properly. But now, i've been working in my book, learning new things, and I created the game in like 30 min. It might sound like awhile, but too me, thats great. I know Im proud. [lol] Well, to the point. I would like you guys to take a look at my code for the game. Please, give me advice or suggestions on my coding style, something I should of added, took out, changed, or done differently in my code. + Thanks for any help, advice, suggestions. [smile] Source Code:


// Brandon Wall: 7/26/06 - 7/26/06
// Tic Tac Toe Clone


#include <iostream>
#include <vector>
#include <ctime>

using namespace std;

void WelcomeScreen(); // welcomes the player
void GetPmove(vector<char>& gBoard); // gets player move and check if its valid
void Drawboard(vector<char>& gBoard); // draws out the tic tac toe board
void GetCmove(vector<char>& gBoard); // gets computers move and checks if its valid
void CheckWin(vector<char>& gBoard, bool& RDone); // check if player or comp wins

int main()
{
	bool done = false;
	srand(time(0));

	vector<char> Board;
	Board.push_back('0');
	Board.push_back('1');
	Board.push_back('2');
	Board.push_back('3');
	Board.push_back('4');
	Board.push_back('5');
	Board.push_back('6');
	Board.push_back('7');
	Board.push_back('8');

	WelcomeScreen();

	while(!done) {
		Drawboard(Board);
		GetPmove(Board);
		CheckWin(Board, done);
		GetCmove(Board);
		Drawboard(Board);
		CheckWin(Board, done);
	}

	return 0;
}

void GetPmove(vector<char>& gBoard)
{
	int move = 0;
	bool done = false;

	while(!done) {
		cout << "\n\t\t\t\tMove: ";
		cin >> move;

		if(gBoard[move] == 'X') {
			cout << "\n\t\t\t\tYou've already moved there.\n";
			done = false;
		}
		else if(gBoard[move] == 'O') {
			cout << "\n\t\t\t\tYour Opponent has already Moved there.\n";
			done = false;
		}
		else {
			gBoard[move] = 'X';
			done = true;
		}
	}
}

void GetCmove(vector<char>& gBoard)
{
	int CMove = 0;
	bool done = false;

	while(!done) {
		CMove = rand() % 8;

		if(gBoard[CMove] == 'X' || gBoard[CMove] == 'O')
			done = false;
		else {
			gBoard[CMove] = 'O';
			done = true;
		}
	}
}

void Drawboard(vector<char>& gBoard)
{
	system("cls");
	
	cout << "\n\n\n\n\n\t\t\tThe Game Board! Good Luck, Human!\n\n";
	cout << "\t\t\t\t" << gBoard[0] << " | " << gBoard[1] << " | " << gBoard[2] << endl;
	cout << "\t\t\t\t" << "=========" << endl;
	cout << "\t\t\t\t" << gBoard[3] << " | " << gBoard[4] << " | " << gBoard[5] << endl;
	cout << "\t\t\t\t" << "=========" << endl;
	cout << "\t\t\t\t" << gBoard[6] << " | " << gBoard[7] << " | " << gBoard[8] << endl;
}

void CheckWin(vector<char>& gBoard, bool& RDone)
{
	if(gBoard[0] == 'X' && gBoard[1] == 'X' && gBoard[2] == 'X' ||
	   gBoard[0] == 'X' && gBoard[3] == 'X' && gBoard[6] == 'X' ||
	   gBoard[0] == 'X' && gBoard[4] == 'X' && gBoard[8] == 'X' ||
	   gBoard[1] == 'X' && gBoard[4] == 'X' && gBoard[7] == 'X' ||
	   gBoard[2] == 'X' && gBoard[5] == 'X' && gBoard[8] == 'X' ||
	   gBoard[2] == 'X' && gBoard[4] == 'X' && gBoard[6] == 'X' ||
	   gBoard[3] == 'X' && gBoard[4] == 'X' && gBoard[5] == 'X' ||
	   gBoard[6] == 'X' && gBoard[7] == 'X' && gBoard[8] == 'X') {
	   cout << "\n\t\t\t\tOMG! YOU WON!\n\n";
	   RDone = true;
	}

	if(gBoard[0] == 'O' && gBoard[1] == 'O' && gBoard[2] == 'O' ||
	   gBoard[0] == 'O' && gBoard[3] == 'O' && gBoard[6] == 'O' ||
	   gBoard[0] == 'O' && gBoard[4] == 'O' && gBoard[8] == 'O' ||
	   gBoard[1] == 'O' && gBoard[4] == 'O' && gBoard[7] == 'O' ||
	   gBoard[2] == 'O' && gBoard[5] == 'O' && gBoard[8] == 'O' ||
	   gBoard[2] == 'O' && gBoard[4] == 'O' && gBoard[6] == 'O' ||
	   gBoard[3] == 'O' && gBoard[4] == 'O' && gBoard[5] == 'O' ||
	   gBoard[6] == 'O' && gBoard[7] == 'O' && gBoard[8] == 'O') {
	   cout << "\n\t\t\t\tROFL! YOU LOST!\n\n";
	   RDone = true;
	}
}

void WelcomeScreen()
{
	cout << "\n\n\t\t\t  Welcome to Tic Tac Toe!\n"
		 << "\t\t\tProgrammed by: Brandon Wall\n\n"
		 << "\t\t\t\t X | X | O \n"
		 << "\t\t\t\t===========\n"
		 << "\t\t\t\t X | X | O \n"
		 << "\t\t\t\t===========\n"
		 << "\t\t\t\t O |   | O \n\n"
		 << "\t\t\t    Press Enter to Play.";
	cin.get();
}





Advertisement
Convert it to OOP.

Board.push_back('0') // to 9;
could be done with a loop.

CheckWin's if statements could probably be done with a loop.

You need more commenting. [smile]
well right after you win the box closes almost instantly, maby you should make it so the user can press enter to replay the game or the escape key to quit?
also you dont have anything programmed to happen when its a tie game.
one last thing, to improve your AI you might wanna try having the first pick be random but for the second turn picking only one of the numbers next to it.
for example:
X 1 2     X 1 2         X 1 23 4 5 --> 3 4 O  or --> 3 4 56 7 O     6 7 O         6 O O

if the real person is X's and the computer is O's the computers turn would choose either the numbers 5 or 7 for its next turn.
you shouldnt necesarilly use this for your AI but you should probably spice it up a bit by doing something than picking a random space :)

~matt~
The computer should try a little harder to win. It doesn't go for the block if you have two in a row already.

Other than that I think it's swell. :)
Thanx for the suggestions and feedback. Im going to be trying to add the AI, im not sure how, but im ganna give it a go [smile]

Looks good. Well done. I would agree the AI could be improved. I'd just use a brute force approach to begin with:

Is there a square I can win on? If so, place there, else:
Is there a square my opponent can win on next turn? If so, place there, else:
Is there a square that will give me two in a row? If so, place there, else:
Is there a square that will give my opponent two in a row? If so, place there, else:
Is the middle square free? If so, place there, else:
Place at random.

As far as I can figure it, you can't really get much smarter than that at tic-tac-toe.

One thing I would suggest though (from your first post) is to try to lose the hang-up about how long these things take you to do. It has little or no relevance to the quality of your code or you as a programmer. Obviously in the commercial world you need to work to deadlines, but it is not a race [smile].

Paul
Quote:
Is there a square I can win on? If so, place there, else:
Is there a square my opponent can win on next turn? If so, place there, else:
Is there a square that will give me two in a row? If so, place there, else:
Is there a square that will give my opponent two in a row? If so, place there, else:
Is the middle square free? If so, place there, else:
Place at random.

That AI would lose 3 out of 8 games to me, the rest being draws.
If you're passing the board as a parameter to the individual functions, why is the board named like the general practice for a global variable?

As mentioned, converting the game to be a class (with the board as a member variable) would probably make more sense.

Abbreviated function names don't make much sense these days - GetPmove() should be named GetPlayerMove(). Except, not really - GetPlayerMove() implies that the function retrieves a player's move for the caller. DoPlayerTurn() would probably make more sense, as that more closely describes what the player does.

CheckWin() should return a bool, instead of passing a result bool in as a parameter.

The "pick a random move" AI could, in theory, sit in an infinite loop, if the random number that it picked kept coming back as an already taken spot. Although unlikely in practice, you should avoid this possibility at the algorithmic level. One simple way to do this would be to keep a vector of untaken spots, and randomly pick from that. When the player or computer makes a move, you would remove the spot taken from the untaken spots vector.

The input code isn't defensive enough. What happens if the user enters -43 for their move?

In the GetCmove() and GetPmove() while loops, why are you repeatedly assigning false to done? It's already set to false outside the loop.

Oh, and also, the computer will never pick the lower-right spot - if it's the last available spot, the AI will hang. Take a closer look at how the % operator works.

Cheers,
Jason
Quote:Original post by Wiggin
Quote:
Is there a square I can win on? If so, place there, else:
Is there a square my opponent can win on next turn? If so, place there, else:
Is there a square that will give me two in a row? If so, place there, else:
Is there a square that will give my opponent two in a row? If so, place there, else:
Is the middle square free? If so, place there, else:
Place at random.

That AI would lose 3 out of 8 games to me, the rest being draws.


Well, for an "off the top of my head while having a coffee at work" AI, I'm actually quite pleased with those statistics [grin].

More seriously, tic-tac-toe is obviously more complicated than it seems. I, and any other human, can technically force any game to at least a draw as I understand it.

Surely there must be a fairly simple rule-based way of simulating this?

Not tested, but should be more or less OK.

Added functionality:

- Tie games are detected.
- Improved error handling for getting the player's move.

Added organization:

- The board is represented with an actual class, with the appropriate functionality being given to that class. Please pay careful attention to the division of labour. The board state is represented with a plain array, because a vector adds unused space overhead and also fails to communicate the design contraint that the board is always that size. This allows us to encapsulate processes like "making a move", such that outside code does not have to worry about the details and will not corrupt the board state.

- The "demo" board in the welcome screen is drawn in terms of the board class' drawing functionality, so that it will stay consistent with any later rendering changes.

- Some constant text and a utility output function have been extracted.

- Instead of modifying an input bool, the win-detection function now returns an enumeration value. This allows for indicating the draw state. As well, the output messages for wins/losses are removed from this function and put back into main(), where they belong. Please pay careful attention to the restructuring of the main game loop.

- The board "play" function determines if the move can be made, making it as a side effect if possible. Alternatively phrased, it makes the move and returns true if possible, and returns false leaving the board untouched otherwise. This behaviour allows for simplifications to the move-getting routines.

- The drawing and win-detection processes have been refactored to avoid duplication.

#include <iostream>#include <vector>#include <ctime>#include <cstdlib> // for system(). ctime happens to include it on your system for you,// but we shouldn't assume things like that.using namespace std;const int rows = 3;const int cols = rows;const int spaces = rows * cols;const char* horizontal_line = "=========";const char* vertical_line = " | ";class Board {  char b[spaces];  int plays;  public:  enum { player, computer, draw, still_playing } status;    Board() : plays(0) {    for (int i = 0; i < spaces; ++i) {      b = i + '0';    }  }    void draw(bool showNumbers = true);  status checkStatus();  bool play(int where, char who) {    if (b[where] == 'X' || b[where] == 'O') {      return false;    }    // Otherwise, we can play.    plays += 1;    b[where] = who;    return true;  }};void outputIndented(const char* phrase) {  cout << "\n\t\t\t\t" << phrase;}void Board::draw(bool showNumbers) {	system("cls");		cout << "\n\n\n\n\n\t\t\tThe Game Board! Good Luck, Human!\n";  for (int row = 0; row < rows; ++row) {    for (int col = 0; col < rows; ++col) {      char c = b[row * cols + col];      if (!showNumbers && c != 'X' && c != 'O') {        c = ' ';      }      if (col == 0) { outputIndented(c); }      else { cout << vertical_line << c; }      if (row != 2) { outputIndented(horizontal_line); }    }  }  cout << endl;}status Board::checkStatus() {  if (plays == spaces) {    return draw;  }    const int win_types = 8;  const int win_patterns[win_types][rows] = {    {0, 1, 2},    {0, 3, 6},    {0, 4, 8},    {1, 4, 7},    {2, 5, 8},    {2, 4, 6},    {3, 4, 5},    {6, 7, 8}  };    for (int i = 0; i < win_types; ++i) {    int x_count = 0;    int o_count = 0;    for (int j = 0; j < rows; ++j) {      char c = b[win_patterns[j]];      if (c == 'X') { x_count += 1; }      else if (c == 'O') { o_count += 1; }    }    if (x_count == rows) { return player; }    if (o_count == rows) { return computer; }  }  return still_playing;}// Get the player's move.void GetPmove(Board& b){	int move = 0;	while (true) {    outputIndented("Move: ");    bool successful = (cin >> move);    cin.clear();    cin.ignore(std::numeric_limits<streamsize>::max(), '\n');        if (!successful) {      outputIndented("That's not even a number, fool.\n");    } else if (move < 0 || move > 8) {      outputIndented("That's not on the board!\n");    } else if (!b.play(move, 'X')) {      outputIndented("That spot is already taken.\n");    } else {      // the move was successful, so bail out      return;    }	}}// Get the computer's move.void GetCmove(Board& b){	int move;  do { // Iteratively		move = rand() % 8; // generate moves  } while (!b.play(move, 'O')); // until one can be played. :)}void WelcomeScreen(){	cout << "\n\n\t\t\t  Welcome to Tic Tac Toe!\n"		   << "\t\t\tProgrammed by: Brandon Wall\n";  Board demo;  demo.move(0, 'X');  demo.move(1, 'X');  demo.move(2, 'O');  demo.move(3, 'X');  demo.move(4, 'X');  demo.move(5, 'O');  demo.move(6, 'O');  demo.move(8, 'O');  demo.draw(false);  outputIndented("Press Enter to Play.");	cin.get();}int main() {	srand(time(0));	WelcomeScreen();	status s = still_playing;  Board b;    bool player_turn = true;  	while (s == still_playing) {		if (player_turn) {		  b.draw();      GetPmove(b);    } else {      GetCmove(b);  		b.draw();    }		s = b.checkStatus();    player_turn = !player_turn;	}  if (s == player) {    outputIndented("OMG! YOU WON!");  } else if (s == computer) {	  outputIndented("ROFL! YOU LOST!");  } else {    // tie game    outputIndented("WTF? TIE GAME?!");  }  cout << '\n' << endl;}

This topic is closed to new replies.

Advertisement