6
\$\begingroup\$

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 <iostream>
#include <limits>

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 << "Incorrect input\nTry again: ";
        }
    }
    //sets player 2's piece, I wanted a lowercase for a lower case and an upper for an upper
    switch (playermark) {
    case 'X': p2.playermark = 'O';
        break;
    case 'x': p2.playermark = 'o';
        break;
    case 'O': p2.playermark = 'X';
        break;
    case 'o': p2.playermark = 'x';
        break;
    }
}

//player turns
void Player::turn(board &game1){
    size_t tempposition = 0; //holds input for space player wishes to mark
    bool inputpass = false; //used to check if the input is passed, failed, or not marked
    while (!inputpass) {
        //tests input type
        if (std::cin >> tempposition) {
            game1.markboard(tempposition, playermark, inputpass); // goes to markboard function
        }
        //if the input type fails
        else {
            std::cout << "INCORRECT INPUT TYPE\nTry agian:";
            std::cin.clear(); //clears input fail state
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
    }
}

board class:

#pragma once

#include <iostream>
#include <string>

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 << " " << position[0] << " |" << " " << position[1] << " |" << " " << position[2] << "\n";
    std::cout << "___|___|___ \n";
    std::cout << " " << position[3] << " |" << " " << position[4] << " |" << " " << position[5] << "\n";
    std::cout << "___|___|___ \n";
    std::cout << " " << position[6] << " |" << " " << position[7] << " |" << " " << position[8] << "\n";
    std::cout << "   |   |   ";
}

//clears all user input from the board
void board::clearboard() {
    position = "123456789";
}

//marks board with x or o in proper space
//boardIndex is position given by user
void board::markboard(const size_t &boardIndex, const char &playermark, bool &inputpass) {
    char check = position[boardIndex - 1];
    //if check is used to determine if currently selected position is marked or not
    if (check != 'X' && check != 'x' && check != 'O' && check != 'o') {
        //tests if space choice is within domain of the board
        if (boardIndex > 0 && boardIndex <= 9) {
            position[boardIndex - 1] = playermark;
            inputpass = true; //'Passes' input so turn is over, otherwise loops through turn again
        }
        else {
            //entered the incorrect position
            std::cout << "INCORRECT SPACE CHOICE\nTry again: ";
        }
    }
    else {
        //entered position that has previously been marked
        std::cout << "SPACE HAS ALREADY BEEN MARKED\nTry again: ";
    }
}

//wrote down every scenario, found two patterns (it was this or 8 if else statements *I thought this was more creative*)
char board::checkwin(bool& gamewin, int &turncnt) {
    for (size_t i = 0, k = 8; i <= 3 && k >= 5; i++, k--) {
        if (position[4] == position[i] && position[i] == position[k]) {
            gamewin = true; //sets game condition to true, game has been won
            return position[4]; //returns winning 'piece' (X or O) to test who won
        }
    }
    int cnt = 0;
    size_t initial = 2;
    //second pattern
    for (size_t i = 1, k = 0; i <= 7 && k <= 9; i += 4, k += 8) {
        if (cnt == 1) {
            i -= 2; k -= 8;
            initial += 4;
        }
        if (position[initial] == position[i] && position[i] == position[k]) {
            gamewin = true;
            return position[initial];
        }
        cnt++;
    }
    //checks for cats game, must be greater than or equal to eight  to return C after game loop has ended(with no winner)
    if (turncnt >= 8) {
        gamewin = true;
        return 'C'; //retruns C for cats game
    }
}

Main cpp:

#include <iostream>
#include <limits>
#include "board.h"
#include "Player.h"

int gamePlay();
void pause();

int main() {

    std::cout << "Welcome to Tic Tac Toe!\n\n";
    gamePlay();
    std::cout << "\n";
    pause();
}

int gamePlay() {
    board game1;
    Player p1, p2;
    bool markpass = false; //checks wether players have selected their pieces
    bool gamewin = false; //sets condition for game
    int turncnt = 0; //counts number of turns

    game1.drawboard(); //draws initial board with numbered spaces
    while (!gamewin) {
        if (!markpass) {
            std::cout << "\nWhat team do you want: X or O?\nPLayer 1: ";
            p1.setmark(markpass, p2); //sets piece for player 1 and 2
            std::cout << "Player 2: " << p2.mark() << "\n\n";
        }

        //turn for player 1
        if (!(turncnt % 2)) {
            std::cout << "Player 1 trun:\nSQUARE: ";
            p1.turn(game1);
            game1.checkwin(gamewin, turncnt);
        }
        //turn for player 2
        else {
            std::cout << "Player 2 trun:\nSQUARE: ";
            p2.turn(game1);
            game1.checkwin(gamewin, turncnt);
        }

        game1.drawboard(); // update gameboard
        std::cout << "\n";
        turncnt++; // increment turn count
    }


    std::cout << "GAME OVER!!!!!!!\n\n";
    //check cats game if none, conditional is used to determine who won
    if (!(game1.checkwin(gamewin, turncnt) == 'C')) {
        std::cout << "WINNER: Player "
            << ((game1.checkwin(gamewin, turncnt) == p1.mark()) ? "1" : "2");
    }
    else {
        std::cout << "Cats game!";
    }
    game1.clearboard();//resest board
    return 0;
}

//used to pause and see results
void pause() {
    char quit;
    std::cout << "Press any key followed by enter to quit";
    std::cin >> quit;
}
\$\endgroup\$
  • \$\begingroup\$ Don't change the code here, but you may want to correct the spelling error in you output in gamePlay(). Especially since the comments have the correct spelling. \$\endgroup\$ – pacmaninbw Apr 9 '16 at 11:52
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer! I know you have told me before about magic numbers and i apologize for using them again, that was merely me trying to see if a could actually write a game that works vs checking it over. Next time i will go over my code several times before posting to eliminate those errors because they shouldn't be there. The spelling mistakes were due to me wanting to wright code, but having work and school all day restricting me to late at night. Any way, what could i call the classes under model view and control, would i for example use board as a view? \$\endgroup\$ – chris360 Apr 9 '16 at 17:43
  • \$\begingroup\$ sorry for the second comment, I just want to see if i'm thinking about this correctly, would view be board, but i separate all the logic from it? And control would house all my I/O? Oh and last question, maybe a little to general for here, but between my address book and this project have you seen improvement? Thanks for your reply! \$\endgroup\$ – chris360 Apr 9 '16 at 17:46
  • \$\begingroup\$ I agree with most of your suggestions with the exception of returning a value from every method, regardless of its function. Is that commonplace for c style programming? I come from a java background and make heavy use of void in my method signatures. \$\endgroup\$ – Nick Savage Apr 9 '16 at 17:48
  • \$\begingroup\$ @NickSavage I didn't suggest returning a value from every method. What I said is that if a function is declared to return a value, it should always return a value. \$\endgroup\$ – Edward Apr 9 '16 at 17:51
  • 1
    \$\begingroup\$ @NickSavage: I've reworded it to try to make that point more clear. \$\endgroup\$ – Edward Apr 9 '16 at 17:58
3
\$\begingroup\$

If anyone has any thoughts on why the compiler generates errors that say my class object is undefined I would appreciate it.

It may be because you don't have constructors for Player and board. If you want the compiler to generate the constructors and destructors you can add the following lines to your class definitions:

board.h

    board() = default;
    virtual ~board() = default;

Player.h

    Player() = default;
    virtual ~Player() = default;

Please note that in my compiler this exposes a warning message in Player.h that playermark is uninitialized.

The class naming is inconsistent, board is all lowercase Player is capitalized.

Constants - No Magic Numbers

You have a lot of raw numbers in your code, these are sometimes referred to as Magic Numbers, to make your code more readable as well as making code easier to modify if the same number is used in multiple places in the code you should use symbolic constants.

    for (size_t i = 1, k = 0; i <= 7 && k <= 9; i += 4, k += 8) {
        if (cnt == 1) {
            i -= 2; k -= 8;
            initial += 4;
        }
        if (position[initial] == position[i] && position[i] == position[k]) {
            gamewin = true;
            return position[initial];
        }
        cnt++;
    }

Might be

static const int BOARD_SIZE = 9;
static const int NAME_ME1 = 8; // Not sure what 8 actually represents.

    for (size_t i = 1, k = 0; i <= 7 && k <= BOARD_SIZE; i += 4, k += NAME_ME1) {
        if (cnt == 1) {
            i -= 2; k -= 8;
            initial += 4;
        }
        if (position[initial] == position[i] && position[i] == position[k]) {
            gamewin = true;
            return position[initial];
        }
        cnt++;
    }

Meaningful Variable Name: The variables i and k don't tell the reader anything about what the code is doing, initial and cnt are better, but spell out count. The variable gamewin is very clear, but use camelCase, gamewin => gameWin.

Meaningful Comments: You don't need anywhere near the number of comments you have, the code itself should clearly explain what it is doing. Comments should explain the why, not the what. The best comment in the code is

//wrote down every scenario, found two patterns (it was this or 8 if else statements *I thought this was more creative*)

but it would be better to explain what the two patters are.

The code here is pretty clear, you don't need the comment:

    //if check is used to determine if currently selected position is marked or not
    if (check != 'X' && check != 'x' && check != 'O' && check != 'o') {
\$\endgroup\$
  • \$\begingroup\$ A few days ago I found a C, C++ or C# question that specifically asked the community to comment on the OP's comments. An established and respected member of the community provided a great answer and I would have liked to use that question as a reference to this question for comments. Could someone point it out to me so I can add it to my favorites? \$\endgroup\$ – pacmaninbw Apr 9 '16 at 13:35
  • \$\begingroup\$ Thank you for your response! And also for solving that compiler error! I'll go through an correct my code and possibly re-post it in the future. Btw, the spelling errors we're because I was so tired but wanted to finish the code! Haha Any way thanks for your response! \$\endgroup\$ – chris360 Apr 9 '16 at 17:50
0
\$\begingroup\$
for (size_t i = 1, k = 0; i <= 7 && k <= 9; i += 4, k += 8) {
    if (cnt == 1) {
        i -= 2; k -= 8;
        initial += 4;
    }
    if (position[initial] == position[i] && position[i] == position[k]) {
        gamewin = true;
        return position[initial];
    }
    cnt++;
}

There is an error which skips the second iteration. That means, it wont be able to check position[2] == position[5] == position[8].

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.