2
\$\begingroup\$

I have made a connect 4 console application in C++ and would love some feedback. In this project I have learned about inheritance and using virtual functions.

Main.cpp

// ConsoleApplication2.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "Player.h"
#include "humanPlayer.h"
#include "computerPlayer.h"
#include "GameLogic.h"
#include "Board.h"
#include "Turn.h"

#include <iostream>
#include <ctime>

bool isComputerInPlay()
{
    char choice = ' ';
    bool decided = false;
    while (!decided)
    {
        std::cout << "Would you like to play against a computer? 'y' - Yes. 'n' - No.";
        std::cin >> choice;

        if (choice == 'y' || choice == 'n') {
            decided = true;
        }
    }

    if (choice == 'y') {
        return true;
    }
    else {
        return false;
    }
}

int main()
{
    srand(time(NULL)); //Seed random. Only using on lite projects

    GameLogic gameLogic;
    Board board;
    Turn turn;

    Player* player1 = new humanPlayer;
    Player* player2 = nullptr;

    gameLogic.setComputerToPlay(isComputerInPlay());

    if(gameLogic.isComputerInplay())
    {
        player2 = new computerPlayer;
    }
    else
    {
        player2 = new humanPlayer;
    }

    //Begin game
    gameLogic.game(gameLogic, *player1, *player2, board, turn);

    return 0;
}

Board.h

#pragma once

#include <iostream>
#include <vector>

enum class Directions
{
    Horizontal,
    Vertical,
    RightDiagonal,
    LeftDiagonal
};

class GameLogic;
class Board
{
private:
    std::vector<std::vector<char>> m_board;

    void initBoard();

    bool searchForWinner(GameLogic& gameLogic, Directions direction, Board& board, char gamePiece);

public:
    Board();
    static const int ROWS = 9;
    static const int COLUMNS = 9;

    static const char X = 'X';
    static const char O = 'O';
    static const char EMPTY = ' ';

    char getPosition(int row, int col) const { return m_board[row][col]; }
    void addGamePiece(const int row, const int col, const char gamePiece) { m_board[row][col] = gamePiece; }

    bool isMoveLegal(const int row, const int col) const;

    void display();
    void clear();
    bool findWinner(GameLogic& gameLogic, Board& board, char gamePiece);
};

Board.cpp

#include "stdafx.h"
#include "Board.h"
#include "GameLogic.h"


Board::Board()
{
    initBoard();
}


void Board::initBoard()
{
    std::vector<char> rows(ROWS, EMPTY);
    m_board = std::vector<std::vector<char>>(COLUMNS, rows);
}


void Board::display()
{
    int rowText = 1;
    std::cout << "\n";
    std::cout << "\t  ";
    //Display The column Text
    for (int colText = 1; colText < COLUMNS - 1; colText++)
    {
        std::cout << colText << "  ";
    }
    std::cout << "\n";
    //Display the board
    for (int row = 1; row < ROWS - 1; row++)
    {
        std::cout << "\t";
        std::cout << rowText; //Display the row text
        rowText++;

        for (int col = 1; col < COLUMNS - 1; col++)
        {
            std::cout << "|" << m_board[row][col] << "|";
        }
        std::cout << "\n";
    }
}

bool Board::isMoveLegal(const int row, const int col) const
{
    std::vector<int> fullRow;
    if (m_board[row][col] == EMPTY)
    {

        if (row == ROWS - 2) //If requested move is on the bottom row
            return true;

        else
        {
            //Check to see whether or not the square underneath requested move is not empty
            int tempRow = row;
            tempRow++;
            if (getPosition(tempRow, col) != EMPTY)
                return true;

            else {
                return false;
            }

        }
    }

    else {
        return false;
    }


}

bool Board::searchForWinner(GameLogic& gameLogic, Directions direction, Board& board, char gamePiece)
{
    if (!gameLogic.isWinnerFound())
    {
        int i = 0;
        for (int row = 1; row < Board::ROWS - 1; row++)
        {
            for (int col = 1; col < Board::COLUMNS - 1; col++)
            {
                while (board.getPosition(row, col) == gamePiece && !gameLogic.isWinnerFound())
                {
                    i++;

                    if (direction == Directions::Horizontal) {
                        row++;
                    }

                    if (direction == Directions::Vertical) {
                        col++;
                    }

                    if (direction == Directions::LeftDiagonal) {
                        row++;
                        col--;
                    }

                    if (direction == Directions::RightDiagonal) {
                        row++;
                        col++;
                    }

                    if (i == GameLogic::WINNING_ROW) {
                        return true;
                    }
                }
                i = 0;
            }
        }
    }

    //If winner has already been found
    else if (gameLogic.isWinnerFound()) {
        return true;
    }

    return false;

}


bool Board::findWinner(GameLogic & gameLogic, Board & board, char gamePiece)
{
    return searchForWinner(gameLogic, Directions::Horizontal, board, gamePiece);
        searchForWinner(gameLogic, Directions::Vertical, board, gamePiece) ||
        searchForWinner(gameLogic, Directions::RightDiagonal, board, gamePiece) ||
        searchForWinner(gameLogic, Directions::LeftDiagonal, board, gamePiece);
}

void Board::clear()
{
    m_board.clear();
    initBoard();
}

GameLogic.h

#pragma once

class Turn;
class Player;
class Board;
class computerPlayer;
class GameLogic
{
private:
    bool m_computerInPlay = false;
    bool m_foundWinner = false;
    bool m_gameOver = false;

    void restartGame(Board& board, Turn& turn);

public:
    static const int WINNING_ROW = 4;

    void game(GameLogic& gameLogic, Player& player1, Player& player2, Board& board, Turn& turn);
    bool isComputerInplay() { return m_computerInPlay; }
    bool isWinnerFound() { return m_foundWinner; }
    void setComputerToPlay(bool inPlay);

    bool isGameOver() { return m_gameOver; }
    bool isRoundOver() { return m_foundWinner; }
    void promptRestart(Board& board, Turn& turn);
};

GameLogic.cpp

#include "stdafx.h"
#include "GameLogic.h"
#include "Board.h"
#include "Player.h"
#include "computerPlayer.h"
#include "Turn.h"


void GameLogic::game(GameLogic& gameLogic, Player & player1, Player & player2, Board & board, Turn& turn)
{
    while (!isGameOver())
    {
        turn.decideFirstTurn(player1, player2);
        board.display();

        while (!isRoundOver())
        {
            if (turn.getTurn() == player1.getGamePiece())
            {
                turn.announceTurn("Player 1");
                player1.takeTurn(board);
                board.display();
                m_foundWinner = board.findWinner(gameLogic, board, player1.getGamePiece());
                turn.changeTurn(turn.getTurn());
            }

            if (!m_foundWinner)
            {
                turn.announceTurn("Player 2");
                player2.takeTurn(board);
                board.display();
                m_foundWinner = board.findWinner(gameLogic, board, player2.getGamePiece());
                turn.changeTurn(turn.getTurn());
            }
        }
        promptRestart(board, turn);
    }

}


void GameLogic::setComputerToPlay(bool inPlay)
{
    if (inPlay) {
        m_computerInPlay = true;
    }
    else {
        m_computerInPlay = false;
    }
}

void GameLogic::promptRestart(Board& board, Turn& turn)
{
    bool decided = false;
    char responce = ' ';
    while (!decided)
    {
        std::cout << "\nWould you like to play again? 'y' - Yes. 'n' - No.";
        std::cin >> responce;

        if (responce == 'y' || responce == 'n') {
            decided = true;
        }
    }

    if (responce == 'n') {
        m_gameOver = true;
    }
    else {
        restartGame(board, turn);
    }
}

void GameLogic::restartGame(Board& board, Turn& turn)
{
    m_foundWinner = false;
    m_gameOver = false;
    turn.setFirstTurn();

    board.clear();
}

Turn.h

#pragma once

class Player;
class Board;
class Turn
{
private:
    char m_turn = ' ';

    void init();

public:
    Turn();
    ~Turn();

    void decideFirstTurn(Player& player1, Player& player2);
    void changeTurn(char turn);
    char getTurn() { return m_turn; }
    void announceTurn(std::string player) { std::cout << player << "'s turn.\n"; }
    void setFirstTurn() { m_turn = Board::X; }
};

Turn.cpp

#include "stdafx.h"
#include "Turn.h"
#include "Board.h"
#include "Player.h"


Turn::Turn()
{
    init();
}


Turn::~Turn()
{
}


void Turn::init()
{
    m_turn = Board::X;
}


void Turn::changeTurn(char turn)
{
    if (turn == Board::X) {
        m_turn = Board::O;
    }
    else {
        m_turn = Board::X;
    }
}


void Turn::decideFirstTurn(Player& player1, Player& player2)
{
    char responce;
    bool decided = false;
    while (!decided)
    {
        std::cout << "Would you like to go first? 'y' - Yes. 'n' - No.";
        std::cin >> responce;

        if (responce == 'y' || responce == 'n') {
            decided = true;
        }
    }

    if (responce == 'y') {
        player1.setGamePiece(Board::X);
        player2.setGamePiece(Board::O);
    }
    else {
        player2.setGamePiece(Board::X);
        player1.setGamePiece(Board::O);
    }

}

Player.h

#pragma once
#include <string>

class Board;
class GameLogic;
class Player
{
protected:
    int m_row = 0;
    int m_col = 0;
    std::string m_name;
    char m_gamePiece = ' ';

public:
    Player();

    virtual void takeTurn(Board& board) = 0;

    //Set
    void setRow(const int row) { m_row = row; }
    void setCol(const int col) { m_col = col; }
    void setName(std::string name) { m_name = name; }   
    void setGamePiece(const char gamePiece) { m_gamePiece = gamePiece; }

    //Get
    char getGamePiece() const { return m_gamePiece; }
    int getRow() const { return m_row; }
    int getCol() const { return m_col; }
    std::string getName() { return m_name; }

};

humanPlayer.h

#pragma once
#include "Player.h"

#include <iostream>

class humanPlayer :
    public Player
{
private:
    int getPosition(std::string direction, int size);
public:
    humanPlayer();
    ~humanPlayer();

    void takeTurn(Board& board);

};

humanPlayer.cpp

#include "stdafx.h"
#include "humanPlayer.h"
#include "Board.h"


humanPlayer::humanPlayer()
{
    std::cout << "Human activated.\n";
}


humanPlayer::~humanPlayer()
{
}



void humanPlayer::takeTurn(Board & board)
{
    //Get spawn position
    bool moveComplete = false;

    while (!moveComplete) 
    {
        m_row = getPosition("Row", Board::ROWS);
        m_col = getPosition("Column", Board::COLUMNS);

        if (board.isMoveLegal(m_row, m_col)) 
        {
            board.addGamePiece(m_row, m_col, m_gamePiece);
            moveComplete = true;
        }

        else {
            std::cout << "Move is not allowed.";
        }
    }

}

int humanPlayer::getPosition(std::string direction, int size)
{
    bool positionAllowed = false;
    int pos = 0;
    std::cout << "Enter " << direction << ".\n";
    while (!positionAllowed) 
    {
        std::cin >> pos;

        if (pos > 0 && pos < size - 1) {
            positionAllowed = true;
            std::cout << "Enter " << direction << ".\n";
        }
    }
    return pos;
}

computerPlayer.h

#pragma once
#include "Player.h"

#include <iostream>
#include <vector>
#include <string>




class GameLogic;
class Board;
class computerPlayer :
    public Player
{
private:
    //Stores all possible positons for computer to spawn on
    std::vector<int> m_rowPositions;
    std::vector<int> m_colPositions;
    int m_availableMoves = 0;

    void getPosition(Board& board);
    int getRandomPosition();

public:
    computerPlayer();

    void takeTurn(Board& board);


};

computerPlayer.cpp

#include "stdafx.h"
#include "computerPlayer.h"
#include "Board.h"
#include "GameLogic.h"


computerPlayer::computerPlayer()
{
    std::cout << "Computer activated.\n";
}


void computerPlayer::takeTurn(Board & board)
{
    bool moveComplete = false;
    while (!moveComplete)
    {
        getPosition(board);

        int spawnPos = getRandomPosition();

        m_row = m_rowPositions[spawnPos];
        m_col = m_colPositions[spawnPos];

        moveComplete = board.isMoveLegal(m_row, m_col);
    }

    board.addGamePiece(m_row, m_col, getGamePiece());

    //Reset all
    m_availableMoves = 0;
    m_rowPositions.clear();
    m_colPositions.clear();
}


void computerPlayer::getPosition(Board& board)
{
    for (int col = 1; col < Board::COLUMNS - 1; col++)
    {
        for (int row = Board::ROWS - 2; row >= 1; row--)
        {
            if (board.getPosition(row, col) == Board::EMPTY)
            {
                m_rowPositions.push_back(row);
                m_colPositions.push_back(col);
                m_availableMoves++;
                break; //Go to next column
            }
        }
    }
}

int computerPlayer::getRandomPosition()
{
    int randNumb = rand() % m_availableMoves;
    return randNumb;
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Overall the principle idea of using a Player class with a takeTurn virtual function that operates on a Board is a good one. But there's quite a lot of little issues/suggestions here...

Player* player1 = new humanPlayer;
Player* player2 = nullptr;
...
player2 = new computerPlayer;

Your Player objects are being stored in raw pointers and are not being deleted. It's not a problem here because your application ends when they should be deleted, but in general will lead to memory leaks. Consider using std::unique_ptr instead as it will automatically manage the object's lifetime.

You have a lot of input functions which all do approximately the same thing - prompt for a Yes/No answer. These can all be factored out into a single function (takes in the text prompt as a parameter).

Your main function is sharing the load of running the game with the GameLogic class. I think you could benefit from a clearer separation of concerns by introducing a new class, Game, which includes GameLogic, Board, Players, etc. All user input would be done by the Game class - GameLogic would be concerned with the "game logic" only, no user interface stuff. (In general we want as few classes interacting with the user as possible; helps with testing and reusability.)

I suggest ordering your class sections public, then protected, then private. The interface (public) is the most used section; the private section is like the class's dirty laundry and you want to hide it away at the bottom of the class declaration.

srand(time(NULL)); //Seed random. Only using on lite projects
int randNumb = rand() % m_availableMoves;

C-style rand is a bad habit to get into. The C++ random library might look intimidating but is actually easy to use in practice.

std::vector<std::vector<char>> m_board;

If the rows and columns are compile-time constants (which they are here), you can use std::array instead of std::vector.

bool isComputerInplay() { return m_computerInPlay; }
void setComputerToPlay(bool inPlay);

These 2 functions in GameLogic are breaking the point of using your Player class. Once the Player objects have been set there is no need for the GameLogic to know whether those players are computer or human players. In fact, GameLogic shouldn't even know that there's such a thing as a computer (or human) player.

Your Player class is missing a virtual destructor. A virtual destructor is needed for inheritance where you are storing the objects by a pointer to the base class.

//Set
void setRow(const int row) { m_row = row; }
void setCol(const int col) { m_col = col; }
void setName(std::string name) { m_name = name; }   
void setGamePiece(const char gamePiece) { m_gamePiece = gamePiece; }

//Get
char getGamePiece() const { return m_gamePiece; }
int getRow() const { return m_row; }
int getCol() const { return m_col; }
std::string getName() { return m_name; }

Getters and setters are normally a bad idea - might as well just make the variables public in that case. Other than set/getGamePiece, I don't believe the getters and setters are even used; just get rid of them. A player can't change their game piece in the middle of the game, so I would replace setGamePiece with passing it in with the Player constructor.

virtual void takeTurn(Board& board) = 0;

For a safer design, I don't think the Player should be allowed to modify the board. Instead I'd have this be:

virtual void getMove(const Board& board, int& row, int& col) = 0;

or

virtual std::pair<int, int> getMove(const Board& board) = 0;

GameLogic can then actually validate the move and make it on the Board. This keeps the "game logic" in the GameLogic class.

Finally, to your question about implementing a proper AI ... I'd strongly suggest starting with a simpler game than Connect-4 like Tic-Tac-Toe. If you persist, a starting point would be a minimax search.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for this, its great! One question; I thought having getters/setters was good because of the fact that it adds security to what you are attempting to change and receive. Surely making them public instead is worse the for program? \$\endgroup\$
    – Ryan Swann
    Feb 12, 2016 at 14:20
  • \$\begingroup\$ Exactly right - public variables are (normally) a bad idea. You don't want a class exposing its internal details for everybody wanting to use the class. Getters and Setters are basically the same thing with a thin layer of indirection. Sometimes you can't get around it - there's some property of your object which needs to be public information - like gamePiece here. But only expose those properties that are truly public information, and if they shouldn't be changed (like gamePiece I believe) then don't include a Setter. \$\endgroup\$
    – theosza
    Feb 12, 2016 at 16:11

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.