HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinorCanonical

Tic Tac Toe C++ with classes

Submitted by: @import:stackexchange-codereview··
0
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 #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 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.