3
\$\begingroup\$

I made some changes to my class structure and the way I determine a win. I was attempting to decide if it made sense to keep the player completely isolated to TicTacToe game class or if it made sense to make a player class to add additional functionality.

So that you can see the progress of the code my initial implementation without classes can be found here and my first implementation with classes can be found here.

I decided to separate out the bool getUserWantToPlay() function from the game class simply because the user could cin a negative answer and avoid creating an instance of the TicTacToe class all together. Is that proper thinking?

Ideally from this review, I would like to receive further advice on class implementation as well as recommendations on how classes should interact with each other. Improving my code with built in functions and clear concise syntax would also be great.

Main

//implementation of TicTacToe
//Using classes this time

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

bool getUserWantToPlay();

int main()
{

    //Assumes no play unless user decides they want to play and initializes game variable to TicTacToe class
    bool play = false;

    play = getUserWantToPlay();

    //allows for multiple games to be played
    while(play == true)
    {

        TicTacToe game;
        char playerWinner = 'n';
        char player = 'X';

        //single game iteration
        while(playerWinner == 'n')
        {

            game.drawBoard();
            game.getPlayerMove(player);
            playerWinner = game.checkForWin(player);

            if(playerWinner == 'n')
            {
                player = game.togglePlayer(player);
            }
        }

        game.drawBoard();

        game = TicTacToe();

        play = getUserWantToPlay();

    }

    return(0);
}

Class Header

/*
 * TicTacToeClass.h
 *
 *  Created on: Jun 15, 2016
 *     
 */

#ifndef TICTACTOECLASS_H_
#define TICTACTOECLASS_H_


class TicTacToe
{

    public:
        TicTacToe();
        void drawBoard();
        void getPlayerMove(char player);
        char togglePlayer(char player);
        char checkForWin(char player);


    private:
        char squareArray[9];
        int moveCounter;
};

#endif /* TICTACTOECLASS_H_ */

Class Implementation

//TicTacToe class implementation
//Leeroy Jenkins

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


TicTacToe::TicTacToe()
: squareArray{'1','2','3','4','5','6','7','8','9'},
  moveCounter(0)
{
}

bool getUserWantToPlay()
{

    char response;
    bool invalidResponse = true;
    bool play = false;

    while(invalidResponse == true)
    {

        std::cout << "Would you like to play a new game of TicTacToe? (y/n) " << std::endl;
        std::cin >> response;

        if(response == 'y')
        {
            invalidResponse = false;

            play = true;
        }
        else if(response == 'n')
        {
            std::cout << "No Problem!";
            invalidResponse = false;
        }
        else
        {
            std::cout << "Please input a proper response (y/n)" << std::endl;
        }

    }

    return play;
}
void TicTacToe::drawBoard()
{

    //draws the game board with updated characters for each player

        std::cout << "Player 1 (X) - Player 2 (O)" << std::endl << std::endl << std::endl;

        std::cout << "    |     |" << std::endl;
        std::cout << " " << squareArray[0] << "  |  " << squareArray[1] << "  |  " << squareArray[2] << std::endl;

        std::cout << "____|_____|____" << std::endl;
        std::cout << "    |     |    " << std::endl;

        std::cout << " " << squareArray[3] << "  |  " << squareArray[4] << "  |  " << squareArray[5] << std::endl;

        std::cout << "____|_____|____" << std::endl;
        std::cout << "    |     |    " << std::endl;

        std::cout << " " << squareArray[6] << "  |  " << squareArray[7] << "  |  " << squareArray[8] << std::endl;

}

void TicTacToe::getPlayerMove(char player)
{
    //Gets player move and stores in board array for display through next iteration

    bool playerMoveFound = false;
    char playerTurn = '0';
    char playerMove = '0';

    if(player == 'X')
    {
        playerTurn = '1';
    }
    else
    {
        playerTurn = '2';
    }


    while(playerMoveFound == false)
    {
        std::cout << "Player " << playerTurn << " please make a move" << std::endl;
        std::cin >> playerMove;

        for(int counter = 0; counter < 9; counter++)
        {
            //If finds the array number makes the change to the iteration...prevents x or o movement
            if(playerMove == squareArray[counter] && playerMove != 'X' && playerMove != 'O' && playerMove != 'x' && playerMove != 'o')
            {
                squareArray[counter] = player;

                playerMoveFound = true;

                //Move counter implemented to streamline check for win/check for tie
                moveCounter++;
            }

        }
        if(playerMoveFound == false)
        {
            std::cout << "Invalid player move..." << std::endl;
        }
    }
}

char TicTacToe::checkForWin(char player)
{
    char playerWin = 'n';

    //Ignores iteration if move counter is less than 5 since win/tie impossible until that point
    if(moveCounter >= 5)
    {

        //Test rows for win
        for(int rowCounter = 0; rowCounter < 8; rowCounter+=2)
        {

            if(squareArray[rowCounter] == squareArray[rowCounter + 1] && squareArray[rowCounter + 1] == squareArray[rowCounter + 2])
            {

                playerWin = player;
            }
        }

        //Tests columns for win
        for(int columnCounter = 0; columnCounter < 3; columnCounter++)
        {

            if(squareArray[columnCounter] == squareArray[columnCounter + 3] && squareArray[columnCounter + 3] == squareArray[columnCounter + 6])
            {
                playerWin = player;
            }
        }

        //Tests both diagnols
        if(squareArray[0] == squareArray[4] && squareArray[4] == squareArray[8])
        {

            playerWin = player;

        }
        else if(squareArray[2] == squareArray[4] && squareArray[4] == squareArray[6])
        {
            playerWin = player;
        }

        //Checks for Tie
        if(moveCounter == 9 && playerWin != player)
        {
            playerWin = 't';
        }


        if(playerWin == player)
        {
            std::cout << std::endl << "Congratulations player " << player << "! You Win!" << std::endl;
        }
        else if(playerWin == 't')
        {
            std::cout << "Tie!  You should play again to settle the duel!" << std::endl;
        }

    }

    return(playerWin);
}

char TicTacToe::togglePlayer(char player)
{

    player = player == 'X' ? 'O':'X';

    return(player);

}
\$\endgroup\$
1
\$\begingroup\$

File Names

File names usually don't include the 'class' postfix, you just name the file after the class contained, i.e. 'TickTackToe.h', not 'TickTackToeClass.h'.

Classless Functions

You wouldn't usually have non-class functions, possibly unless they are friend functions, in the implementation file for the class. So, getUserWantsToPlay should be moved.

Bug

When checking for row wins, should be incrementing rowCounter by 3, not by 2

for(int rowCounter = 0; rowCounter < 8; rowCounter+=3)

Structure

With your current code, it feels like the main method has too much of an orchestration role for your class. If the methods are called out of order, or the wrong player is passed in, then the game wouldn't function as expected. One way to get around this is to make a lot of the methods private expose only methods that a client would need to call as public.

I'd also suggest introducing some helper methods to the class to help reduce some of it's complexity and make it easier to follow the logic. You may also want to consider adding some constants for some of the magic numbers (such as 5 for minimum moves needed before you need to check for a winner.

To help illustrate some of the above, I've done some refactoring on your code (below). This isn't a complete job, just moves it a couple of steps forward.

Main.cpp

#include "TicTacToe.h"

bool getUserWantToPlay();

int main()
{
    //Assumes no play unless user decides they want to play and initializes game variable to TicTacToe class
    bool play = false;

    //allows for multiple games to be played
    while (getUserWantToPlay())
    {
        TicTacToe game;

        game.playGame();
    }

    return(0);
}

bool getUserWantToPlay()
{

    char response;
    bool invalidResponse = true;
    bool play = false;

    while (invalidResponse == true)
    {

        std::cout << "Would you like to play a new game of TicTacToe? (y/n) " << std::endl;
        std::cin >> response;

        if (response == 'y')
        {
            invalidResponse = false;

            play = true;
        }
        else if (response == 'n')
        {
            std::cout << "No Problem!";
            invalidResponse = false;
        }
        else
        {
            std::cout << "Please input a proper response (y/n)" << std::endl;
        }

    }

    return play;
}

TickTackToe.h

#ifndef TICTACTOE_H_
#define TICTACTOE_H_


class TicTacToe
{

public:
    TicTacToe();

    void playGame();

private:
    void drawBoard();
    void getPlayerMove(char player);
    char togglePlayer(char player);

    bool checkForWin(char player);
    bool checkLineForWin(int square1, int square2, int square3, char player);
    bool checkForTie();

    char squareArray[9];
    int moveCounter;
};

#endif /* TICTACTOE_H_ */

TicTacToe.cpp

#include "TicTacToe.h"

static const int MinMovesNeededToWin = 5;
static const int MaxMovesInGame = 9;

TicTacToe::TicTacToe()
    : squareArray{ '1','2','3','4','5','6','7','8','9' },
    moveCounter(0)
{
}


void TicTacToe::drawBoard()
{

    //draws the game board with updated characters for each player

    std::cout << "Player 1 (X) - Player 2 (O)" << std::endl << std::endl << std::endl;

    std::cout << "    |     |" << std::endl;
    std::cout << " " << squareArray[0] << "  |  " << squareArray[1] << "  |  " << squareArray[2] << std::endl;

    std::cout << "____|_____|____" << std::endl;
    std::cout << "    |     |    " << std::endl;

    std::cout << " " << squareArray[3] << "  |  " << squareArray[4] << "  |  " << squareArray[5] << std::endl;

    std::cout << "____|_____|____" << std::endl;
    std::cout << "    |     |    " << std::endl;

    std::cout << " " << squareArray[6] << "  |  " << squareArray[7] << "  |  " << squareArray[8] << std::endl;

}

void TicTacToe::getPlayerMove(char player)
{
    //Gets player move and stores in board array for display through next iteration

    bool playerMoveFound = false;
    char playerTurn = '0';
    char playerMove = '0';

    if (player == 'X')
    {
        playerTurn = '1';
    }
    else
    {
        playerTurn = '2';
    }


    while (playerMoveFound == false)
    {
        std::cout << "Player " << playerTurn << " please make a move" << std::endl;
        std::cin >> playerMove;

        for (int counter = 0; counter < 9; counter++)
        {
            //If finds the array number makes the change to the iteration...prevents x or o movement
            if (playerMove == squareArray[counter] && playerMove != 'X' && playerMove != 'O')
            {
                squareArray[counter] = player;

                playerMoveFound = true;

                //Move counter implemented to streamline check for win/check for tie
                moveCounter++;
            }

        }
        if (playerMoveFound == false)
        {
            std::cout << "Invalid player move..." << std::endl;
        }
    }
}

bool TicTacToe::checkLineForWin(int square1, int square2, int square3, char player) {
    return squareArray[square1] == player &&
           squareArray[square2] == player &&
           squareArray[square3] == player;
}

bool TicTacToe::checkForWin(char player)
{
    bool playerWin = false;

    if (moveCounter >= MinMovesNeededToWin)
    {
        // Only need to check for current player
        // Only need to check until we have a win

        for (int rowCounter = 0; !playerWin && rowCounter < 8; rowCounter += 3)
        {           
            playerWin = checkLineForWin(rowCounter, rowCounter + 1, rowCounter + 2, player);
        }

        //Tests columns for win
        for (int columnCounter = 0; !playerWin && columnCounter < 3; columnCounter++)
        {
            playerWin = checkLineForWin(columnCounter, columnCounter + 3, columnCounter + 6, player);
        }

        //Tests both diagnols
        playerWin |= (checkLineForWin(0, 4, 8, player) | checkLineForWin(2, 4, 6, player));
    }

    return playerWin;
}

bool TicTacToe::checkForTie() {
    return moveCounter == MaxMovesInGame;
}

void TicTacToe::playGame() {
    char playerWinner = 'n';
    char player = 'X';
    bool gameOver = false;

    //single game iteration
    do{
        drawBoard();
        getPlayerMove(player);

        if (checkForWin(player)) {
            std::cout << std::endl << "Congratulations player " << player << "! You Win!" << std::endl;
            gameOver = true;
        } else  if (checkForTie()) {
            std::cout << "Tie!  You should play again to settle the duel!" << std::endl;
            gameOver = true;
        }
        player = togglePlayer(player);
    } while (!gameOver);

    drawBoard();
}

char TicTacToe::togglePlayer(char player)
{

    player = player == 'X' ? 'O' : 'X';

    return(player);

}

Next Steps

As for next steps, you might want to consider the community challenge, which this month is snakes and ladders. If you extend it to make it playable it should give a few options for creating classes that interact with each other. There have been a few different submissions including one in C++.

\$\endgroup\$
  • \$\begingroup\$ So as a general rule of thumb; it is better to create an instance of a class to access a function rather than a stand alone function? Also, it is proper to drive a class from a public member function of that class? (I'm not questioning you; i'm just asking for clarification so I know for the future haha.) Thank you for all your help! \$\endgroup\$ – StormsEdge Jun 24 '16 at 12:35
  • 1
    \$\begingroup\$ @StormsEdge There's nothing wrong with stand alone functions, you just don't normally find them in source files that have class implementations in them. A compromise can be to have static member functions of the class. They are clearly tied to the class, so would belong with the rest of the code, but because they aren't tied to specific instances of the class they can be invoked without creating an instance. So, you could have: TicTacToe::getUserWantToPlay \$\endgroup\$ – forsvarir Jun 24 '16 at 13:21
  • 1
    \$\begingroup\$ @StormsEdge Classes are encapsulated bundles of functionality and the public interface defines what you can ask the class to do so you need to consider how that interface impacts the internal implementation. I made checkForWin private, because it doesn't make sense (in my head) for a client of TicTacToe to be able to ask the game to checkForWin('Z'), since Z isn't playing the game. You can design your class either way, however with a private function it's ok to assume you're being called correctly. With public functions, you need to validate that you are called correctly. \$\endgroup\$ – forsvarir Jun 24 '16 at 13:25
  • 1
    \$\begingroup\$ @StormsEdge That check needs to not only validate any arguments supplied, but also if necessary, that they are being called in the correct order. So, my checkForTie method has to be called after the checkForWin method, otherwise it doesn't work. This is ok because it's private, but wouldn't be ok if it was public. \$\endgroup\$ – forsvarir Jun 24 '16 at 13:27
0
\$\begingroup\$

code level comments

++ you have consistent naming style. Class design and code positioning could be improved (as per previous answers). ++comments. ++Nothing too clever

(A debated topic) I would always recommend using a naming convention for member variables, this makes things much easier when reading code. YOu know if you are changing class data or local function data. Common conventions are 'm_' prefix or '_' suffix.

use enums. For example playerWin is either 't' or 'y' or 'n'. Its better to do

enum GameResult
{
   XWins,
   OWins,
   Tie
}

now its clear what return Tie means

invalidRespopnse in getUserWantToPlay is not used.

Compile the code with -wall -werror. Make it a matter of pride that your code compiles cleanly with those settings (whine about everything, every whine is fatal).

\$\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.