TicTacToe game attempt review

Started by
16 comments, last by MarkS_ 7 years, 1 month ago

"using namespace" is mostly similar to Java-ish


static import std.* ;

Didn't touch PHP long enough to know it had imports at all :)

There was one other thing I noticed afterwards, that I should mention here, you wrote


void myfunction(std::string s) { ... }

In C++, a variable is storage of the entire object (unlike eg Java where it is just a reference to the object). When you call 'myfunction', the 'string s' parameter says, "make a copy of the string at the stack". In nearly all cases, you don't want to write in such objects (since they get destroyed when you walk out the function anyway). It is generally cheaper in that case to write


void myfunction(const std::string &s) { ... }

which puts a read-only reference to your argument onto the stack, which is often smaller, and doesn't involve copying objects.

Advertisement

"using namespace" is mostly similar to Java-ish


static import std.* ;

Didn't touch PHP long enough to know it had imports at all :)

There was one other thing I noticed afterwards, that I should mention here, you wrote


void myfunction(std::string s) { ... }

In C++, a variable is storage of the entire object (unlike eg Java where it is just a reference to the object). When you call 'myfunction', the 'string s' parameter says, "make a copy of the string at the stack". In nearly all cases, you don't want to write in such objects (since they get destroyed when you walk out the function anyway). It is generally cheaper in that case to write


void myfunction(const std::string &s) { ... }

which puts a read-only reference to your argument onto the stack, which is often smaller, and doesn't involve copying objects.

Relevant for std::string.

Some things I noticed when I compiled that haven't been addressed:

1.) There is no error checking for player input. When I was asked to choose the board size, I naively entered "3,3", because I wanted a 3x3 board. That crashed the program.
2.) Related to #1, there are no instructions as to what is a valid input. I had to go back to the code on several occasions to figure out what I was allowed to enter. I got ahead of myself and entered '3' for a symbol and expected a crash. Typically the two symbols are 'X' and 'O'.
3.) Choosing 'X' forced the CPU to choose '#'. ??
4.) When I was asked to "Choose X:", I had to stop and go back to the code. I DID choose 'X'. A better instruction would be "Choose Column:".

Now, relating directly to the code. I had to make a change in the main function.

Arrays must be defined via constants. This will not compile:
    Player players[numberOfPlayers]; //<- numberOfPlayers is not a constant.
Instead, use a dynamic array:
    Player *players = new Player[numberOfPlayers];

    // Add before exiting. DON'T forget!
    delete [] players;
Or better yet, use a std::vector, but your code is going to need a reformat. Worth it though!

Also, Board.cpp gave me a compile error at "Board::print()". Mark Kughler touched on this.

This line will not compile unless you add "#include <string>;" in the source file.
cout << "\n  " << string(size * 5, '-') << "\n"; //<- std::cout doesn't have the correct conversion needed to display a std::string if the string header is not included.
Other than that and what others have stated, good work! It is a very good start!
Yes, definitely use std::vector. I wouldn't advise a beginner to use new and delete for pretty much anything. Replacing an array with a vector is usually as easy as this:

    std::vector<Player> players(numberOfPlayers);

Yes, definitely use std::vector. I wouldn't advise a beginner to use new and delete for pretty much anything. Replacing an array with a vector is usually as easy as this:

    std::vector<Player> players(numberOfPlayers);


Yeah, I don't know what I was thinking. It took less than a minute to rewrite it to use vectors.
I rewrote main.cpp to show what we've been talking about. My goal wasn't to do the work for you, but to show you how it could be done. Feel free to change this as you see fit.


#include <iostream> 
#include <string>
#include <vector>
#include <stdlib.h>
#include "Player.h"
//#include "AutoPlayer.h"
#include "Board.h"
#include "Game.h"

// using namespace std; // Don't do this.

void dumpPlayers(Player * p, int n)
{	
	for (int i = 0; i < n; i++) {
		std::cout << p[i].getName() << " : " << p[i].getSymbol() << std::endl;
	}
}

void PlayGame()
{
	string		currentPlayerName;
	char		currentPlayerSymbol;
	int			numberOfPlayers;
	int			boardSize;
	bool		needBot = false;

	/**
	* Get the number of players.
	*/

	std::cout << "\t=== Tic Tac Toe ===" << std::endl << std::endl;
	std::cout << "Enter the number of players: ";
	std::cin >> numberOfPlayers;
	std::cout << std::endl;

	if (numberOfPlayers == 1)
	{
		numberOfPlayers++;
		needBot = true;
	}

	/**
	* Array container for all the players.
	*/
	std::vector<Player>		players(numberOfPlayers);

	if (needBot == true)
	{
		std::cout << "Hi, player! What's your name? ";
		std::cin >> currentPlayerName;

		std::cout << "Well, hello " << currentPlayerName << '!' << std::endl << "What would you like your symbol to be (X or O)? ";
		//Traditionally two player Tic Tac Toe only uses X and O as symbols.
		while (true)
		{ // Only allow 'X' or 'O' for two player mode.
			std::cin >> currentPlayerSymbol;
			currentPlayerSymbol = toupper(currentPlayerSymbol); // Convert to uppercase. Not necessary, but it does simplify the proceeding test cases.
			if (currentPlayerSymbol == 'X' || currentPlayerSymbol == 'O')
			{
				break;
			}else {
				std::cout << "Sorry, symbol must be X or O for single player mode." << std::endl;
			}
		}

		std::cout << std::endl;

		players[0].setName(currentPlayerName);
		players[0].setSymbol(currentPlayerSymbol);

		players[1].setName("CPU");

		if (currentPlayerSymbol == 'X')
			players[1].setSymbol('O');
		else if (currentPlayerSymbol == 'O')
			players[1].setSymbol('X');

		players[1].makeBot();
	}else if (needBot == false && numberOfPlayers == 2) {
		std::cout << "Hi, player 1! What's your name? ";
		std::cin >> currentPlayerName;

		std::cout << "Well, hello " << currentPlayerName << '!' << std::endl << "What would you like your symbol to be (X or O)? ";

		//Traditionally two player Tic Tac Toe only uses X and O as symbols.
		while (true)
		{ // Only allow 'X' or 'O' for two player mode.
			std::cin >> currentPlayerSymbol;
			currentPlayerSymbol = toupper(currentPlayerSymbol); // Convert to uppercase. Not necessary, but it does simplify the proceeding test cases.
			if (currentPlayerSymbol == 'X' || currentPlayerSymbol == 'O')
			{
				break;
			}
			else {
				std::cout << "Sorry, symbol must be X or O for two player mode." << std::endl;
			}
		}

		std::cout << std::endl;

		players[0].setName(currentPlayerName);
		players[0].setSymbol(currentPlayerSymbol);

		std::cout << "Hi, player 2! What's your name? ";
		std::cin >> currentPlayerName;

		players[1].setName(currentPlayerName);

		if (currentPlayerSymbol == 'X')
			players[1].setSymbol('O');
		else if (currentPlayerSymbol == 'O')
			players[1].setSymbol('X');

		std::cout << "Well, hello " << currentPlayerName << '!' << std::endl << "Your symbol will be " << players[1].getSymbol() << std::endl << std::endl;
	}else {
		for (int i = 0; i < numberOfPlayers; i++) {
			std::cout << "Hi, player " << i + 1 << "! What's your name? ";
			std::cin >> currentPlayerName;

			std::cout << "Well, hello " << currentPlayerName << '!' << std::endl << "What would you like your symbol to be? ";

			std::cin >> currentPlayerSymbol;

			std::cout << std::endl;

			players[i].setName(currentPlayerName);
			players[i].setSymbol(currentPlayerSymbol);
		}
	}

	/**
	* Generate the board.
	*/

	std::cout << "Enter the board size (3 will give a 3x3 board, 5 will give a 5x5 board, etc.): ";
	std::cin >> boardSize;

	Board	board(boardSize);
	Game	game(&board);

	game.start();
	board.print();
	int currentPlayer = 0;
	while (game.isRunning()) {
		game.setCurrentPlayer(&players[currentPlayer]);

		if (game.getCurrentPlayer()->isBot()) {
			std::cout << game.getCurrentPlayer()->getName() << ": ";
			game.getCurrentPlayer()->analyze(&board);
			game.getInputFromBot(game.getCurrentPlayer());
		}else {
			game.getInput();
		}
		/**
		* Handle players array overflow.
		*
		* When the current player is the last player
		* in the array, we go back to first player
		* from the array.
		*/
		if (currentPlayer == (numberOfPlayers - 1)) {
			currentPlayer = 0;
		}else {
			currentPlayer++;
		}

		// Insert the current player's symbol in the requested cell.
		game.setCell();

		// Print the last version of the board.
		board.print();

		// Analyze the board for winning situations.
		game.analyze();

		if (game.isWon()) {
			game.stop();
			std::cout << game.getWinner() << " has won!" << std::endl << "Congratulations!" << std::endl << std::endl;
		}else if (game.isDraw()) {
			game.stop();
			std::cout << "Draw!" << std::endl << std::endl;
		}
	}
}

int main()
{
	char	exit;

	do {
		PlayGame();

		std::cout << "Would you like to play again (Y or N)? ";
		std::cin >> exit;
		if(tolower(exit) == 'n')
			break;
		system("cls"); // Clear console.
	}while(true);

	return 0;
}
Also, this bears repeating:

void Game::getInput()
{
    cout << this->currentPlayer->getName() << ", choose column: "; // MUCH more clear than 'X' and 'Y'.
    cin >> currentX;
    cout << this->currentPlayer->getName() << ", choose row: ";
    cin >> currentY;
	
	/**
	 * Subtract one from the values that the user has entered.
	 * This is necessary because these values are used to 
	 * search in the array. For example, when the user 
	 * enters value 1, this will refer to the first
	 * element in the array, which is at index 0.
	 */
	currentX--;
	currentY--;
}
@Alberth & @Kercyn
Still this stuff in C++ confuses me a bit. Like, I know that when you need to pass a value to read it only, you need to put const keyword in front of the parameter, and pass it by reference (or pointer). However, I'm not sure when to use pass by reference or pass by pointer. As I've understood:
- pass by value means copying the object in the function's scope
- pass by reference means passing that specific object into the function
- pass by pointer means passing only the address of the object
@MarkS
Surprisingly, it allowed me to initialize the array without a constant expression. Not sure why and also not sure why you'd need a constant when you allocate the array on the stack.
As for the different syntax of the array, correct me if I'm wrong:
- Player players[numOfPlayers] would allocate the memory on the stack (or in the context where the array is declared - in this case the stack, because it is declared in function).
- Player * players = new Player[numOfPlayers] would allocate memory on the heap while keeping a reference on the stack.
- delete [] players would free the heap memory allocated for the array, but the players reference would still remain. Therefore, a players = nullptr would be good practice after the delete. Am I correct?
Thanks for the tips, guys. This game is a way for me to learn to program in C++ and to do games development. I skipped the basic checks and the flow of the game lacks stuff because I wanted to concentrate on things that I found confusing. Things like making the game use a bot when there's only one player, doing some artificial intelligence, although my bot is reaaaaaaaly dumb now. I am still not really sure how should I go about making it smarter.
I am thinking of making my bot look for situations where the other player could win. For example, when the bot finds two X's in row, he should put his mark on the cell that's empty on that row. Some tips on AI would be appreciated (everything I find on the web now is too confusing... minimax algorithm? It seems to complex for me, for now).
Also, one other thing that I'd like to do is provide a GUI for the game. I kind of learn by doing. :)

@MarkS
Surprisingly, it allowed me to initialize the array without a constant expression. Not sure why and also not sure why you'd need a constant when you allocate the array on the stack.
As for the different syntax of the array, correct me if I'm wrong:
- Player players[numOfPlayers] would allocate the memory on the stack (or in the context where the array is declared - in this case the stack, because it is declared in function).


Someone smarter than me will have to explain this for you. I've never fully understood why this is either and I honestly cannot remember anyone ever even attempting to answer it. It's one of those things that I've always just nodded my head and accepted without truly knowing why. Even when I tried to come up with a logical answer, I kept finding myself contradicting myself. Why it worked for you? I have no idea. Maybe a bug in your compiler? But if it did work, that shows that it can work, so the confusion as to why it isn't allowed mounts...

- Player * players = new Player[numOfPlayers] would allocate memory on the heap while keeping a reference on the stack.
- delete [] players would free the heap memory allocated for the array, but the players reference would still remain. Therefore, a players = nullptr would be good practice after the delete. Am I correct?


This is precisely why Álvaro refuted me for even bringing it up. This is a dangerous path to take if you do not know what you're doing and I shouldn't have mentioned it. You can cause weird and mysterious bugs when using new/delete unless you are 100% sure what you're doing. To answer your questions, no, new doesn't place anything on the stack (to the best of my knowledge) and once delete is called, the memory is freed and the pointers now point to garbage. Setting the pointer(s) to nullptr is redundant.

This topic is closed to new replies.

Advertisement