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.

I'm fairly new to C++ and to game programming itself. Today I decided to build a multiplayer Tic Tac Toe program using only elemental C++ syntax/data structures and I also implement a text interface for the game instead of real graphics.

I paste the source code below. Any suggestions for the code or tips?

#include <iostream>
#include <string>

#define GRID_SIZE 9

using namespace std;

void printGame(char gameGrid[]);
void inputToSlot(char gameGrid[], int slot, char choice);
void initialiseGame(char gameGrid[]);
bool isFull(char gameGrid[]);
bool isEmpty(char input);
bool checkWon(char gameGrid[]);
bool rowWon(char gameGrid[]);
bool columnWon(char gameGrid[]);
bool diagonalWon(char gameGrid[]);

int main(int argc, const char * argv[])
{
    char gameGrid[GRID_SIZE];
    initialiseGame(gameGrid);
    int currentTurn = 0;

    cout << "Tic Tac Toe by Bryan Lean" << endl << endl;
    printGame(gameGrid);
    cout << endl;

    while (!isFull(gameGrid)) {
        int slotChoice;
        currentTurn += 1;
        bool isP2 (currentTurn % 2 == 0);

        // Prompt
        if (isP2) {
            cout << "Player 2 turn..." << endl << "Insert to slot > ";
        } else {
            cout << "Player 1 turn..." << endl << "Insert to slot > ";
        }
        cin >> slotChoice;

        while (!isEmpty(gameGrid[slotChoice])) {
            cout << "Slot occupied. Please select another slot > ";
            cin >> slotChoice;
        }

        // Insert
        if (isP2) {
            gameGrid[slotChoice] = 'o';
        } else {
            gameGrid[slotChoice] = 'x';
        }

        // Print
        printGame(gameGrid);
        cout << endl;

        // Check Winner
        if (checkWon(gameGrid)) {
            if (isP2) {
                cout << "Player 2 won the game.";
            } else {
                cout << "Player 1 won the game.";
            }

            break;
        } 

    }

    if (!checkWon(gameGrid)) {
        cout << "Draw.";
    }


}

void initialiseGame(char gameGrid[]) {
    for (int i = 0; i < GRID_SIZE; ++i) {
        gameGrid[i] = ' ';
    }
}

void printGame(char gameGrid[]) {
    cout << "+---+---+---+" << endl;
    for (int i = 0; i < GRID_SIZE; ++i) {
        cout << "| " << gameGrid[i] << " ";
        if ((i+1) % 3 == 0 && i != 0) {
            cout << "|" << endl;
            cout << "+---+---+---+" << endl;
        }
    }
}

void inputToSlot(char gameGrid[], int slot, char choice) {
    gameGrid[slot] = choice;

}

bool isFull(char gameGrid[]) {
    for (int i = 0; i < GRID_SIZE; ++i) {
        if (gameGrid[i] == ' ') {
            return false;
        }
    }

    return true;
}

bool isEmpty(char input) {
    if (input == ' '){
        return true;
    }
    return false;
}

bool checkWon(char gameGrid[]) {
    if (rowWon(gameGrid)) {
        return true;
    } else if (columnWon(gameGrid)) {
        return true;
    } else if (diagonalWon(gameGrid)) {
        return true;
    }

    return false;
}

bool rowWon(char gameGrid[]) {
    for (int i = 0; i < GRID_SIZE; i += 3) {
        char firstInRow = gameGrid[i];
        char secondInRow = gameGrid[i + 1];
        char thirdInRow = gameGrid[i + 2];

        if (!isEmpty(firstInRow) && !isEmpty(secondInRow) && !isEmpty(thirdInRow)) {
            if (firstInRow == secondInRow && firstInRow == thirdInRow) {
                return true;
            }
        }
    }

    return false;
}

bool columnWon(char gameGrid[]) {
    for (int i = 0; i < 3; ++i) {
        char firstInColumn = gameGrid[i];
        char secondInColumn = gameGrid[i + 3];
        char thirdInColumn = gameGrid[i + 6];

        if (!isEmpty(firstInColumn) && !isEmpty(secondInColumn) && !isEmpty(thirdInColumn)) {
            if (firstInColumn == secondInColumn && firstInColumn == thirdInColumn) {
                return true;
            }
        }

    }

    return false;
}

bool diagonalWon(char gameGrid[]) {
    char center = gameGrid[4];
    if (!isEmpty(center) && !isEmpty(gameGrid[0]) && !isEmpty(gameGrid[8])) {
        if (center == gameGrid[0] && center == gameGrid[8]) {
            return true;
        }
    } else if (!isEmpty(center) && !isEmpty(gameGrid[2]) && !isEmpty(gameGrid[6])) {
        if (center == gameGrid[2] && center == gameGrid[6]) {
            return true;
        }
    }

    return false;

}


/************************

 Tic Tac Toe
Text Graphics

 +---+---+---+
 | x | o | x |
 +---+---+---+
 | x | o | x |
 +---+---+---+
 | o | o | x |
 +---+---+---+

*************************/
share|improve this question
1  
if (isP2) { cout << "Player 2 turn..." << endl << "Insert to slot > "; } else { cout << "Player 1 turn..." << endl << "Insert to slot > "; } can be replaced with ternary, cout << "Player " << isP2 ? "2" : "1" << " turn\nInsert to slot > "; same with checkWon etc –  vaxquis May 30 at 13:30
    
Oh yeah, totally! C++ can be so beautiful, man. I need to practise more C++ exclusive syntax... –  leansie May 30 at 17:02
    
ternary operator is not a C++-exclusive syntax per se... it's used in most C-like languages nowadays, Java, JS & PHP included. –  vaxquis May 30 at 17:31
    
Ahh~ I started programming using high level language (python), there's a lot of things I have yet to know. –  leansie May 30 at 19:04
    
Well, strictly speaking, all of the languages I enumerated are considered pure high-level languages, since JS & PHP are scripting languages, Java uses VM and C++ is considered high level language due to OOP paradigm, lambdas etc, as opposed to mid/low-level pure C. Furthermore, most code sweetening & syntactic sugar (ternary, Elvis & null-safe operators, lambda syntax, throw-try-catch and so on) you'll encounter will be specific to high-level language domains. Yup, I'm mixing apples with oranges, but note that all of the features are basically replaceable with equivalent low-level code. –  vaxquis May 30 at 19:38
add comment

4 Answers 4

Overall, I think it is a pretty good program.

I think I see one bug. You don't check that the user's input for slotChoice is valid. If the input is outside the range 0..8, the program overruns the array.

Some style and refactoring suggestions, in no particular order...

  • I don't like the global GRID_SIZE constant. I would make a struct (or a class) that holds gameGrid and the size. Then pass a reference to the struct (a const reference when possible) to the functions that currently have a gameGrid parameter.

  • Later, if you want to make the program more object-oriented, you can make most of the current functions into member functions of the class. Since you are new to C++, I would treat that as a separate project.

  • Consider putting currentTurn in the struct also.

  • I think getting the slotChoice input and validating it is a good bit of work to move to a new function. I would move this:

    cin >> slotChoice;
    
    while (!isEmpty(gameGrid[slotChoice])) {
        cout << "Slot occupied. Please select another slot > ";
        cin >> slotChoice;
    }
    

    plus new code to check that the input is in range, to a new function.

  • (Small point) I like to read things in their natural order. I'd rather see the "stuff" for player 1 come before the "stuff" for player 2. So I'd use a bool named isP1 and rearrange several conditional statements.

  • In the "Check Winner" section of the while loop, you could save the return value from checkWon() in a local bool variable. Then you wouldn't need to call the function again to decide if the game was a draw.

  • I see some code duplication, especially in the code that generates output.

    cout << "Player 2 won the game."; appears again, with only 1 character changed.

    cout << "+---+---+---+" << endl; appears twice, and could go in a separate function.

  • An alternative for checkWon():

    bool checkWon(const char gameGrid[]) {
       if (rowWon(gameGrid)) {
           return true;
       }
       if (columnWon(gameGrid)) {
           return true;
       }
       if (diagonalWon(gameGrid)) {
           return true;
       }
    
       return false;
    }
    

    I made the parameter const. I changed the cascading else if statements to three independent if statements. I think that reflects the underlying logic a bit better; the three win conditions aren't mutually exclusive.

  • You could simplify the logic in function rowWon(). If you check isEmpty(firstInRow), you don't need to also check isEmpty(secondInRow) and isEmpty(thirdInRow). (The later comparisons with firstInRow will exclude cases where the other cells in the row are empty.) columnWon() and diagonalWon() could get the same simplification.

  • There's a lot of code duplication (and conceptual duplication) between RowWon(), columnWon(), and diagonalWon(). At the core of each is a complicated if statement that examines 3 cells and returns a bool result. But all of those if statements are essentially the same. That decision could go in a a separate function -- pass it the 3 cell values. The other part of each function involves deciding which 3 cells to test; those decisions are different for each win condition.

share|improve this answer
1  
Thank you so much for the insight! Oh yeah, it definitely need an input guardian for slotChoice, I totally forgot. Some duplication need to be factor out especially for the isEmpty(). I also want to code this using structs and references after I reach that part of my C++ learning (I started programming using Python). And really, thanks for the advice!! –  leansie May 29 at 13:48
    
-1 saying the code is good because it's good for somebody who is a beginner is a not sequitur IMO. either the OP code is good (which it isn't since it basically has 10s of things to improve), or it isn't. btw, calling an objectless C++ code which can be obviously solved in an object domain a "pretty good program" is a severe abuse of words IMO. –  vaxquis May 30 at 13:34
    
as stated in another answer, "[the code] defines several functions that all refer to the same gameGrid array. This strongly suggests that you could (and should!) use a class instead and make those functions member functions of the class. - emphasis mine; saying "Later, if you want to make the program more object-oriented, (...) Since you are new to C++, I would treat that as a separate project." is essentially promoting a structural approach to an OOP problem. Being a beginner is no excuse to write sloppy code. –  vaxquis May 30 at 13:38
    
@vaxquis: I called it a "pretty good program" because it does a lot of things well. I didn't think it worthwhile to list them, but if I had, there would have been more than the list of improvements I suggested. This is NOT what I would call "sloppy code" (and I've seen lots of that). It is essentially C code with cin and cout, not full-fledged C++. A very object-oriented approach might be "better", but perhaps not for a programmer at this experience level. The TicTacToe class in Edward's answer shows a useful evolution, but it might be too big a leap for someone at the OP's level. –  GraniteRobert May 30 at 14:46
    
if you think this code is "pretty good", and you really seen "lots of [worse] code", I can only express my utter pity for the people you have to work with. I've used OOP since I was 14, and I actually learned C++ before raw struct & memory alloc C, Pascal and Java... I wouldn't consider learning OOP basics above anyone's "experience level", unless he's < 15 years old. –  vaxquis May 30 at 17:30
show 1 more comment

Here are some things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use const variables instead of #define

The use of #define to define constants in C++ is generally better avoided. Instead, for example, your code could have used const size_t GRID_SIZE = 9;

Avoid spurious use of std::endl

The code uses std::endl in a number of places where it's not useful to do so. Remember that std::endl inserts a newline character and flushes the stream. If you don't really need to flush the stream, which is the case for nearly every instance in this program, you should simply use the \n character instead.

Use constants consistently

Your code creates a constant GRID_SIZE, which suggests that it might be possible to change it to some other value, but then the printGrid() function has a hardcoded 3 which make that not really work. With just a bit more effort, your code could be modified to accept any size square, or even a rectangular shape if you were interested in that.

Reconsider the definition of isEmpty()

Right now your code has a whole function isEmpty which is actually identical in function to input==' ' which is rather slim for a function. Further, every instance it's used, it's actually passed a copy of the contents of one of the squares in gameGrid. What would make more sense is to pass in the index value into isEmpty and avoid making all of those copies.

Reconsider the definition of checkWon()

Since you already know, based on isP2, which player might just have won, it would make your code simpler if you would pass that value to checkWon() so that it could avoid checking for empty squares. All the code really needs to look for in that instance is either an o or an x value.

Use objects

Your code defines several functions that all refer to the same gameGrid array. This strongly suggests that you could (and should!) use a class instead and make those functions member functions of the class.

Hide arbitrary implementation choices

If you use an object, the specific characters used for x, o and empty should all be private members of the class. That way, you could arbitrarily change to some other value and calling code would need not know of the change.

Scrub input data

Right now, you use an inserter to get the value of slotChoice but if the user inputs a letter, the code will be stuck in an infinite loop. Also, you really must add bounds checking to disallow the user putting in some value such as 209581 and altering memory well outside the program space.

update ...

I refactored your code into a C++ class:

class TicTacToe 
{
private:
    // stores the actual square game grid contents
    char *gameGrid;
    // width and total number of squares in the gameGrid
    size_t width, squares; 
    // horizontal bar to print between rows
    std::string hbar;
    // which player's turn is it? (0 or 1) 
    int player;
    // number of free squares remaining on game board
    unsigned freeslots;
    // this contains one character each for player 1, player 2 and empty
    static const char pchar[3];
public:
    // create a square TicTacToe board with width = n
    TicTacToe(size_t n=3);
    // destructor
    ~TicTacToe();
    // reset can be used to play again after object exists
    void reset();
    // inserter for printing board state to ostream
    friend std::ostream &operator <<(std::ostream &out, const TicTacToe &ttt);
    bool isNotFull() const { return freeslots > 0; }
    // return character representing the player whose turn it is
    char playerTurn() const { return pchar[player]; }
    // return character representing player who just took a turn
    char lastPlayer() const { return pchar[1-player]; }
    // return true if the slot was successfully marked for current player
    bool applyTurn(unsigned slot);
    // return true if the last player just won
    bool winner() const;
};

As you can see, the counting of turns has disappeared, replaced by a single private member player which keeps track of which player is next to play. You might want to start with this template and see if you can fill in the missing member functions based on the code you've already written.

share|improve this answer
    
Sorry for the input of slotChoice. Yeah, I should have make better use of the #define or implement more flexibility as well, maybe like GRID_SIZE/NUMBER_OF_ROWS? –  leansie May 29 at 14:06
    
I'd define std::string hbar once at startup time to contain the horizontal bar, and then print the grid perhaps with nested for loops for rows and columns. –  Edward May 29 at 14:11
add comment

The code already looks nice. It is easy to understand and well structured. I would change these things:

  • You define GRID_SIZE as 9. This made me immediately wonder: Why not 3?, since Tic Tac Toe is played on a 3 times 3 board. Usually the 'board size' is measured in one dimension only.

  • Defining this constant is probably useless, unless you want to switch to other board sizes later.

  • But if you define this constant, you should change all the other code so that by just changing this constant you can immediately play on a 9 times 9 board. Currently there are lots of 3 in the code.

  • The line bool isP2 (currentTurn % 2 == 0); looks unusual. Simple variables are usually initialized using the assigment operator, like this: bool isP2 = (currentTurn % 2 == 0);.

  • Before checking isEmpty(gameGrid[slotChoice]) you should make sure that 0 <= slotChoice && slotChoice < GRID_SIZE. Otherwise you are invoking 'undefined behaviour'.

  • You should move the // Check winner code out of the while loop. The loop should then be while (!checkWon(gameGrid) && !isFull(gameGrid)) { ... }.

  • In the expression (i+1) % 3 == 0 && i != 0 you can leave out the i != 0, since that can never be true.

  • The function inputToSlot is defined but not used.

  • In the function isEmpty you can simply write:

    bool isEmpty(char input) {
        return input == ' ';
    }
    
  • In the functions rowWon, columnWon and diagonalWon you don't need to check for isEmpty(secondInRow). It is enough to check !isEmpty(firstInRow) && secondInRow == firstInRow && thirdInRow == firstInRow.

share|improve this answer
    
Wow, as for the bool isP2, I don't even know that I have missed the assignment operator and it still works? And I totally forgot about the inputToSlot() function as well, darn. –  leansie May 29 at 14:13
    
-1 for "The code already looks nice. It is easy to understand and well structured."; the improvement list for this code is longer than the code itself, it's easy to understand only because the problem at hand is simple, and the structuring is obviously lousy due to lack of more complex data structures/objects than char array... –  vaxquis May 30 at 17:32
    
@vaxquis: Although the list of suggestions is long, I still found the code easy to read. The point is: if you are currently in the stage of writing a tic tac toe game, you are still at the very beginning of your programming knowledge. Keeping that in mind, the code could also have been a single main function with lots of global variables, deeply nested conditionals and the like. This code is structured into some functions with really good names, and that's a great start for doing a code review and improving the remaining details. –  Roland Illig Jun 1 at 15:45
    
@RolandIllig just because something can be done worse, doesn't mean it's good. I find this kind of reasoning a non sequitur, my friend. Since OP clearly stated he has previous experience in high-level programming, I can only assume he wants to improve, not get friendly pats on the back. Besides, since when "having code structured into functions with really good names" is a reason for OOP code to be good or nice? Actually, having excess functions in OOP code, all getting the very same char[] param, is the very first reason the code is flawed. –  vaxquis Jun 1 at 15:51
add comment

The others have mentioned many great things, but I'll add some thing that were left out:

  • You can shorten the code a bit by defining main() at the very bottom, thus you will no longer need the function prototypes above.

  • Whenever possible, use a storage container instead of C-arrays in C++. They can be especially problematic when being passed to functions because they decay to pointers; the array itself isn't actually passed. Storage containers are also safer and are more useful.

    For this program, you may use std::vector or, if you have C++11, std::array. There are many resources around on how to use storage containers effectively.

  • Instead of using += for 1:

    currentTurn += 1;
    

    just use ++ (pre or post increment):

    currentTurn++;
    
  • A simple conditional like this:

    bool isEmpty(char input) {
        if (input == ' '){
            return true;
        }
        return false;
    }
    

    can be shortened to this:

    bool isEmpty(char input) {
        return (input == ' ');
    }
    

    This will automatically return true or false.

  • Instead of manually filling an array with a loop:

    for (int i = 0; i < GRID_SIZE; ++i) {
        gameGrid[i] = ' ';
    }
    

    use std::fill_n:

    std::fill_n(gameGrid, GRID_SIZE, ' ');
    

    This is part of <algorithm>, so it works with both C-arrays and containers.

share|improve this answer
    
Oh shoots... Python habits. (no ++ operator) –  leansie May 29 at 13:53
    
Nothing wrong with fill, but to my eye in this case, I think I'd prefer std::fill_n(gameGrid, GRID_SIZE, ' '); for purely aesthetic reasons. –  Edward May 29 at 13:55
    
@Edward: Ah, I didn't catch that one! Thanks! I'll add it anyway. –  Jamal May 29 at 14:06
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.