Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here is my two-player Tic Tac Toe game. How can I improve it?

#include <iostream>
#include <vector>
//Simple Display
void Display(std::vector<char> const &grid){
    //Creating a onscreen grid
    std::cout << "   " << 1 << "  " << 2 << "  " << 3 << "\n";
    for(int a = 0; a < 9; a++){
        if(a == 0)
            std::cout << "A ";
        if(a == 3)
            std::cout << "\nB ";
        if(a == 6)
            std::cout << "\nC ";
        //displaying grid.
        std::cout <<  " " << grid[a] << " ";


    }
    std::cout << "\n\n";
}
//Returns true if the grid is already used.
bool Used(int const& position, std::vector<char> const& grid){
    if(grid[position] == '-')
        return false;
    else
        return true;
}

void Turn(std::vector<char> &grid, char player){
    int row = 0;
    char column = 0;
    int position = 0;
    bool check = true;
    std::cout <<"\n" << player << ": Please play. \n";

    while(check == true){

        std::cout << "Row(1,2,3): ";
        std::cin >> row;
        std::cout << player << ": Column(A,B,C): ";
        std::cin >> column;
        position = 3*(column-'A')+(row-1);
        if(!Used(position,grid)){
            check = false;
        }
        else{
            std::cout << "Already Used. Try Again. \n";
        }
    }
    grid[position] = player;
    std::cout << "\n\n";
}

bool Win(std::vector<char> const& grid, char player){
    for(int a = 0; a < 3; a++){
        if(grid[a] == player && grid[a+3] == player && grid[a+6] == player){
            return true;
        }
        if(grid[3*a] == player && grid[3*a+1] == player && grid[3*a+2] == player){
            return true;
        }
    }
    if(grid[0] == player && grid[4] == player && grid[8] == player){
        return true;
    }
    if(grid[2] == player && grid[4] == player && grid[6] == player){
        return true;
    }
    return false;

}

int main(){
    std::vector<char>grid(9,'-');

    while(true){
        Display(grid);
        Turn(grid, 'X');
        if(Win(grid, 'X')){
            Display(grid);
            std::cout << "\nX is the Winner!";
            break;
        }
        Display(grid);
        Turn(grid,'O');
        if(Win(grid, 'O')){
            Display(grid);
            std::cout << "\nO is the Winner!";
            break;
        }
    }
}
share|improve this question
add comment

2 Answers

I'll just review what you have here, although something like this would work better with classes (you could always attempt this implementation later).

  • Your vector isn't quite grid-like as it's only one-dimensional. It may also be why Display() is a bit confusing, which shouldn't be the case with a 2D structure.

    Either way, you just need an std::array since the board's size is fixed:

    std::array<std::array<char, 3>, 3> grid;
    

    You first need to include <array>, and you can then remove <vector>.

  • In bool Used(), this:

    if(grid[position] == '-')
        return false;
    else
        return true;
    

    can simply become this:

    return !(grid[position] == '-');
    

    The statement already gives a conditional result, so you just have to return it.

    Also, you don't need to pass position by const& as it's a native type. Just pass by value, with const if you want to be safe.

  • Don't just list variables at the start of Turn() (or in general). Declare or initialize them as close to their use as possible.

    For instance, here's what it should be for the user input portion:

    std::cout << "Row(1,2,3): ";
    int row;
    std::cin >> row;
    std::cout << player << ": Column(A,B,C): ";
    char column = 0;
    std::cin >> column;
    

    Side-note: row and column don't need be initialized; they can just be declared.

    Consider having input validation in case the user inputs a valid board spot, otherwise the program will stop working and the game will have to be restarted.

    You could also simplify the function and the input validation by using either letters or numbers for both row and column. Using different ones makes the code and interface more confusing.

  • This:

    while(check == true)
    

    is the same as this but more concise:

    while(check)
    

    For false, use !:

    while(!check)
    
  • The while loop in main() is quite repetitive. You could just have a bool assigned to the winning player, then display that value in the winning (or losing) message after the loop.

    You should also have a way of choosing whether to start with X or O. Normally, someone will choose to go first, and they will choose their own symbol.

share|improve this answer
add comment

One improvement you can make is to only use one set of routines for each player. One simple way to do this is with a char array to represent each player:

int main()
{
    std::vector<char>grid(9,'-');
    char players[] = {'X','O'};
    int player = 1;
    while(true)
    {
        player = abs(player - 1);
        Display(grid);
        Turn(grid, players[player]);
        if(Win(grid, players[player]))
        {
            Display(grid);
            std::cout << "\n" << players[player] << " is the Winner!";
            break;
        }
    }
}

One other thing I noticed, is the logic for the turns isn't accurate. The columns and rows seem to get swapped.

share|improve this answer
    
I would move the player swap to the bottom of the loop and start with 0. It's minor, but reading top down I was first confused why you started with O (1). Also, you can simplify abs(player - 1) with 1 - player. –  David Harkness 4 hours ago
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.