Thread: Critique my TicTacToe game (terminal-based)

  1. #1
    Time-Lord Victorious! The Doctor's Avatar
    Join Date
    Aug 2012
    Location
    Perth, Western Australia
    Posts
    50

    Critique my TicTacToe game (terminal-based)

    I kind of wanted to brag about my first real C++ program. I know it's simple, but I just am really happy about it.

    While you're looking at it, if you see something I could have done better, please feel free to tell me.

    Thanks! (Plasticcaz is an alias of mine)

    EDIT: By the way, I'm coming from a Java background, with a little experience in C.

    main.cpp:
    Code:
    #include <iostream> 
    #include "grid.hpp" 
     
    using namespace std; 
      
      //AUTHOR: Plasticcaz;
    #define VERSION 1.3
    //ORIGINAL CREATION: 23/10/2012
    //LAST MODIFIED: 30/10/2012
    
    // KNOWN ISSUES:
    /*      If a number and then a letter is inputed as a move (EXAMPLE 7a), it
            will make the move, and then tell the next user that his input 
            failed, and needs to try again. Must be a problem with the "Solution" to
            infinite loops when there is invalid input.
                                                    */
    
    // CHANGE LOG:
    
    // Version 1.3 (30/10/2012): 
    //              Fixed an issue where I had only allotted 8 chars to the
    //              boxes array in grid.cpp.
    
    // Version 1.2 (30/10/2012): 
    //              Got rid of unused variable bool running. No purpose for it.
    
     
    void switchPlayers( int *playerNum) 
    { 
        if (*playerNum == PLAYER_ONE) 
        { 
            *playerNum = PLAYER_TWO; 
        } else 
        { 
            *playerNum = PLAYER_ONE; 
        } 
    } 
     
    int main() 
    { 
        int playerNum; 
        int playerWon; 
        Grid* grid; 
     
        grid = new Grid(); 
     
        playerNum = PLAYER_ONE; 
        playerWon = NO_ONE; 
     
        cout << "\n~-~-~TIC TAC TOE v" << VERSION << "~-~-~\n\n"; 
        cout << "by Plasticcaz\n\n~-~-~-~-~-~-~-~-~-~-~-~-~-\n\n\n"; 
     
        grid->printGrid(); 
        try 
        { 
            while( grid->isMoreMoves() && ( grid->gameWon() == NO_ONE)) //ie. no Player has won. 
            { 
                grid->makeMove( playerNum); 
                cout << "\n\n"; 
                grid->printGrid(); 
     
                switchPlayers( &playerNum); 
            } 
        } catch ( int e) 
        { 
            cout << "ERROR ID: " << e << endl; 
        } 
     
        playerWon = grid->gameWon(); 
     
        if ( playerWon != NO_ONE) 
        { 
            cout << "Player " << playerWon << " has won! Congratulations!" << endl; 
        } else 
        { 
            cout << "The game was a draw. Better luck next time!" << endl; 
        } 
     
        cout << "See you next time!" << endl << endl; 
     
        delete grid; 
     
        cout << "\n\n...Press ENTER to Exit System..."; 
        cin.get(); 
        cin.get(); 
     
        return 0; 
    }
    grid.hpp:
    Code:
    #ifndef GRID_HPP_INCLUDED 
    #define GRID_HPP_INCLUDED 
     
    //ERRORS 
    #define INVALID_PLAYER 1 
     
    //PLAYER WON CONSTANTS 
    #define NO_ONE 0 
    #define PLAYER_ONE 1 
    #define PLAYER_TWO 2 
     
     
    class Grid 
    { 
        private: 
            char *boxes; 
            char *marks; 
            int playerWon; 
     
        public: 
            Grid(); 
     
            ~Grid(); 
     
            void makeMove( int); 
     
            bool isMoreMoves(); 
     
            int gameWon(); 
     
            void printGrid(); 
    }; 
     
     
     
     
    #endif // GRID_HPP_INCLUDED
    grid.cpp:
    Code:
    #include <iostream> 
    #include "grid.hpp" 
     
    using namespace std; 
     
    Grid::Grid() 
    { 
        boxes = new char[9]; 
        marks = new char[2]; 
        boxes[0] = '1'; 
        boxes[1] = '2'; 
        boxes[2] = '3'; 
        boxes[3] = '4'; 
        boxes[4] = '5'; 
        boxes[5] = '6'; 
        boxes[6] = '7'; 
        boxes[7] = '8'; 
        boxes[8] = '9'; 
     
     
        marks[0] = 'X'; 
        marks[1] = 'O'; 
     
        playerWon = 0; 
    } 
        Grid::~Grid() 
        { 
            delete boxes; 
            delete marks; 
        } 
     
    void Grid::makeMove( int playerNo) 
    { 
        int block; 
        bool validBlock = false; 
     
        if ( playerNo > 2 || playerNo < 1) 
        { 
            cout << "\nERROR: Invalid player being asked to move.\n"; 
            throw INVALID_PLAYER; 
        } 
     
        do 
        { 
            cout << "\nPlayer " << playerNo <<" , make your move: "; 
            //INPUT THE DESIRED BLOCK WHILE CHECKING IF IT WORKED 
            //Found this solution to my problem at: http://www.daniweb.com/software-development/cpp/threads/314840/infinite-loop-due-to-char-input 
            while(!(cin >> block)) 
            { 
                cout << "Input failed...try again\n\n\n"; 
                printGrid(); 
                cout << "\nPlayer " << playerNo <<" , make your move: "; 
                cin.clear(); 
                while(cin.get() != '\n'); 
            } 
     
            if ( block >= 1 && block <= 9) 
            { 
                if ( boxes[block - 1] != marks[0] && 
                    boxes[block - 1] != marks[1]    ) 
                { 
                    validBlock = true; 
                } 
            } 
     
     
            if( !validBlock) 
            { 
                cout << "Invalid choice." << endl; 
            } 
     
        } while (! validBlock); 
     
        boxes[block - 1] = marks[playerNo - 1]; 
        cout << "\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n"; 
     
    } 
     
    bool Grid::isMoreMoves() 
    { 
        bool moreMoves = false; 
     
        for ( int i = 0; i < 9; i++) 
        { 
            if ( (boxes[i] != marks[0]) && (boxes[i] != marks[1]) ) 
            { 
                moreMoves = true; 
            } 
        } 
     
        return moreMoves; 
    } 
     
    int Grid::gameWon() 
    { 
        for ( int m = 0; m < 2; m++) 
        { 
            //Each possible way to win. 
            if ( ((boxes[0] == marks[m]) && (boxes[1] == marks[m]) && (boxes[2] == marks[m])) || 
                ((boxes[3] == marks[m]) && (boxes[4] == marks[m]) && (boxes[5] == marks[m])) || 
                ((boxes[6] == marks[m]) && (boxes[7] == marks[m]) && (boxes[8] == marks[m])) || 
                ((boxes[6] == marks[m]) && (boxes[3] == marks[m]) && (boxes[0] == marks[m])) || 
                ((boxes[7] == marks[m]) && (boxes[4] == marks[m]) && (boxes[1] == marks[m])) || 
                ((boxes[8] == marks[m]) && (boxes[5] == marks[m]) && (boxes[2] == marks[m])) || 
                ((boxes[6] == marks[m]) && (boxes[4] == marks[m]) && (boxes[2] == marks[m])) || 
                ((boxes[8] == marks[m]) && (boxes[4] == marks[m]) && (boxes[0] == marks[m]))  ) 
            { 
                playerWon = m + 1; //Set it equal to the player that won. 
            } 
        } 
     
        return playerWon; 
        } 
     
    void Grid::printGrid() 
    { 
        cout << boxes[6] << "|" << boxes[7] << "|" << boxes[8] << endl; 
        cout << "-----" << endl; 
        cout << boxes[3] << "|" << boxes[4] << "|" << boxes[5] << endl; 
        cout << "-----" << endl; 
        cout << boxes[0] << "|" << boxes[1] << "|" << boxes[2] << endl; 
    }
    Last edited by The Doctor; 10-29-2012 at 08:44 PM. Reason: Updated to Version 1.3
    Code:
    if (codeWorks( code) && !youCanDoSomethingBetter( code) )
    {
         beHappy();
    } else
    {
         fixCode( code);
    }

  2. #2
    Registered User
    Join Date
    Oct 2012
    Posts
    5
    In your grid::ismoremoves() function, you are only checking 8 boxes:

    if i < 9 then it will stop on 9 and not continue through the last box. If you want to check all nine, it needs to be either

    i <= 9 or i < 10

  3. #3
    Time-Lord Victorious! The Doctor's Avatar
    Join Date
    Aug 2012
    Location
    Perth, Western Australia
    Posts
    50
    Quote Originally Posted by pogrady View Post
    In your grid::ismoremoves() function, you are only checking 8 boxes:

    if i < 9 then it will stop on 9 and not continue through the last box. If you want to check all nine, it needs to be either

    i <= 9 or i < 10
    boxes is declared as

    Code:
        boxes = new char[8];
    EDIT: Hang on a minute????? I declared it as only having 8 characters, and yet it seems to still be working fine! I'll go and fix that now though. I guess that means it's assigning a value to index 8, even though it's technically not been allotted to it? EDIT AGAIN: Fixed, and updated original post to V1.3



    And I'm checking from 0 - less than 9. I think I'm checking all 9 boxes.

    I check 0,1,2,3,4,5,6,7, and 8. I'm pretty sure 9 doesn't exist.
    Last edited by The Doctor; 10-29-2012 at 08:45 PM.
    Code:
    if (codeWorks( code) && !youCanDoSomethingBetter( code) )
    {
         beHappy();
    } else
    {
         fixCode( code);
    }

  4. #4
    Registered User
    Join Date
    Oct 2012
    Posts
    5
    No your right. Sorry about that.

  5. #5
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    In your constructor, you write:
    Code:
    boxes = new char[9];
    marks = new char[2];
    but in your destructor, you write:
    Code:
    delete boxes;
    delete marks;
    This is wrong as it should be:
    Code:
    delete[] boxes;
    delete[] marks;
    However, dynamic memory allocation was unnecessary to begin with. It would have sufficed to declare:
    Code:
    char boxes[9];
    char marks[2];
    If you really did want dynamic arrays, then you should have used std::vector (or std::string).

    Note that your member functions are not const correct: isMoreMoves and printGrid do not change the observable/logical state of the object, so they should be declared const.

    Instead of using #define to create global constants, consider making them static const members of the class.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  6. #6
    Time-Lord Victorious! The Doctor's Avatar
    Join Date
    Aug 2012
    Location
    Perth, Western Australia
    Posts
    50
    Thanks for the advice. I might fiddle around with the program later.

    As I indicated in my post, I'm coming from a Java background, and so some of the concepts in C++ are new to me.

    I have heard of vectors. They're like an array that has a changeable length am I right? I'm going to have to do some research on that at some point.
    Code:
    if (codeWorks( code) && !youCanDoSomethingBetter( code) )
    {
         beHappy();
    } else
    {
         fixCode( code);
    }

  7. #7
    Lurking whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    9,613
    Concerning the known issue, you might want to change your input routine to something like

    Code:
    while ( !(cin >> block) ) {
       ...
    }
    if (cin.peek() != '\n') {
       // Not a block number by itself (e.g. 7a)
    }
    This presents few issues and is easy to understand. As long as you aren't going to be redirecting stdin or something, then it should integrate well into your existing code.
    Last edited by whiteflags; 10-29-2012 at 11:15 PM.

  8. #8
    Time-Lord Victorious! The Doctor's Avatar
    Join Date
    Aug 2012
    Location
    Perth, Western Australia
    Posts
    50
    Thanks to both laserlight and whiteflags. The code is working a lot better now.

    This is the first I've heard of "const correctness", I guess I'm going to have to do some research on that too. I have done a little, and changed my code accordingly.

    I do however have one more question:

    Quote Originally Posted by laserlight View Post
    Instead of using #define to create global constants, consider making them static const members of the class.
    Why would it be better to make them members of the class rather than let the preprocessor handle them? Isn't it quicker to have the compiler convert it straight to the value? Or am I missing some big advantage to static constants?
    Code:
    if (codeWorks( code) && !youCanDoSomethingBetter( code) )
    {
         beHappy();
    } else
    {
         fixCode( code);
    }

  9. #9
    Time-Lord Victorious! The Doctor's Avatar
    Join Date
    Aug 2012
    Location
    Perth, Western Australia
    Posts
    50
    Sorry for the double post, but here's V1.4 if anyone's interested:

    Main.cpp:
    Code:
    #include <iostream> 
    #include "grid.hpp" 
     
    using namespace std; 
     
    //AUTHOR: Plasticcaz; 
    #define VERSION 1.4 
    //ORIGINAL CREATION: 23/10/2012 
    //LAST MODIFIED: 31/10/2012 
     
    // KNOWN ISSUES: 
    /*    #1 -- RESOLVED (1.4) 
        If a number and then a letter is inputed as a move (EXAMPLE 7a), it 
        will make the move, and then tell the next user that his input  
        failed, and needs to try again. Must be a problem with the "Solution" to 
        infinite loops when there is invalid input. 
                            */ 
     
    // CHANGE LOG:  
     
    // Version 1.4 (31/10/2012) 
    //        Fixed known issue #1. Thanks to whiteflags from the C forums: 
    //        http://cboard.cprogramming.com/cplusplus-programming/151802-critique-my-tictactoe-game-terminal-based.html#post1130786 
             
    //        Changed the delete statements in Grid's destructor to delete[] statements, as they should be. 
     
    //        Made a number of Grid functions const, to ensure const correctness. (The above two were suggested by laserlight, at the  
    //        afore mentioned forums.) 
     
    // Version 1.3 (30/10/2012):  
    //        Fixed an issue where I had only allotted 8 chars to the 
    //        boxes array in grid.cpp. 
     
    // Version 1.2 (30/10/2012):  
    //        Got rid of unused variable bool running. No purpose for it. 
     
     
     
    void switchPlayers( int *playerNum) 
    { 
        if (*playerNum == PLAYER_ONE) 
        { 
            *playerNum = PLAYER_TWO; 
        } else 
        { 
            *playerNum = PLAYER_ONE; 
        } 
    } 
     
    int main() 
    { 
        int playerNum; 
        int playerWon; 
        Grid* grid; 
     
        grid = new Grid(); 
     
        playerNum = PLAYER_ONE; 
        playerWon = NO_ONE; 
     
        cout << "\n~-~-~TIC TAC TOE v" << VERSION << "~-~-~\n\n"; 
        cout << "by Plasticcaz\n\n~-~-~-~-~-~-~-~-~-~-~-~-~-\n\n\n"; 
     
        grid->printGrid(); 
        try 
        { 
            while( grid->isMoreMoves() && ( grid->gameWon() == NO_ONE)) //ie. no Player has won. 
            { 
                grid->makeMove( playerNum); 
                cout << "\n\n"; 
                grid->printGrid(); 
     
                switchPlayers( &playerNum); 
            } 
        } catch ( int e) 
        { 
            cout << "ERROR ID: " << e << endl; 
        } 
     
        playerWon = grid->gameWon(); 
     
        if ( playerWon != NO_ONE) 
        { 
            cout << "Player " << playerWon << " has won! Congratulations!" << endl; 
        } else 
        { 
            cout << "The game was a draw. Better luck next time!" << endl; 
        } 
     
        cout << "See you next time!" << endl << endl; 
     
        delete grid; 
     
        cout << "\n\n...Press ENTER to Exit System..."; 
        cin.get(); 
        cin.get(); 
     
        return 0; 
    }
    Grid.hpp:
    Code:
    #ifndef GRID_HPP_INCLUDED
    #define GRID_HPP_INCLUDED
    
    //ERRORS
    #define INVALID_PLAYER 1
    
    //PLAYER WON CONSTANTS
    #define NO_ONE 0
    #define PLAYER_ONE 1
    #define PLAYER_TWO 2
    
    
    class Grid
    {
        private:
            char *boxes;
            char *marks;
            int playerWon;
    
        public:
            Grid();
    
            ~Grid();
    
            void makeMove(const int);
    
            bool isMoreMoves() const;
    
            int gameWon();
    
            void printGrid() const;
    };
    
    
    
    
    #endif // GRID_HPP_INCLUDED
    grid.cpp:
    Code:
    #include <iostream> 
    #include "grid.hpp" 
     
    using namespace std; 
     
    Grid::Grid() 
    { 
        boxes = new char[9]; 
        marks = new char[2]; 
        boxes[0] = '1'; 
        boxes[1] = '2'; 
        boxes[2] = '3'; 
        boxes[3] = '4'; 
        boxes[4] = '5'; 
        boxes[5] = '6'; 
        boxes[6] = '7'; 
        boxes[7] = '8'; 
        boxes[8] = '9'; 
     
     
        marks[0] = 'X'; 
        marks[1] = 'O'; 
     
        playerWon = 0; 
    } 
        Grid::~Grid() 
        { 
            delete[] boxes; 
            delete[] marks; 
        } 
     
    void Grid::makeMove( const int playerNo) 
    { 
        int block; 
        bool validBlock = false; 
     
        if ( playerNo > 2 || playerNo < 1) 
        { 
            cout << "\nERROR: Invalid player being asked to move.\n"; 
            throw INVALID_PLAYER; 
        } 
     
        do 
        { 
            cout << "\nPlayer " << playerNo <<" , make your move: "; 
            //INPUT THE DESIRED BLOCK WHILE CHECKING IF IT WORKED 
            //Found this solution to my problem at: http://www.daniweb.com/software-development/cpp/threads/314840/infinite-loop-due-to-char-input 
            while(!(cin >> block)) 
            { 
                cout << "Input failed...try again\n\n\n"; 
                printGrid(); 
                cout << "\nPlayer " << playerNo <<" , make your move: "; 
                cin.clear(); 
                while(cin.get() != '\n'); 
            } 
        if ( cin.peek() != '\n') 
        { 
            //Not a block number by itself (eg. 7a) 
            block = 10; //Make it invalid, so the user must redo input. 
        } 
     
            if ( block >= 1 && block <= 9) 
            { 
                if ( boxes[block - 1] != marks[0] && 
                    boxes[block - 1] != marks[1]    ) 
                { 
                    validBlock = true; 
                } 
            } 
     
     
            if( !validBlock) 
            { 
                cout << "Invalid choice." << endl; 
            } 
     
        } while (! validBlock); 
     
        boxes[block - 1] = marks[playerNo - 1]; 
        cout << "\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n"; 
     
    } 
     
    bool Grid::isMoreMoves() const 
    { 
        bool moreMoves = false; 
     
        for ( int i = 0; i < 9; i++) 
        { 
            if ( (boxes[i] != marks[0]) && (boxes[i] != marks[1]) ) 
            { 
                moreMoves = true; 
            } 
        } 
     
        return moreMoves; 
    } 
     
    int Grid::gameWon() 
    { 
        for ( int m = 0; m < 2; m++)  
        { 
            //Each possible way to win. 
            if ( ((boxes[0] == marks[m]) && (boxes[1] == marks[m]) && (boxes[2] == marks[m])) || 
                ((boxes[3] == marks[m]) && (boxes[4] == marks[m]) && (boxes[5] == marks[m])) || 
                ((boxes[6] == marks[m]) && (boxes[7] == marks[m]) && (boxes[8] == marks[m])) || 
                ((boxes[6] == marks[m]) && (boxes[3] == marks[m]) && (boxes[0] == marks[m])) || 
                ((boxes[7] == marks[m]) && (boxes[4] == marks[m]) && (boxes[1] == marks[m])) || 
                ((boxes[8] == marks[m]) && (boxes[5] == marks[m]) && (boxes[2] == marks[m])) || 
                ((boxes[6] == marks[m]) && (boxes[4] == marks[m]) && (boxes[2] == marks[m])) || 
                ((boxes[8] == marks[m]) && (boxes[4] == marks[m]) && (boxes[0] == marks[m]))  ) 
            { 
                playerWon = m + 1; //Set it equal to the player that won. 
            } 
        } 
     
        return playerWon; 
        } 
     
    void Grid::printGrid() const 
    { 
        cout << boxes[6] << "|" << boxes[7] << "|" << boxes[8] << endl; 
        cout << "-----" << endl; 
        cout << boxes[3] << "|" << boxes[4] << "|" << boxes[5] << endl; 
        cout << "-----" << endl; 
        cout << boxes[0] << "|" << boxes[1] << "|" << boxes[2] << endl; 
    }
    Code:
    if (codeWorks( code) && !youCanDoSomethingBetter( code) )
    {
         beHappy();
    } else
    {
         fixCode( code);
    }

  10. #10
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by The Doctor
    Why would it be better to make them members of the class rather than let the preprocessor handle them? Isn't it quicker to have the compiler convert it straight to the value? Or am I missing some big advantage to static constants?
    Macro names do not obey the rules of scope and the type is the type of whatever the value happens to be, rather than a type that you specify (unless you explicitly cast).
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  11. #11
    Time-Lord Victorious! The Doctor's Avatar
    Join Date
    Aug 2012
    Location
    Perth, Western Australia
    Posts
    50
    Okay, I've adjusted the code further... The only preprocessor constant left is VERSION, as I saw no need to put that in the Grid class.

    I'm tempted to put them in a separate class of themselves, as they don't really have much to do with a grid in and of themselves.
    Code:
    if (codeWorks( code) && !youCanDoSomethingBetter( code) )
    {
         beHappy();
    } else
    {
         fixCode( code);
    }

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Need help with tictactoe game(C)
    By devildog561 in forum Game Programming
    Replies: 2
    Last Post: 03-10-2012, 06:46 PM
  2. Need help with tictactoe game
    By devildog561 in forum C Programming
    Replies: 1
    Last Post: 03-10-2012, 06:36 PM
  3. i need this for my tictactoe game. pls help.
    By riel in forum C Programming
    Replies: 9
    Last Post: 01-21-2008, 05:10 AM
  4. Help Wit TicTacToe Game
    By WackoWolf in forum C Programming
    Replies: 5
    Last Post: 11-17-2005, 12:20 PM
  5. My TicTacToe game
    By Vicious in forum Game Programming
    Replies: 11
    Last Post: 05-26-2002, 11:02 AM