Argthtjtr

I am learning C++ and I need help to review my first code and see how I could improve it in any way. Any suggestions are welcome. It was tested and it is free of bug.

The code is a game called TicTacToe and do the following:

1. Create a 4x4 game board


2. Prompt the first user (the 'x' user) to enter their name


3. Prompt the second user (the 'o' user) to enter their name


4. Prompt the 'x' user to select a grid position where they would like to place an 'x'.


5. Prompt the 'o' user to select a grid position where they would like to place an 'o'.


6. After each user has a turn, check for any row, column, diagonal that has 4 'x's or 4 'o's.
   
   
   
   
   • If 4 'x's are found in the same col, row, diagonal, declare the 'x' user the winner.
   
   
   • If 4 'o's are found in the same col, row, diagonal, declare the 'o' user the winner.
   
   
   • End the game and declare the winner.
   
   
   • If the grid is filled (each player gets 8 turns) and there is not a row, column, diagonal with 4 of the same symbol, the game is tied. Declare a tie game.
   
   
   
   

main.cpp

#include "main.hpp"

#include "User.cpp"

#include "Gameboard.cpp"



int main() {

    int    count=1, out=1;

    string str0 ="";

    //Create a 4x4 game board

    Gameboard game; 

    //Prompt users to enter their name

    usrs players; 

    players = create_2user();

    //Create list, list iterator

    list<User> playerList = { players.usr0, players.usr1 };

    //Play until there is a winner or the gird is filled

    while((count < 16)&&(out != 0)) {

        for( User& usr : playerList ) {

            //Prompt users to select a grid position

            cout<<"n       "<< usr.get_name() <<", select a grid position: n";

            cout<<"n"; game.printInfo(); cout<<"n > ";

            cin>>str0;     

            //update the gameboard after converting str0 into coordinate ( i, j )

            game.updateBoard(str0, usr.get_symbol());

            //check if four symbols are aligned:

            if ( game.findFour(usr.get_symbol())==1 ) { 

                cout<<"n"<<usr.get_name() <<" WINS!n"; 

                out=0; break; }

            else if( count >= 16 ) { 

                cout<<"nThe game is tiedn"; 

                game.printInfo(); 

                out=0; break; }

            ++count;

        }

    }

   return 0;

}

User.cpp

#include "main.hpp"

using namespace std;

/* ----------- class User ------------------------------------------- */

class User {                             //create a CLASS (type) called 'User'

    string name;                             //data attribute - private

    char   chr;

    public:                                  //procedural attribute- public

        User();                                  //constructors         

        void set_name  (string in_name);         //mutator method : SETTER

        void set_symbol(char   in_chr);           

        string get_name();                   //accessor method: GETTER

        char   get_symbol();

        void  printInfo();                       //helper functions

};

User::User()  { name="Unkonw"; chr='.'; }          //define the constructor



void User::set_name(string in_name) { name  = in_name; }    //mutator method

void User::set_symbol (char in_chr) { chr   = in_chr; } 

string User::get_name()   { return name; }                  //accessor method

char   User::get_symbol() { return chr; }

void   User::printInfo() { cout<<name<<"t"<<chr; }



/* ----------- use class User --------------------------------------- */

struct usrs { User usr0, usr1; };



usrs create_2user() {

    User array_usr[2];

    char array_chr = { 'x', 'o' };

    string str0;



    for(int i=0;i<2;i++) {

        cout<<"Enter player "<<i<<"'s name: ";

        cin>>str0;

        array_usr[i].set_name(str0);

        array_usr[i].set_symbol(array_chr[i]);

    }

    usrs result = { array_usr[0], array_usr[1] };

    return result;

}

Gameboard.cpp

#include "main.hpp"

using namespace std;

/* ----------- class Gameboard -------------------------------------- */

class Gameboard {

    string gameSpace[4][4];

    char chr;

    public:

        Gameboard(); //initialize the board with '-' in all 16 spaces

        void setGameSpace(int row,int column, char value); //x,y,or '-' in each game square

        string getGameSpace(int row,int column);

        int findFour(char chr); //four 'x's in any row 'wins'

        void printInfo(); //print the game board in a 4x4 matrix

        void updateBoard(string str0, char symbol);

};



Gameboard::Gameboard() { //define the constructor

    for(int i=0;i<4;i++) { 

        for(int j=0;j<4;j++) {

            gameSpace[i][j] = to_string( (i+1)*10 + (j+1) );

            //test0: OK - diag0 - if(i==j) { gameSpace[i][j] = "x"; }

            //test1: OK - diag1 - if(i==(3-j)) { gameSpace[i][j] = "x"; }

            //test2: OK - row - gameSpace[1][j] = "x";

            //test3: OK - col - gameSpace[i][1] = "x";

        };

    };

}



void Gameboard::setGameSpace(int row, int column, char value) { gameSpace[row][column] = value;} //mutator method

string Gameboard::getGameSpace(int row, int column) { return gameSpace[row][column];} //accessor method



void Gameboard::printInfo() { //print the game board in a 4x4 matrix

    for(int i=0;i<4;i++) { 

        for(int j=0;j<4;j++) {

            cout<<gameSpace[i][j]<<"t";

        };

        cout<<"n";

    };

}

int Gameboard::findFour(char chr) { //four symbols in any row, col, diagonals 'wins'

    int int_dg0=0, int_dg1=0;

    for(int i=0;i<4;i++) { 

        int int_row=0, int_col=0;

        for(int j=0;j<4;j++) {

            if(gameSpace[i][j][0]==chr) { ++int_row;}

            if(gameSpace[j][i][0]==chr) { ++int_col;}

            if( (gameSpace[i][j][0]==chr)&&(i==j) ) { ++int_dg0;}

            if( (gameSpace[i][j][0]==chr)&&(i==(3-j)) ) { ++int_dg1;}

        };

        if((int_row==4)||(int_col==4)||(int_dg0==4)||(int_dg1==4)) { return 1; }

    };

    return 0;

}



void Gameboard::updateBoard(string str0, char symbol) {

    //Convert player's input in coordinates

    int row=0, column=0, k=0;

    stringstream(str0) >> k;

    row=k/10-1;

    column=k%10-1;

    //Update gameboard setGameSpace(int row, int column, char value)

    gameSpace[row][column] = symbol;

}

main.hpp

#ifndef MAIN_HPP_

#define MAIN_HPP_



#include <iomanip>

#include <iostream>

#include <list>

#include <string>

#include <sstream>

using namespace std;



#endif /* MAIN_HPP_ */

First, congratulations on getting it to work. Most falter earlier.

Now, let's see what you can improve:

1. Generally, you use multiple files to enable separate compilation, and in line with that you don't #include source-files, only header-files.




2. Please, please ensure your files are consistently lower-case, even if Windows doesn't care. It's not quite as bad for source-files as for header-files, as nobody should #include them.




3. using namespace std; is plain evil. That namespace is not designed for inclusion, thus there is no comprehensive, fixed and reliable list of its contents.
   
   See "Why is “using namespace std;” considered bad practice?" for details.




4. You don't use anything from <iomanip>, so don't include it.




5. usrs is pretty useless. Just use a std::array<User, 2> and be done with it. As a bonus, you no longer need a std::list.




6. Try to initialize variables on definition. As a bonus, you can use auto, thus avoiding error-prone repetition of useless trivia.




7. There are far simpler and more efficient ways to parse a number than creating and destroying a std::stringstream. For example std::stoi().




8. Never assume input is valid. Even if it should have the right format, a surprise, it might still ask for an illegal operation.




9. A block is a full statement. Adding an additional semicolon is valid most of the time, because it is its own empty statement. You will get problems with else-branches though.




10. If you output single characters, prefer character literals to length-1 string-literals. It might be marginally more efficient.




11. It's acceptable to indent access-specifiers one level. But then all members should be indented two levels, not only those after the first access-specifier.




12. Use the ctor-init-list to initialise members, though prefer in-class-initialisers if you can. Inside a ctor-body is far too late for initialisation.

That should be enough to point you in the right direction.

Here are a number of things that may 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. It's especially bad to use it when writing headers.

Separate interface from implementation

The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. The reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file. In other words, split your existing Gameboard.cpp and User.cpp file into a .h file and a .cpp file and only #include the .h files in main.cpp.

Separate I/O from initialization

The create_2user() function prints prompts, reads responses and then creates User objects. Better design would be to separate prompts and input from initialization. In other words, you could ask for names within main and then after both names are received, construct User objects.

Understand compound objects

In the User.cpp file, we have this struct:

struct usrs { User usr0, usr1; };

Which is returned by this function:

usrs create_2user() 

But then in main what we actually use is a list:

list<User> playerList = { players.usr0, players.usr1 };

What would make things simpler would be to take the advice in the previous section (asking for names before object creation) and then using those directly to initialize the compound structure you actually want as further shown below.

Use appropriate data structures

The std::list used in main to store the players is generally not a very good structure to use for that purpose. It's typically implemented as a linked list which supports constant time insertion and removal of items but no random access. You don't need that for this program. Instead, I'd recommend using std::array since no insertion, removal or lookup is required.

Don't use a single "include everywhere" file

Generally, it's better practice not to have a single header file that's included everywhere, but individual header files that only include the minimally sufficient interface description as mentioned above.

Use more whitespace to enhance readability of the code

Instead of crowding things together like this:

while((count < 16)&&(out != 0)) {

most people find it more easily readable if you use more space:

while ((count < 16) && (out != 0)) {

Use consistent formatting

The code as posted has inconsistent use of spaces within conditional clauses (as with while and for). Pick a style and apply it consistently.

Avoid magic numbers

One of the lines of code here is this:

while((count < 16)&&(out != 0)) {

First, the number 16 has significance, but it's not immediately obvious what the significance is. Second, since it seems to be related to the dimension 4 within the Gameboard class, should it be stored there?

Use appropriate data types

It appears that out is only intended to be 0 or 1. That strongly suggests that it should be of type bool instead of int.

Use better variable names

The variable name players is good, but the name out is not. The first name explains something about what the variable means within the context of the code, but the latter is only confusing. A better name might be playing.

Use const where possible

The Gameboard::printInfo and User::get_name functions do not (and should not) alter the underlying structurs and should therefore be declared const.

Don't write getters and setters for every class

C++ isn't Java and writing getter and setter functions for every C++ class is not good style. Instead, move setter functionality into constructors and think very carefully about whether a getter is needed at all. In this code, neither getter nor setter for Gameboard is ever used, which emphasizes why they shouldn't be written in the first place.

Put each statement on a single line

It detrimental to the readability of your code to jam multiple statements on a single line like this:

cout<<"n"; game.printInfo(); cout<<"n > ";

Instead, I'd have preferred to write that like this:

std::cout << 'n' << game << "n > ";

Which brings us to the next suggestion:

Prefer a stream inserter to a custom print routine

Your custom Gameboard::printInfo routine could instead be written as a stream inserter:

friend std::ostream& operator<<(std::ostream& out, const Gameboard& game) {

    for(int i = 0; i < 4; ++i) { 

        for(int j = 0; j < 4; ++j) {

            out << game.gameSpace[i][j] << 't';

        }

        out << 'n';

    }

    return out;

}

Eliminate spurious semicolons

With the commented-out lines removed, the Gameboard constructor looks like this:

Gameboard::Gameboard() { //define the constructor

    for(int i=0;i<4;i++) { 

        for(int j=0;j<4;j++) {

            gameSpace[i][j] = std::to_string( (i+1)*10 + (j+1) );

        };   // <-- no semicolon here

    };  // <-- no semicolon here

}

As marked by the comments, there are spurious semicolons there which should be removed.

Reconsider the interface

Right now there is little relationship between the game play, defined in main, and the Gameboard class. It would likely make the code simpler and better to move most of the game logic inside Gameboard to keep it all in one place. I'd suggest that it may also make sense to have the Gameboard object keep track of players. If fully encapsulated, main might look like this:

int main() {

    Game::play();

}

where play could be a static function that does everything main does right now.