Simple text-based tic-tac-toeTic-tac-toe in SQL with optimal AIBasic Tic-Tac-ToeGo (board game) in JavaSimple...
Porting Linux to another platform requirements
Why do neural networks need so many training examples to perform?
Why has the mole been redefined for 2019?
One Half of Ten; A Riddle
What is the wife of a henpecked husband called?
Can a Pact of the Blade warlock use the correct existing pact magic weapon so it functions as a "Returning" weapon?
My cat mixes up floors. How can I help him?
In Linux what happens if 1000 files in a directory are moved to another location while another 300 files were added to the source directory?
Bash Script Function Return True-False
Why exactly do action photographers need high fps burst cameras?
Why did Democrats in the Senate oppose the Born-Alive Abortion Survivors Protection Act (2019 S.130)?
Moving from one room/cave to another within the same dungeon
Is it possible to "destroy" a wallet?
How does Leonard in "Memento" remember reading and writing?
Why is working on the same position for more than 15 years not a red flag?
Finding a mistake using Mayer-Vietoris
speculum - A simple, straightforward Arch Linux mirror list optimizer
Replacement expressions
It took me a lot of time to make this, pls like. (YouTube Comments #1)
How can my powered armor quickly replace its ceramic plates?
Can a long polymer chain interact with itself via van der Waals forces?
Graph with overlapping labels
Simple text-based tic-tac-toe
Why publish a research paper when a blog post or a lecture slide can have more citation count than a journal paper?
Simple text-based tic-tac-toe
Tic-tac-toe in SQL with optimal AIBasic Tic-Tac-ToeGo (board game) in JavaSimple Tic Tac Toe gameTic Tic Tic Tac Tac Tac Toe Toe ToeSimple Tic-Tac-Toe gameSimple Tic Tac ToeBoggle board solver in C++Console Based Tic-Tac-Toe in C++Text-based Tic Tac Toe in C
$begingroup$
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:
- Create a 4x4 game board
- Prompt the first user (the 'x' user) to enter their name
- Prompt the second user (the 'o' user) to enter their name
- Prompt the 'x' user to select a grid position where they would like to place an 'x'.
- Prompt the 'o' user to select a grid position where they would like to place an 'o'.
- 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_ */
c++ beginner object-oriented tic-tac-toe
$endgroup$
add a comment |
$begingroup$
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:
- Create a 4x4 game board
- Prompt the first user (the 'x' user) to enter their name
- Prompt the second user (the 'o' user) to enter their name
- Prompt the 'x' user to select a grid position where they would like to place an 'x'.
- Prompt the 'o' user to select a grid position where they would like to place an 'o'.
- 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_ */
c++ beginner object-oriented tic-tac-toe
$endgroup$
add a comment |
$begingroup$
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:
- Create a 4x4 game board
- Prompt the first user (the 'x' user) to enter their name
- Prompt the second user (the 'o' user) to enter their name
- Prompt the 'x' user to select a grid position where they would like to place an 'x'.
- Prompt the 'o' user to select a grid position where they would like to place an 'o'.
- 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_ */
c++ beginner object-oriented tic-tac-toe
$endgroup$
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:
- Create a 4x4 game board
- Prompt the first user (the 'x' user) to enter their name
- Prompt the second user (the 'o' user) to enter their name
- Prompt the 'x' user to select a grid position where they would like to place an 'x'.
- Prompt the 'o' user to select a grid position where they would like to place an 'o'.
- 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_ */
c++ beginner object-oriented tic-tac-toe
c++ beginner object-oriented tic-tac-toe
edited 2 hours ago
Deduplicator
11.4k1850
11.4k1850
asked 4 hours ago
mo2mo2
384
384
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
$begingroup$
First, congratulations on getting it to work. Most falter earlier.
Now, let's see what you can improve:
Generally, you use multiple files to enable separate compilation, and in line with that you don't #include source-files, only header-files.
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.
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.You don't use anything from
<iomanip>
, so don't include it.usrs
is pretty useless. Just use astd::array<User, 2>
and be done with it. As a bonus, you no longer need astd::list
.Try to initialize variables on definition. As a bonus, you can use
auto
, thus avoiding error-prone repetition of useless trivia.There are far simpler and more efficient ways to parse a number than creating and destroying a
std::stringstream
. For examplestd::stoi()
.Never assume input is valid. Even if it should have the right format, a surprise, it might still ask for an illegal operation.
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.If you output single characters, prefer character literals to length-1 string-literals. It might be marginally more efficient.
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.
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.
$endgroup$
$begingroup$
@Duplicator: thx you very much for feedback
$endgroup$
– mo2
17 mins ago
add a comment |
$begingroup$
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.
$endgroup$
$begingroup$
Thx you very much for feedback
$endgroup$
– mo2
18 mins ago
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f214465%2fsimple-text-based-tic-tac-toe%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
First, congratulations on getting it to work. Most falter earlier.
Now, let's see what you can improve:
Generally, you use multiple files to enable separate compilation, and in line with that you don't #include source-files, only header-files.
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.
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.You don't use anything from
<iomanip>
, so don't include it.usrs
is pretty useless. Just use astd::array<User, 2>
and be done with it. As a bonus, you no longer need astd::list
.Try to initialize variables on definition. As a bonus, you can use
auto
, thus avoiding error-prone repetition of useless trivia.There are far simpler and more efficient ways to parse a number than creating and destroying a
std::stringstream
. For examplestd::stoi()
.Never assume input is valid. Even if it should have the right format, a surprise, it might still ask for an illegal operation.
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.If you output single characters, prefer character literals to length-1 string-literals. It might be marginally more efficient.
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.
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.
$endgroup$
$begingroup$
@Duplicator: thx you very much for feedback
$endgroup$
– mo2
17 mins ago
add a comment |
$begingroup$
First, congratulations on getting it to work. Most falter earlier.
Now, let's see what you can improve:
Generally, you use multiple files to enable separate compilation, and in line with that you don't #include source-files, only header-files.
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.
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.You don't use anything from
<iomanip>
, so don't include it.usrs
is pretty useless. Just use astd::array<User, 2>
and be done with it. As a bonus, you no longer need astd::list
.Try to initialize variables on definition. As a bonus, you can use
auto
, thus avoiding error-prone repetition of useless trivia.There are far simpler and more efficient ways to parse a number than creating and destroying a
std::stringstream
. For examplestd::stoi()
.Never assume input is valid. Even if it should have the right format, a surprise, it might still ask for an illegal operation.
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.If you output single characters, prefer character literals to length-1 string-literals. It might be marginally more efficient.
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.
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.
$endgroup$
$begingroup$
@Duplicator: thx you very much for feedback
$endgroup$
– mo2
17 mins ago
add a comment |
$begingroup$
First, congratulations on getting it to work. Most falter earlier.
Now, let's see what you can improve:
Generally, you use multiple files to enable separate compilation, and in line with that you don't #include source-files, only header-files.
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.
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.You don't use anything from
<iomanip>
, so don't include it.usrs
is pretty useless. Just use astd::array<User, 2>
and be done with it. As a bonus, you no longer need astd::list
.Try to initialize variables on definition. As a bonus, you can use
auto
, thus avoiding error-prone repetition of useless trivia.There are far simpler and more efficient ways to parse a number than creating and destroying a
std::stringstream
. For examplestd::stoi()
.Never assume input is valid. Even if it should have the right format, a surprise, it might still ask for an illegal operation.
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.If you output single characters, prefer character literals to length-1 string-literals. It might be marginally more efficient.
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.
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.
$endgroup$
First, congratulations on getting it to work. Most falter earlier.
Now, let's see what you can improve:
Generally, you use multiple files to enable separate compilation, and in line with that you don't #include source-files, only header-files.
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.
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.You don't use anything from
<iomanip>
, so don't include it.usrs
is pretty useless. Just use astd::array<User, 2>
and be done with it. As a bonus, you no longer need astd::list
.Try to initialize variables on definition. As a bonus, you can use
auto
, thus avoiding error-prone repetition of useless trivia.There are far simpler and more efficient ways to parse a number than creating and destroying a
std::stringstream
. For examplestd::stoi()
.Never assume input is valid. Even if it should have the right format, a surprise, it might still ask for an illegal operation.
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.If you output single characters, prefer character literals to length-1 string-literals. It might be marginally more efficient.
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.
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.
edited 2 hours ago
answered 3 hours ago
DeduplicatorDeduplicator
11.4k1850
11.4k1850
$begingroup$
@Duplicator: thx you very much for feedback
$endgroup$
– mo2
17 mins ago
add a comment |
$begingroup$
@Duplicator: thx you very much for feedback
$endgroup$
– mo2
17 mins ago
$begingroup$
@Duplicator: thx you very much for feedback
$endgroup$
– mo2
17 mins ago
$begingroup$
@Duplicator: thx you very much for feedback
$endgroup$
– mo2
17 mins ago
add a comment |
$begingroup$
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.
$endgroup$
$begingroup$
Thx you very much for feedback
$endgroup$
– mo2
18 mins ago
add a comment |
$begingroup$
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.
$endgroup$
$begingroup$
Thx you very much for feedback
$endgroup$
– mo2
18 mins ago
add a comment |
$begingroup$
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.
$endgroup$
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.
answered 1 hour ago
EdwardEdward
46.9k378212
46.9k378212
$begingroup$
Thx you very much for feedback
$endgroup$
– mo2
18 mins ago
add a comment |
$begingroup$
Thx you very much for feedback
$endgroup$
– mo2
18 mins ago
$begingroup$
Thx you very much for feedback
$endgroup$
– mo2
18 mins ago
$begingroup$
Thx you very much for feedback
$endgroup$
– mo2
18 mins ago
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f214465%2fsimple-text-based-tic-tac-toe%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown