Could Some one look at this Program for me Please?

[Timmy01]

I think everything is in order, but After each player makes a move the board keeps reappering along with putting the X or the O in the slot selected. Does anyone know how to fix this and also if anyone see's a way to make this better than I have done that would be cool too. Thanks.

#include <iostream>
using namespace::std;
#include <stdlib.h>


void Game_Board();
bool checkWin();
void move(bool);
bool isLegal(int);
char spot[9] = {'1','2','3','4','5','6','7','8','9'};


void main()
{
    bool player = false; // true = X false = O
    
	Game_Board();
    
	while(!checkWin())
	{
        if(player == true)
			player = false;
        else
        	player = true;
        	move(player);
    }
        	
		if(player == true)
        		cout << "Player 1 WINS" << endl;
        
		else
        		cout << "Player 2 WINS" << endl;

}
 
void Game_Board()
{
	
        	
	cout << "\n " << spot[0] << " | " << spot[1] << " | " << spot[2] << endl
         << " ---------" << endl
         << " " << spot[3] << " | " << spot[4] << " | " << spot[5] << endl
         << " ---------" << endl
         << " " << spot[6] << " | " << spot[7] << " | " << spot[8] << endl;
}


void move(bool who)
{
	int potition;
    
	if(who == true)
		cout << "\nSelect number to place your X in Player 1, and press enter: ";
    else
        cout << "\nSelect number to place your O in Player 2, and press enter: ";
        cin >> potition;
        	
	if(isLegal(potition))
	{
            		
		if(who == true)
            spot[potition-1] = 'X';
            		
		else
            spot[potition-1] = 'O';
    }
            	
		else
            move(who);
            Game_Board();
}
        
bool isLegal(int potition)
{
            	
	if(spot[potition-1] == 'X' || spot[potition-1] == 'O')
		return false;
    
	else
        return true;
}

bool checkWin()
{
        	
	if(spot[0] == spot[1] && spot[2] == spot[0] )
        return true;
    
	else if(spot[3] == spot[4] && spot[5] == spot[3])
        return true;
    
	else if(spot[6] == spot[7] && spot[8] == spot[6])
        return true;
    
	else if(spot[0] == spot[3] && spot[6] == spot[0])
        return true;
    
	else if(spot[1] == spot[4] && spot[7] == spot[1])
        return true;
    
	else if(spot[2] == spot[5] && spot[8] == spot[2])
        return true;
    
	else if(spot[0] == spot[4] && spot[8] == spot[0])
		return true;
    
	else if(spot[2] == spot[4] && spot[6] == spot[2])
        return true;
    
	else
        return false;
}

[Daved]

You call Game_Board() at the end of the move function, which I assume would be fine. The only potential problem I see is if IsLegal return false, then the gameboard would be printed twice because you call move recursively.

To fix it, don't call move recursively, use a loop to loop while the move is not legal.

[Timmy01]

Thanks Daved but I went another route. Can you see what I did? Thanks for the help though I appretiate the response. Thanks.

#include <iostream>
using namespace::std;
#include <stdlib.h>


void Game_Board();
bool checkWin();
void move(bool);
bool isLegal(int);
char spot[9] = {'1','2','3','4','5','6','7','8','9'};


void main()
{
    bool player = false; // true = X false = O
    
	Game_Board();
    
	while(!checkWin())
	{
        if(player == true)
			player = false;
        else
        	player = true;
        	move(player);
    }
        	
		if(player == true)
        		cout << "Player 1 WINS" << endl;
        
		else
        		cout << "Player 2 WINS" << endl;

}
 
void Game_Board()
{
	
	system ("cls");
        	
	cout << "\n " << spot[0] << " | " << spot[1] << " | " << spot[2] << endl
         << " ---------" << endl
         << " " << spot[3] << " | " << spot[4] << " | " << spot[5] << endl
         << " ---------" << endl
         << " " << spot[6] << " | " << spot[7] << " | " << spot[8] << endl;
}


void move(bool who)
{
	int potition;
    
	if(who == true)
		cout << "\nSelect number to place your X in Player 1, and press enter: ";
    else
        cout << "\nSelect number to place your O in Player 2, and press enter: ";
        cin >> potition;
        	
	if(isLegal(potition))
	{
            		
		if(who == true)
            spot[potition-1] = 'X';
            		
		else
            spot[potition-1] = 'O';
    }
            	
		else
            move(who);
            Game_Board();
}
        
bool isLegal(int potition)
{
            	
	if(spot[potition-1] == 'X' || spot[potition-1] == 'O')
		return false;
    
	else
        return true;
}

bool checkWin()
{
        	
	if(spot[0] == spot[1] && spot[2] == spot[0] )
        return true;
    
	else if(spot[3] == spot[4] && spot[5] == spot[3])
        return true;
    
	else if(spot[6] == spot[7] && spot[8] == spot[6])
        return true;
    
	else if(spot[0] == spot[3] && spot[6] == spot[0])
        return true;
    
	else if(spot[1] == spot[4] && spot[7] == spot[1])
        return true;
    
	else if(spot[2] == spot[5] && spot[8] == spot[2])
        return true;
    
	else if(spot[0] == spot[4] && spot[8] == spot[0])
		return true;
    
	else if(spot[2] == spot[4] && spot[6] == spot[2])
        return true;
    
	else
        return false;
}

But if ya have any other recomendations that would be cool. However, I could use some advice on if there are no winners how to effectively code that. Also with a cout << statement would be cool. Some example code would be helpful.

[anon]

However, I could use some advice on if there are no winners how to effectively code that.

There are nine moves in the game. If these are up and no winner then it's a draw.

        if(player == true)
			player = false;
        else
        	player = true;

This could be written as

player = !player;

(Not true = false, not false = true.)

However, you might also do something like:

char symbols[2] = {'X', 'O'};
int player = 0;

To toggle between the values 1 and 0, you can use XOR like this:

player ^= 1;

(1 XOR 1 = 0, 0 XOR 1 = 1.)

If you go with this scheme you can display the winner like this (but you might also have an array of two player-names):

 
cout << "Player "
//start counting from 1, not zero
<< player + 1 
<< " WINS" << endl;

(This would get rid of all the if-else's based on who's the player. You do the same thing for both, only small pieces of data are different, like player ID and the symbol to draw.)

And to be correct C++, you should include <cstdlib> instead of <stdlib.h> and change the return type of main to int. You might also make move input more error-proof. It can't recover if I don't input a number at all.

I'm not too big of a fan of using recursion like you do. Technically, if I keep entering the same incorrect input over and over again, if I have enough patience this program will run out of stack space and crash. If you want to do the exact same thing again, use a loop instead of recursion. Recursion is like more for cases where you break down a big problem into smaller, but similar problems.

[foxman]

I might be a little bit off topic, but I saw this

using namespace::std;

in your code and felt a bit surprised. I mean, there's no namespace named "namespace" in your code (as far as I can see), and in that namespace, there isn't a "std" name. I bet what you wanted to write was

using namespace std;

[Timmy01]

Hey guys thanks alot you were all most helpful and your recomendations were most helpful to. I do see my errors now. I did have a couple of typo's as well. Thanks guys and/or gals?

You never know so I had to throw the dog a bone. (lol)

[Daved]

>> Thanks Daved but I went another route. Can you see what I did?
I only looked quickly, but I don't see what you did. The issue I mentioned before looks like it is still there. If the user picks a position that is already taken, it will print out the game board an extra time. If the user picks a bad position many times in a row it will crash the program because you are using recursion where a simple loop would do (although I'm not sure that a crash is really that likely).

It's a good programming exercise to change your move function to not be recursive, and it's the right thing to do for your program, too.

[Timmy01]

Well to stop the reprint of the board I used

system ("cls");

in the " The_Game_Board () function.

As far as the other problems you are talking about, I guess I am having a little trouble following. If you wouldn't mind terribly, could you quote my code and show me an example of what you mean with your own code or in your own words. I'm sorry I am fairly new to this language and I understand better when I can actually see the code versus a spoken or written explaination. Thanks again.

[Daved]

void move(bool who)
{
    int potition;

    if(who == true)
        cout << "\nSelect number to place your X in Player 1, and press enter: ";
    else
        cout << "\nSelect number to place your O in Player 2, and press enter: ";
    cin >> potition;

    if(isLegal(potition))
    {
        if(who == true)
            spot[potition-1] = 'X';
        else
            spot[potition-1] = 'O';
    }
    else
        move(who);

    // <--- After move(who) finishes the control goes here.

    Game_Board();
}

Mixing tabs and spaces makes it hard to read the indentation when posted on this board, so I fixed the indentation to make it clear what is happening. The situation I'm talking about is when isLegal(potition) returns false because the position is not legal. In that case, the red code will be executed, but the blue code will not.

So the first time move is called, the call stack looks like this (with the first function on the bottom):

move
main

What happens when move(who) in red is called? It is another function call. All the parameter information is added to the stack, another local variable named potition is created (that is different from the potition in the previous call) and the function is run again:

move
move
main

What if the player inputs an illegal position again, then it will call the red code again and the call stack will look like this:

move
move
move
main

As you can see, each time an illegal position is given, the stack is added to. Eventually you might run out of stack space when you program like this, especially when you don't know how many times you might end up calling move. A better solution is to use a loop instead of calling move again. You already know how to do this. You do it inside main with your while(!checkWin()) loop.

As to the gameboard appearing multiple times, let's say in the above example that the user finally gives a legal position. Then the code in blue is run and the code in red is skipped. Then Game_Board() is called and the game board is displayed. After that the function ends. But it is just the top function on the stack, which now looks like this:

move
move
main

The top function has ended, but the next function is still running, and it continues in its own code at the <--- part. That means that Game_Board() is called again before that function ends. Control goes back to the first move function, which calls Game_Board() a third time before returning control back to main(). That's why the game board is drawn several times on illegal input.

Using system("cls") to clear the screen before showing the board might not be a bad idea, but it doesn't fix the source of the problem, which is the bug in your code I just described. It's probably better to find a way to fix that bug than to rely on the fact that the clearing of the screen hid it.

Hope that makes more sense.