Tic Tac Toe C++ MVC design

Question

This is my new version of my Tic Tac Toe game written in C++. I tried to follow community advice by separating I/O from the game logic and view (MCV). The code works great, and I am looking to see if I followed the model control view design pattern correctly.

TicTacToe.h

#ifndef TICTACTOE
#define TICTACTOE

#include<string>

class TicTacToe
{
    std::string boardInfo;
    char player1;
    char player2;
public:
    TicTacToe();
    ~TicTacToe();
    void markBoard(const size_t &, const char&);
    void setPlayerMark(const char &);
    char getMark(const int &);
    char checkWin(const int&);
    char positionValue(const size_t&);
};

#endif 

TicTacToe.cpp

#include "TicTacToe.h"

TicTacToe::TicTacToe()
{
    boardInfo = "123456789";
    player1 = 'X';
    player2 = 'O';
}

TicTacToe::~TicTacToe()
{
}

void TicTacToe::markBoard(const size_t & boardIndex, const char &playerMark) {
    boardInfo[boardIndex - 1] = playerMark;
}

// 'X' or 'O' = game win, 'C' = catsgame, and 'N' = no win
char TicTacToe::checkWin(const int &turnCount) {
    //winning solutions involving middle square
    for (size_t i = 0, k = 8; i <= 3 && k >= 5; i++, k--) {
        if (boardInfo[4] == boardInfo[i] && boardInfo[i] == boardInfo[k]) {
            return boardInfo[4];
        }
    }
    //remaining winning solutions
    if (boardInfo[2] == boardInfo[1] && boardInfo[1] == boardInfo[0]) { return boardInfo[2]; }
    if (boardInfo[2] == boardInfo[5] && boardInfo[5] == boardInfo[8]) { return boardInfo[2]; }
    if (boardInfo[6] == boardInfo[3] && boardInfo[3] == boardInfo[1]) { return boardInfo[6]; }
    if (boardInfo[6] == boardInfo[7] && boardInfo[7] == boardInfo[8]) { return boardInfo[6]; }

    //catsgame
    int totalTurns = 9; //total possible turns
    if (turnCount > totalTurns) { return 'C'; }

    return 'N';
}

void TicTacToe::setPlayerMark(const char &mark) {
    player1 = mark;
    //the other player will always receives opposite mark
    switch (mark) {
    case 'X': player2 = 'O';
        break;
    case 'x': player2 = 'o';
        break;
    case 'O': player2 = 'X';
        break;
    case 'o': player2 = 'x';
        break;
    }
}

char TicTacToe::positionValue(const size_t &index) {
    return boardInfo[index];
}

char TicTacToe::getMark(const int &player) {
    if (player == 1) {
        return player1;
    }
    if (player == 2) {
        return player2;
    }
    return 'N';
}

Game.h

#ifndef GAME
#define GAME

#include <iostream>
#include <limits>
#include "TicTacToe.h"

class Game
{
    TicTacToe board1;
public:
    Game();
    ~Game();
    void drawBoard();
    void turn(int &, TicTacToe &);
    void setMarks();
    void run();
};

#endif

Game.cpp

#include "Game.h"

Game::Game()
{
}


Game::~Game()
{
}

void Game::drawBoard() {
    std::cout << " " << board1.positionValue(0) << " |" << " " << board1.positionValue(1) << " |" << " " << board1.positionValue(2) << "\n";
    std::cout << "___|___|___ \n";
    std::cout << " " << board1.positionValue(3) << " |" << " " << board1.positionValue(4) << " |" << " " << board1.positionValue(5) << "\n";
    std::cout << "___|___|___ \n";
    std::cout << " " << board1.positionValue(6) << " |" << " " << board1.positionValue(7) << " |" << " " << board1.positionValue(8) << "\n";
    std::cout << "   |   |   ";
}

void Game::turn(int &turnCount, TicTacToe &board1) {
    char mark;
    size_t spaceChoice;
    if (turnCount % 2) {
        mark = board1.getMark(1); //player 1's mark
        turnCount++;
        std::cout << "\nPlayer 1's turn\nChoose space to mark: ";
    }
    else {
        mark = board1.getMark(2); //player 2's mark
        turnCount++;
        std::cout << "\nPlayer 2's turn\nChoose space 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 >> spaceChoice) {
            //checks domain
            if (spaceChoice > 0 && spaceChoice < 10) {
                char check = board1.positionValue(spaceChoice-1); //used to check if space has been marked
                if (check != 'X' && check != 'x' && check != 'O' && check != 'o') {
                    board1.markBoard(spaceChoice, mark);
                    inputpass = true;
                }
                else {
                    std::cout << "\nSPACE ALREADY MARKED\nTry again: ";
                }
            }
            else {
                std::cout << "\nINNCORRECT SPACE\nTry agian: ";
            }
        }
        //if the input type fails
        else {
            std::cout << "\nINCORRECT INPUT TYPE\nTry agian:";
            std::cin.clear(); //clears input fail state
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
    }
}

void Game::setMarks() {
    char tempMark;
    bool inputPass = false;
    while (!inputPass) {
        if (std::cin >> tempMark) {
            if (tempMark == 'X' || tempMark == 'x' || tempMark == 'O' || tempMark == 'o') {
                board1.setPlayerMark(tempMark);
                inputPass = true;
            }
            else {
            std::cout << "MARK MUST BE X OR O\nTry again: ";
            }
        }
        else {
            std::cout << "INCCORECT INPUT TYPE\nTry again: ";
        }
    }
 }

void Game::run() {
    std::cout << "Enter wether player 1 is 'X' or 'O': ";
    setMarks();
    char gameWin = 'N'; 
    int turnCount = 1;
    drawBoard();
    while (gameWin != 'X' && gameWin != 'x' && gameWin != 'O' && gameWin != 'o' && gameWin != 'C') {
        turn(turnCount, board1);
        gameWin = board1.checkWin(turnCount);
        drawBoard();
    }
    if (gameWin != 'C') {
        std::cout << ((board1.getMark(1) == gameWin) ? "\nThe winner is: Player 1!" : "\nThe winner is: Player 2!");
    }
    else
        std::cout << "\nCATS GAME!";
}

main.cpp

#include <iostream>
#include "Game.h"

void pause();

int main() {
    Game game1;
    game1.run();
    pause();
} 

void pause() {
    char end;
    std::cout << "\nPress any key followed by ENTER to continue...";
    std::cin >> end;
}

Answer 1

Good that you got around the #pragma once problem.

My IDE generates

#ifndef TICTACTOE_H_
#define TICTACTOE_H_

#endif /* TICTACTOE_H_ */

I thing that's a little more unique, possibly better.

MAGIC NUMBERS:

You still have a lot of magic numbers in your code:

    for (size_t i = 0, k = 8; i <= 3 && k >= 5; i++, k--) {
        if (boardInfo[4] == boardInfo[i] && boardInfo[i] == boardInfo[k]) {
            return boardInfo[4];
        }
    }
    //remaining winning solutions
    if (boardInfo[2] == boardInfo[1] && boardInfo[1] == boardInfo[0]) {
        return boardInfo[2];
    }
    if (boardInfo[2] == boardInfo[5] && boardInfo[5] == boardInfo[8]) {
        return boardInfo[2];
    }
    if (boardInfo[6] == boardInfo[3] && boardInfo[3] == boardInfo[1]) {
        return boardInfo[6];
    }
    if (boardInfo[6] == boardInfo[7] && boardInfo[7] == boardInfo[8]) {
        return boardInfo[6];
    }

    //catsgame
    int totalTurns = 9; //total possible turns
    if (turnCount > totalTurns) { return 'C'; }

In the public section of TicTacToe.h you should define

const int MAX_TURNS  = 9;

There are coding standards that apply to symbolic constant names, this link is one, but there are others.

It doesn't do as much good where it is defined in checkWin().

Code which may lead to possible problems:

Your switch statement doesn't have a default case, generally switch statements should contain a default case which handles all cases not specified and can be used to provide error messages.

    switch (mark) {
    case 'X': player2 = 'O';
        break;
    case 'x': player2 = 'o';
        break;
    case 'O': player2 = 'X';
        break;
    case 'o': player2 = 'x';
        break;
    default:
        std::cerr << "Please provide the correct input" << std::endl;
        break;
    }

Variable Names:

What are i and k in the following code?

    for (size_t i = 0, k = 8; i <= 3 && k >= 5; i++, k--) {
        if (boardInfo[4] == boardInfo[i] && boardInfo[i] == boardInfo[k]) {
            return boardInfo[4];
        }
    }

Fix the Typos:

Game.cpp: wether => whether, agian => again.

Model View Controller:

Generally a MVC design pattern has a set of classes for the Model, a set of classes for the view and a set of classes for the control. The classes in this implement contain elements of all 3.

What you really need to do is separate the logic of the game from the display mechanism. An example, Game contains both game control and display. The reason for this is that it allows your model classes to be reused on different operating systems using different display mechanisms (console text as you are now or graphics).

Amoung other things your controller should contain the code whether or not to play another game (remove pause() from Main.cpp, add playAnotherGame() to a controller class.

The controller class should call both the model and view classes. The view classes should only handle formatting the output, the model should have all the internal game functionality.

Let the Compiler do more work for you: In Game.h you have Game(); and ~Game(); and in Game.cpp you have

Game::Game()
{
}
Game::~Game()
{
}

Since you are not doing anything special in either the constructor or distructor you can use the default constructor and distructor if you are using C++11 or C++14. This allows you to declare/define your constructors only in your header file, and let the C++ compiler generate the proper constructors and destructors. The less code you have to write, the less error prone it is.

In C++11 or C++14

class Game
{
    TicTacToe board1;
public:
    Game() = default;
    ~Game() = default;
};

Functions in Main.cpp: If you reverse the order of pause() and main() in Main.cpp you don't need the function prototype of void pause(); prior to main(). Having main() as the last function in the file is fairly common for this reason.

In the past on certain operating systems, pause(); has been a system call. I would avoid it's use, although the system call did return int so the signature is different. It also isn't the most descriptive name you could give this function.

Answer 2

First, well done on improvements! It's much better than the previous version. I see some things that may help you further improve your code.

Cleanly separate implementation and interface

Mostly the separation between the two is clean, but there can be a bit more improvement. For instance, there's nothing in Game.h that really requires <iostream> or <limits>, so the only thing that really needs to be included there is "TicTacToe.h". Generally, the header file should only contain that which is necessary to understand the interface. Implementation details are best put in the corresponding .cpp file.

Let the compiler generate code where it can

The constructor and destructor for Game are empty and could have been created by the compiler. For such instances, it's usually better to let it do so when it can.

Use const where practical

The Game::drawBoard() function doesn't alter the underlying Game object, so it should be declared const. The same is true of at least a few other functions:

void Game::drawBoard() const;
char TicTacToe::getMark(const int &) const;
char TicTacToe::checkWin(const int&) const;
char TicTacToe::positionValue(const size_t&) const;

Fix the typos

The word "incorrect" is misspelled "INNCORRECT" and also "INCCORECT" (which is kind of amusing, really). That's not really a serious 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.

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 "4" and then trying to determine if this particular 4 means the number of the center square or some other constant that happens to have the same value.

Prefer generic over specific code

The checkWin() function has a lot of "magic numbers" that are both unnamed and mostly undocumented. Instead of coding like this:

//remaining winning solutions
if (boardInfo[2] == boardInfo[1] && boardInfo[1] == boardInfo[0]) { return boardInfo[2]; }
if (boardInfo[2] == boardInfo[5] && boardInfo[5] == boardInfo[8]) { return boardInfo[2]; }
if (boardInfo[6] == boardInfo[3] && boardInfo[3] == boardInfo[1]) { return boardInfo[6]; }
if (boardInfo[6] == boardInfo[7] && boardInfo[7] == boardInfo[8]) { return boardInfo[6]; }

Imagine instead that you have a named constant _dim which would be 3 for the usual three-by-three version of the game. Checking for row or column winners could then be written like this:

// check for row or column wins
for(unsigned i = 0; i < _dim; ++i){
bool rowwin = true;
bool colwin = true;
for (unsigned j=0; j < _dim; ++j) {
    rowwin &= boardInfo[i*_dim+j] == player;
    colwin &= boardInfo[j*_dim+i] == player;
}
if (colwin || rowwin) 
    return true;
}

Note, too that this assumes that player is passed into the function. We can do this easily, since it's clear that only the player that just made a move is the only one that could win in any particular turn. It therefore means that the function needs only to return true or false to answer the question, "was that the winning move?" The advantage here is that now we can easily change _dim to any other value and the logic of the game does not need to be touched -- it works as well with a 5x5 as with a 3x3 game. You can check for a tie game by simply moving the check to a separate function.