patterncppMinorCanonical
Tic Tac Toe C++ with classes
Viewed 0 times
withtoetictacclasses
Problem
After A ton of work I finally have a working Tic Tac Toe game that I am very proud of, and it works great. I would like some feedback on anything that can be improved/ needs to be changed.
I had a problem implementing my header guards. After I finished everything, my IDE automatically sets up
Player Class:
Player cpp:
board class:
board cpp:
```
#include "board.h"
//outputs the board onto the screen
void board::drawboard() {
std::cout 0 && boardIndex = 5; i++, k--) {
I had a problem implementing my header guards. After I finished everything, my IDE automatically sets up
#pragma once. After frustration and sleepiness I just left #pragma once with an intention to fix it later. If anyone has any thoughts on why the compiler generates errors that say my class object is undefined I would appreciate it.Player Class:
#pragma once
#include
#include
class board;
class Player
{
char playermark;
public:
char mark() const;
void setmark(bool &, Player &);
void turn(board &);
};Player cpp:
#include "Player.h"
#include "board.h"
//returns the mark for current player i.e X or O
char Player::mark() const{
return playermark;
}
//sets the mark of each player i.e x or o
//uses markpass to tell game loop that correct inputs have been chosen
void Player::setmark(bool &markpass, Player &p2) {
char tempmark; //hold input to select piece
while (!markpass) {
std::cin >> tempmark;
if (tempmark == 'x' || tempmark == 'X' || tempmark == 'o' || tempmark == 'O') {
playermark = tempmark;
markpass = true;
}
else {
std::cout > tempposition) {
game1.markboard(tempposition, playermark, inputpass); // goes to markboard function
}
//if the input type fails
else {
std::cout ::max(), '\n');
}
}
}board class:
#pragma once
#include
#include
class board
{
std::string position = "123456789";
public:
void drawboard();
void clearboard();
void markboard(const size_t &, const char &, bool &);
char checkwin(bool&,int&);
};board cpp:
```
#include "board.h"
//outputs the board onto the screen
void board::drawboard() {
std::cout 0 && boardIndex = 5; i++, k--) {
Solution
I see some things that may help you improve your program.
Fix the typos
The word "turn" is misspelled "trun" in two places within
All control paths should return something
Within the
Don't use references for primitive types
Passing a reference to a
Consider an alternative design
Right now, the responsibilities of the objects are somewhat diffuse. The
See also Tic-Tac-Toe in C++11 - follow-up 2 for more detail on how this might be done.
Use constructors
Right now the
Reconsider function and class names
The name
Eliminate "magic numbers"
This code is littered with "magic numbers," that is, unnamed constants such as 5, 8, 9, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "9" and then trying to determine if this particular 9 means the number of squares or some other constant that happens to have the same value.
Perform better error checking for user input
As it is currently written, one can type in "66" for the square number and the code proceeds to proces that input as though it were a valid square within
Fix the typos
The word "turn" is misspelled "trun" in two places within
gamePlay(). That's not really a code error, but typos in the user interface tend to cause users of the program to wonder what else was done wrong and generally gives a bad impression. That should be fixed.All control paths should return something
Within the
board::checkwin() routine, it returns either the winning piece or 'C' or... nothing at all. The latter choice is a problem. Every control path for non-void functions should return something even if it's just a dummy value such as 0.Don't use references for primitive types
Passing a reference to a
std::string makes sense because it allows the compiler to avoid making a copy of the string, but passing a reference to a primitive type, such as bool as in the case of board::checkwin() does not make much sense. A bool is likely smaller than a pointer to bool, so it doesn't gain anything there. What the code is attempting to do is to allow the variable to be altered. Instead, I'd suggest altering the interface so that non-const references to primitive types are minimized or eliminated. See the next suggestion.Consider an alternative design
Right now, the responsibilities of the objects are somewhat diffuse. The
board object, for example, doesn't actually manage the play of the game, and the Player objects need to know about each other to be created. Also, I/O is done in all classes and also in main. That's not a very clean design. Consider instead isolating I/O to a single module and using something more like the Model-View-Controller design pattern.See also Tic-Tac-Toe in C++11 - follow-up 2 for more detail on how this might be done.
Use constructors
Right now the
board object doesn't have a real constructor. Instead, the clearboard() routine is used. I'd suggest that instead, there should be a constructor that initializes the object. The clearboard function is only used once, and that's just before the destructor is about to be called anyway!Reconsider function and class names
The name
drawboard would probably be better named draw because the "board" part is already part of the class. Also, the naming of board with lowercase 'b' but Player with uppercase 'P' seems a little inconsistent.Eliminate "magic numbers"
This code is littered with "magic numbers," that is, unnamed constants such as 5, 8, 9, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "9" and then trying to determine if this particular 9 means the number of squares or some other constant that happens to have the same value.
Perform better error checking for user input
As it is currently written, one can type in "66" for the square number and the code proceeds to proces that input as though it were a valid square within
markboard, producing invalid results at best or a crash at worst. Better would be to include more checking to make sure the value is in range before using it as an index into the string.Context
StackExchange Code Review Q#125206, answer score: 6
Revisions (0)
No revisions yet.