7
\$\begingroup\$

I have attempted to make the classic game Connect Four in C++, and I would love some feedback on this project. I have done everything myself without using the help of a tutorial or guide, so sorry if there are some bad practices here.

Main.cpp

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

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

#include <iostream>
#include <vector>

/*
AI
Search for empty blocks with block filled under
Make data structure with these blocks
randomly choose a block from that data structure to spawn on

*/

int main()
{
    Board board;
    Player player1, player2;
    GameLogic gameLogic;

    gameLogic.game(board, player1, player2, gameLogic);

    char c;
    std::cin >> c;
    return 0;
}

GameLogic.h

#pragma once

#include <string>

class Board;
class Player;
class GameLogic
{
private:
    char m_Turn = X;
    bool m_FoundWinner = false;
    char m_Winner = ' ';

    void allocateFirstTurns(Player& player1, Player& player2, char choice);
    void decideFirstTurn(Player& player1, Player& player2);
    void gameRound(Board& board, Player& player1, Player& player2, GameLogic& gameLogic);
    void changeTurn(char turn);

public:
    static const int ROWS = 9;
    static const int COLUMNS = 9;
    static const int WINNING_ROWS = 4;

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

    void game(Board& board, Player& player1, Player& player2, GameLogic& gamelogic);
    void setWinner(char gamePiece);
};

GameLogic.cpp

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


void GameLogic::allocateFirstTurns(Player & player1, Player & player2, char choice)
{
    if (choice == 'y')
    {
        player1.setGamePiece(X);
        player2.setGamePiece(O);
    }

    if (choice == 'n')
    {
        player1.setGamePiece(O);
        player2.setGamePiece(X);
    }
}

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

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

    allocateFirstTurns(player1, player2, choice);
}


void GameLogic::game(Board & board, Player& player1, Player& player2, GameLogic& gameLogic)
{

    char turn = X; //First turn
    decideFirstTurn(player1, player2);

    while (!m_FoundWinner)
    {
        gameRound(board, player1, player2, gameLogic);
    }

    board.displayBoard();

    //Announce winner

    std::cout << "Winner: " << m_Winner << "\n";

    if (m_Winner == player1.getGamePiece())
        std::cout << "Player 1 wins the game.\n";

    if (m_Winner == player2.getGamePiece())
        std::cout << "Player2 wins the game.\n";
}

void GameLogic::gameRound(Board & board, Player & player1, Player & player2, GameLogic& gameLogic)
{
    while (!m_FoundWinner)
    {
        if (m_Turn == player1.getGamePiece())
        {
            board.displayBoard();
            std::cout << "Player 1 move.";
            player1.move(board);
            m_FoundWinner = board.checkForWinner(gameLogic, player1.getGamePiece());
            changeTurn(player1.getGamePiece());
        }

        else
        {
            board.displayBoard();
            std::cout << "Player 2 move.";
            player2.move(board);
            m_FoundWinner = board.checkForWinner(gameLogic, player2.getGamePiece());
            changeTurn(player2.getGamePiece());
        }
    }
}

void GameLogic::changeTurn(char turn)
{
    if (turn == X)
        m_Turn = O;

    else
        m_Turn = X;
}

void GameLogic::setWinner(char gamePiece)
{
    m_Winner = gamePiece;
}

Board.h

#pragma once

#include <vector>
#include <iostream>


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

    std::string m_SrchHorizontal = "Horizontal";
    std::string m_SrchVertical = "Vertical";
    std::string m_SrchRightDiagonal = "DiagonalRight";
    std::string m_SrchLeftDiagonal = "DiagonalLeft";

    void initBoard();
    void searchForWinner(GameLogic& gameLogic, char playerPiece, std::string searchDirection, bool& foundWinner);

public:
    Board();

    char getBoardPosition(int row, int col) const;

    void displayBoard();
    bool isMoveLegal(int row, int col);
    void addPlayerPiece(int row, int col, char playerPiece);
    bool checkForWinner(GameLogic& gameLogic, char playerPiece);
};

Board.cpp

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

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

void Board::initBoard()
{
    std::vector<char> tempBoard;

    for (int i = 0; i < GameLogic::ROWS; i++)
    {
        tempBoard.push_back(GameLogic::EMPTY);
    }

    for (int i = 0; i < GameLogic::ROWS; i++)
    {
        m_Board.push_back(tempBoard);
    }

}

void Board::displayBoard()
{
    std::cout << "\n";
    for (int row = 0; row < GameLogic::ROWS - 1; row++)
    {

        std::cout << "\t"; //std::cout << "1 2 3 4 5 6";
        //std::cout << "\nrow";
        for (int col = 0; col < GameLogic::COLUMNS - 1; col++)
        {
            if(col != 0)
                std::cout << "|" << m_Board[row][col] << "|";
        }
        std::cout << "\n";
    }
}

bool Board::isMoveLegal(int row, int col)
{

    if (m_Board[row][col] == GameLogic::EMPTY) //If square player wnats to move to is empty
    {
        if (row == GameLogic::ROWS - 2 && m_Board[row][col] == GameLogic::EMPTY) //If square player wants to move to is on the bottom row.
        {
            return true;
        }

        else
        {
            int tempRow = row;
            tempRow++;
            if (m_Board[tempRow][col] != GameLogic::EMPTY)
            {
                return true;
            }

            else
            {
                std::cout << "You cannot move here.\n";
                return false;
            }

        }
    }

    else
    {
        std::cout << "You cannot move here.\n";
        return false;
    }
}

void Board::addPlayerPiece(int row, int col, char playerPiece)
{
    m_Board[row][col] = playerPiece;

}



char Board::getBoardPosition(int row, int col) const
{
    return m_Board[row][col];
}

bool Board::checkForWinner(GameLogic& gameLogic, char playerPiece)
{
    bool foundWinner = false;

    searchForWinner(gameLogic, playerPiece, m_SrchHorizontal, foundWinner);
    searchForWinner(gameLogic, playerPiece, m_SrchVertical, foundWinner);
    searchForWinner(gameLogic, playerPiece, m_SrchRightDiagonal, foundWinner);
    searchForWinner(gameLogic, playerPiece, m_SrchLeftDiagonal, foundWinner);

    return foundWinner;
}


void Board::searchForWinner(GameLogic& gameLogic, char playerPiece, std::string searchDirection, bool& foundWinner)
{
    if (!foundWinner)
    {
        int i = 0;
        for (int row = 0; row < GameLogic::ROWS; row++)
        {
            for (int col = 0; col < GameLogic::COLUMNS; col++)
            {
                while (m_Board[row][col] == playerPiece && !foundWinner)
                {
                    i++; //Counts blocks 
                    gameLogic.setWinner(playerPiece);

                    if (searchDirection == m_SrchHorizontal)
                        col++;

                    if (searchDirection == m_SrchVertical)
                        row++;

                    if (searchDirection == m_SrchRightDiagonal)
                    {
                        row++;
                        col++;
                    }

                    if (searchDirection == m_SrchLeftDiagonal)
                    {
                        row++;
                        col--;
                    }
                    if (i == GameLogic::WINNING_ROWS)
                    {
                        foundWinner = true;
                    }
                }
                i = 0;
            }
        }
    }
    else
    {
        return;
    }
}

Player.h

#pragma once

class GameLogic;
class Board;
class Player
{
private:
    char m_GamePiece = ' ';
    int m_Score = 0;

    int getRowPosition(const Board& board) const;
    int getColPosition(const Board& board) const;


public:

    void move(Board& board);
    void setGamePiece(const char gamePiece);
    char getGamePiece() const;
};

Player.cpp

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


int Player::getRowPosition(const Board& board) const
{
    int row = 0;
    bool positionAllowed = false;
    while (!positionAllowed)
    {
        std::cout << "Enter row.\n";
        std::cin >> row;

        if (row >= 1 && row < GameLogic::ROWS - 1)
            positionAllowed = true;
        else
            std::cout << "Position out of bounds. Please enter again.\n";
    }
    return row;
}

int Player::getColPosition(const Board& board) const
{
    int row = 0;
    bool positionAllowed = false;
    while (!positionAllowed)
    {
        std::cout << "Enter column.\n";
        std::cin >> row;

        if (row >= 1 && row < GameLogic::COLUMNS - 1)
            positionAllowed = true;
        else
            std::cout << "Position out of bounds. Please enter again.\n";
    }
    return row;
}


void Player::move(Board& board)
{
    int row = 0;
    int col = 0;
    bool moveComplete = false;
    while (!moveComplete)
    {
        row = getRowPosition(board);
        col = getColPosition(board);

        if (board.isMoveLegal(row, col))
        {
            board.addPlayerPiece(row, col, getGamePiece());
            moveComplete = true;
        }
    }

}

void Player::setGamePiece(const char gamePiece)
{
    m_GamePiece = gamePiece;
}

char Player::getGamePiece() const
{
    return m_GamePiece;
}
\$\endgroup\$
2
\$\begingroup\$

Overall, you've done a pretty good job. There's always room for improvement, though...

Compiling with extra warnings

It's always a good idea to compile with as strict a level of compiler warnings as you can. Given the usage of stdafx.h, you're likely using Visual Studio, so this'll be under C/C++ -> General -> Warning Level. I'm using GCC to compile everything, so this is simply -Wall -Wextra. This shows up a few things:

Player.cpp:6:5: warning: unused parameter ‘board’ [-Wunused-parameter]
int Player::getRowPosition(const Board& board) const
    ^

Player.cpp:23:5: warning: unused parameter ‘board’ [-Wunused-parameter]
int Player::getColPosition(const Board& board) const
    ^

GameLogic.cpp: In member function ‘void GameLogic::game(Board&, Player&, Player&, GameLogic&)’:
GameLogic.cpp:42:10: warning: unused variable ‘turn’ [-Wunused-variable]
    char turn = X; //First turn

These are pretty mild as far as warnings go - nothing that's actually dangerous. However, passing in a parameter that you don't use can be confusing for the person reading your code, so you should try and fix these up.

Stringly-typed functions and reference parameters

In your searchForWinner function, you passing in a std::string searchDirection, which is limited to a few different options. If you ever have a limited number of (distinct) options, an enum (or enum class) is a better choice:

enum class SearchDirection
{
    Horizontal, Vertical, DiagonalLeft, DiagonalRight
};

Passing a boolean reference parameter (bool& foundWinner) is also a bit of a code-smell. Much better would be to return a boolean. This would make your searchForWinner function look like:

bool searchForWinner(GameLogic& gameLogic, char playerPiece, SearchDirection searchDirection);

Your checkForWinner function could then look like:

bool Board::checkForWinner(GameLogic& gameLogic, char playerPiece)
{
    return searchForWinner(gameLogic, playerPiece, SearchDirection::Horizontal) ||
       searchForWinner(gameLogic, playerPiece, SearchDirection::Vertical) ||
       searchForWinner(gameLogic, playerPiece, SearchDirection::DiagonalRight) ||
       searchForWinner(gameLogic, playerPiece, SearchDirection::DiagonalLeft);
}

Code simplifications

initBoard() (which actually has a bug that just hasn't manifested itself because ROWS == COLUMNS currently in your code) can be simplified. std::vector has a constructor that takes a count and a value to initialize with:

void Board::initBoard()
{
    const static std::vector<char> row(GameLogic::COLUMNS, GameLogic::EMPTY);
    m_Board = std::vector<std::vector<char>>(GameLogic::ROWS, row);
}

You could even do away with this function entirely, and simply do this in the constructor via an initializer-list:

Board::Board()
    : m_Board(GameLogic::ROWS, std::vector<char>(GameLogic::COLUMNS, GameLogic::EMPTY))
{ }

Your getRowPosition and getColPosition have exactly the same code (including the variable being named row in both!). Whenever you have copy-paste code like this, you should refactor it into a different method:

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

        if (pos >= 1 && pos < size - 1)
            positionAllowed = true;
        else
            std::cout << "Position out of bounds. Please enter again.\n";
    }
    return pos;
}

You can then change getRowPosition and getColumnPosition to:

int Player::getRowPosition() const
{
    return getPosition("row", GameLogic::ROWS);
}

int Player::getColumnPosition() const
{
    return getPosition("column", GameLogic::COLUMNS);
}

There's a bit of confusion regarding the 0-based vs 1-based coordinates you are using. You try to correct for this initially (but only for rows?) with:

 if (row == GameLogic::ROWS - 2 && ...)

This looks far more complicated than need be. You've done your input checking in getRowPosition and getColPosition. I'd simply convert them to the equivalent 0-based array offsets (by subtracting 1), so move then looks like:

row = getRowPosition();
col = getColumnPosition();

if(board.isMoveLegal(row - 1, col - 1)) {
     ...
}
\$\endgroup\$
  • \$\begingroup\$ This is fantastic! It addressed everything that I was concerned about with my code and more. Thank you so much. \$\endgroup\$ – Ryan Swann Jan 25 '16 at 13:40
  • \$\begingroup\$ Hello there. After adding everything that you suggested it seems to me there is a bug within the code. When checking for a winner I am now returning true or false but it doesn't stay in the while loop when doing so. I am now returning true when 'i == winningRows' but this causes a bug that doesn't actually count every gamePiece available. So in essence; using a void function and passing as a bool ref works but returning as true doesn't. Thank you for your time. \$\endgroup\$ – Ryan Swann Jan 27 '16 at 21:15
  • \$\begingroup\$ Ah ha! I have solved the bug. I wasn't returning anything other than true and it caused my program to error in this way. \$\endgroup\$ – Ryan Swann Jan 27 '16 at 21:21

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.