2
\$\begingroup\$

I have finished a battleship game, written in c++. I was hoping someone could review my code and tell me where I could improve. I have posted all of the code below, I hope that is okay. If not, and I can only post snippets, please let me know and I will delete the question. The options for gameplay are a human vs cpu and cpu vs cpu. Thank you!

main:

int main() {
    Game();
    return 0;
}

battleship.hpp:

#ifndef battleship_hpp
#define battleship_hpp

#include <iostream>
#include <cstring>
#include <cmath>
#include <time.h>
#include <stdio.h>
#include <unistd.h>
#include "Ship.h"
#include "Board.h"
#include "Player.h"
#include "Game.h"

const std::string border = "---------------------------------------------------------\n";
const std::string alphabet = "abcdefghijklmnopqrstuvwxyz";

Game.h:

#ifndef Game_h
#define Game_h

class Game {
public:
    Game();
    Player& getPlayer(int id);
    void fire(Player&, int, int); //attacks enemy ship
private:
    Player p1;
    Player p2;
};

#endif /* Game_h */

Game.cpp:

#include "battleship.hpp"

Player nullPlayer(-1);
const int speed1 = 0; //0 seconds
const int speed2 = 1; //1 second

Player& Game::getPlayer(int id) {
    if(id == 1) { return p1; }
    else if(id == 2) { return p2; }
    else {
        std::cout << "error getting player\n";
        return nullPlayer;
    }
}

void Game::fire(Player& playerBeingAttacked, int attackX, int attackY) {
    //referenced multiple times
    Ship* spaceBeingAttacked = &playerBeingAttacked(attackY, attackX);
    Ship* shipStoredInArray = &playerBeingAttacked.getBoard().getShip(playerBeingAttacked(attackY, attackX).getShipNumber());
    std::cout << border;
    //ensure valid target
    if(attackX >= 0 && attackX < BOARD_SIZE && attackY >= 0 && attackY < BOARD_SIZE) {
        //if already attacked, return error
        if(spaceBeingAttacked->getFiredUpon()) {
            std::cout << "error, already attacked\n";
            return;
        }
        //otherwise, attack
        else {
            spaceBeingAttacked->setFiredUpon(true);
            //sink ship, decrease board ships[] by 1
            if(spaceBeingAttacked->getStatus() == afloat) {
                spaceBeingAttacked->setStatus(sunk);
                //decrement ship in array
                shipStoredInArray->setSize(shipStoredInArray->getSize() - 1);
                std::cout << "HIT!\n";
                //sink board ship[] if all ship objects are sunk
                if(shipStoredInArray->getSize() == 0) {
                    shipStoredInArray->setStatus(sunk);
                    std::cout << "Player " << playerBeingAttacked.getID() << "'s " << shipStoredInArray->getName() << " SUNK!\n";
                }
            }
            else {
                std::cout << "MISS!\n";
            }
        }
    }
    else {
        std::cout << "invalid attack coordinate\n";
    }
}

//two gamemode's
//1 = human vs cpu
//2 = cpu vs cpu
Game::Game() {
    int gamemode;
    std::cout << border;
    std::cout << "Gamemodes:\n1. Player vs. CPU\n2. CPU vs. CPU\n";
    std::cout << border;
    std::cout << "Please select a gamemode (1/2):\n: ";
    std::cin >> gamemode;

    p1.setID(1);
    p2.setID(2);
    p1.setTurn(true);
    p2.getBoard().randomizeFleet();
    p2.setPlayerType("cpu");

    std::cout << "---------------------------------------------------------\nGAME STARTED\n";
    std::string startLoc, endLoc;
    int x1, y1, x2, y2, shipNumber;
    char choice;

    if(gamemode == 1) { p1.setPlayerType("human"); }
    else if(gamemode == 2) { p1.setPlayerType("cpu"); }
    else { std::cout << "invalid gamemode\n"; return; }

    //place if human
    if(p1.getPlayerType() == "human") {
        //place ships
        std::cout << "Player 1, please place your ships.\n";
        std::cout << border;
        //while all ships aren't placed
        //give option to randomize
        std::cout << "Would you like to place or randomize your fleet (p/r)?\n: ";
        std::cin >> choice;
        std::cout << border;
        if(choice != 'r') {
            while(!p1.getBoard().allShipsPlaced()) {
                //print reamining ships
                //print board
                std::cout << "p1's current board:\n";
                p1.getBoard().printBoard();
                p1.getBoard().printRemainingShips();

                std::cout << "Please enter ship number to place:\nShip Number: ";
                std::cin >> shipNumber;
                std::cout << "Please enter range to place ship:\nStart Location (ie: a3): ";
                std::cin >> startLoc;
                std::cout << "End Location (ie: a6): ";
                std::cin >> endLoc;

                x1 = std::stoi(startLoc.substr(0,1));
                y1 = std::stoi(startLoc.substr(1,1));
                x2 = std::stoi(endLoc.substr(0,1));
                y2 = std::stoi(endLoc.substr(1,1));

                //place ship
                p1.getBoard().setShip(shipNumber, x1, y1, x2, y2);
            }
        }
        else if(choice == 'r') { p1.getBoard().randomizeFleet(); }
        else { std::cout << "invalid option\n"; return; }
    }
    //computer player
    else {
        srand(static_cast<unsigned int>(time(NULL)));
        sleep(1);
        p1.getBoard().randomizeFleet();
        std::cout << "CPU Player: Ships Randomly Placed\n";
        std::cout << border;
    }

    Player currPlayer = p1;
    Player nextPlayer = p2;
    Player tmpPlayer;
    int option;
    std::string attackCoord;
    if(currPlayer.getPlayerType() == "human") {
        while(!(currPlayer.getBoard().allShipsSunk() || nextPlayer.getBoard().allShipsSunk())) {
            //take turns
            std::cout << "Player " << currPlayer.getID() << "'s turn:\n";
            std::cout << "Please select a command.......\n";
            std::cout << border;
            std::cout << "1. Print My Board\n2. Attack Enemy\n3. Print Enemy Board\n: ";
            std::cin >> option;

            if(option == 1) {
                std::cout << "Player " << currPlayer.getID() << "'s board\n";
                currPlayer.getBoard().printBoard();
            }
            else if(option == 2) {
                std::cout << border;
                std::cout << "Enter coordinate to attack (ie: a3)\n: ";
                std::cin >> attackCoord;
                x1 = (int)attackCoord[0] - 97;
                y1 = (int)attackCoord[1] - 48;
                if(x1 >= 0 && y1 >= 0 && x1 < BOARD_SIZE && y1 < BOARD_SIZE && !nextPlayer.getBoard()(x1, y1).getFiredUpon()) {
                    fire(nextPlayer, x1, y1);
                    std::cout << border;
                    //swap players if successful fire
                    tmpPlayer = currPlayer;
                    currPlayer = nextPlayer;
                    nextPlayer = tmpPlayer;
                }
                else {
                    std::cout << border;
                    std::cout << "invalid target, try again\n";
                    std::cout << border;
                }
            }
            else if(option == 3) {
                std::cout << border;
                std::cout << "Player " << nextPlayer.getID() << "'s board\n";
                nextPlayer.getBoard().printBoard();
            }
            else {
                std::cout << border;
                std::cout << "invalid option, try again.\n";
                std::cout << border;
            }
        }

        //the last player to guess will have won and they will have ben reassigned to nextPlayer
        std::cout << "PLAYER " << nextPlayer.getID() << " WON!!!!\nCONGRATULATIONS!!!\n";
        std::cout << "play again (y/n)?\n: ";
        char again = 'q';
        std::cin >> again;
        do {
            std::cout << border;
            if(again == 'y') { Game(); }
            else if(again == 'n') { return; }
            else {
                std::cout << "invalid selection, try again.\nplay again (y/n)?\n: ";
                std::cin >> again;
            }
        }while(again != 'y' || again != 'n');
    }
    //if player 1 is a cpu
    else {

        //initialize array of random numbers 0-99 to attack ships randomly
        int p1RandomCoords[BOARD_SIZE*BOARD_SIZE];
        int p2RandomCoords[BOARD_SIZE*BOARD_SIZE];
        int tmpRandNumber1, tmpRandNumber2, tmpNumber;

        for(int i = 0; i < BOARD_SIZE*BOARD_SIZE; i++) {
            p1RandomCoords[i] = i;
            p2RandomCoords[i] = i;
        }

        srand(static_cast<unsigned int>(time(NULL)));

        for(int k = 0; k < BOARD_SIZE*BOARD_SIZE*BOARD_SIZE; k++) {
            tmpRandNumber1 = rand()%100;
            tmpNumber = p1RandomCoords[tmpRandNumber1];
            p1RandomCoords[tmpRandNumber1] = p1RandomCoords[BOARD_SIZE*BOARD_SIZE - 1];
            p1RandomCoords[BOARD_SIZE*BOARD_SIZE - 1] = tmpNumber;

            tmpRandNumber2 = rand()%100;
            tmpNumber = p2RandomCoords[tmpRandNumber2];
            p2RandomCoords[tmpRandNumber2] = p2RandomCoords[BOARD_SIZE*BOARD_SIZE - 1];
            p2RandomCoords[BOARD_SIZE*BOARD_SIZE - 1] = tmpNumber;
        }

        int attackX, attackY;
        attackX = rand() % BOARD_SIZE;
        attackY = rand() % BOARD_SIZE;

        std::cout << "Player " << currPlayer.getID() << "'s board\n";
        std::cout << border;
        currPlayer.getBoard().printBoard();
        std::cout << "Player " << nextPlayer.getID() << "'s board\n";
        std::cout << border;
        nextPlayer.getBoard().printBoard();

        int i = 0;
        int k = 0;
        //until one of player's has all ships sunk
        while(!(currPlayer.getBoard().allShipsSunk() || nextPlayer.getBoard().allShipsSunk())) {
            //generate random attack that hasn't already been guessed
            //pick from random numbers p1
            if(currPlayer.getID() == 1) {
                attackX = p1RandomCoords[i] / 10;
                attackY = p1RandomCoords[i] % 10;
                i++;
            }
            else {
                attackX = p2RandomCoords[i] / 10;
                attackY = p2RandomCoords[i] % 10;
                k++;
            }
            //take turns
            std::cout << "Player " << currPlayer.getID() << "'s turn.....\n";
            //quick pause (as if cpu is thinking)
            sleep(speed1);
            std::cout << border;
            std::cout << "Attacking player " << nextPlayer.getID() << "\nCoordinate's being attacked: (" << attackX << ',' << attackY << ")\n";
            sleep(speed1);
            fire(nextPlayer, attackX, attackY);
            std::cout << "Attack Successful\n";
            std::cout << border;
            //print boards
            sleep(speed1);
            std::cout << "Printing player 1's board:\n";
            std::cout << border;
            sleep(speed2);
            //print player 1's board
            std::cout << "Player 1's board\n";
            std::cout << border;
            if(currPlayer.getID() == 1) { currPlayer.getBoard().printBoard(); }
            else { nextPlayer.getBoard().printBoard(); }
            std::cout << "Printing player 2's board:\n";
            std::cout << border;
            sleep(speed2);
            //print player 2's board
            std::cout << "Player 2's board\n";
            std::cout << border;
            if(currPlayer.getID() == 2) { currPlayer.getBoard().printBoard(); }
            else { nextPlayer.getBoard().printBoard(); }
            sleep(speed1);
            //swap players on successful fire
            tmpPlayer = currPlayer;
            currPlayer = nextPlayer;
            nextPlayer = tmpPlayer;
        }

        //the last player to guess will have won and they will have ben reassigned to nextPlayer
        std::cout << border;
        std::cout << "PLAYER " << nextPlayer.getID() << " WON!!!!\nCONGRATULATIONS!!!\n";
        std::cout << "play again (y/n)?\n: ";
        char again = 'q';
        std::cin >> again;
        do {
            std::cout << border;
            if(again == 'y') { Game(); }
            else if(again == 'n') { return; }
            else {
                std::cout << "invalid selection, try again.\nplay again (y/n)?\n: ";
                std::cin >> again;
            }
        }while(again != 'y' || again != 'n');
    }
}

Player.h:

#ifndef Player_h
#define Player_h

class Player {
public:
    Player() { }
    Player(int nID) { id = nID; }
    Player(std::string nType, int nID) { playerType = nType; id = nID; }
    Player(const Player &rhs);
    std::string getPlayerType() { return playerType; }
    void setPlayerType(std::string nType) { playerType = nType; }
    int getID() { return id; }
    void setID(int nID) { id = nID; }
    bool getTurn() { return turn; }
    void setTurn(bool nTurn) { turn = nTurn; }
    Board& getBoard() { return playerBoard; }
    Ship& operator()(int, int);
    Player& operator=(const Player &p);
    bool operator==(const Player &rhs);
    bool operator!=(const Player&);
private:
    Board playerBoard;
    std::string playerType = "";
    int id = -1;
    bool turn = false;
};

#endif /* Player_h */

Player.cpp:

#include "battleship.hpp"

Ship& Player::operator()(int x, int y) {
    return getBoard()(y, x);
}

Player& Player::operator=(const Player &rhs) {
    if(this != &rhs) {
        playerType = rhs.playerType;
        id = rhs.id;
        turn = rhs.turn;
        playerBoard = rhs.playerBoard;
    }
    return *this;
}

Player::Player(const Player &rhs) {
    playerType = rhs.playerType;
    id = rhs.id;
    turn = rhs.turn;
    playerBoard = rhs.playerBoard;
}

bool Player::operator==(const Player&rhs) {
    if((playerBoard != rhs.playerBoard) || (playerType != rhs.playerType) || (id != rhs.id) || (turn != rhs.turn)) {
        return false;
    }
    return true;
}

bool Player::operator!=(const Player& rhs) {
    return !(*this == rhs);
}

Board.h:

#ifndef Board_h
#define Board_h
#include "battleship.hpp"

const int BOARD_SIZE = 10;
const int numOfShips = 5;
class Board {
public:
    Board();
    Board(const Board &rhs);
    ~Board() {}
    void printBoard();
    void setShip(int shipNumber, int x1, int y1, int x2, int y2);
    bool isValidDirection(int x1, int y1, int x2, int y2);
    bool rangeIsOccupied(int x1, int y1, int x2, int y2);
    bool fitsOnBoard(int x1, int y1, int x2, int y2);
    bool isSunk(int shipNumber);
    void randomizeFleet();
    bool allShipsPlaced();
    bool allShipsSunk();
    void printRemainingShips();
    Ship& operator()(int, int);
    Ship& getShip(int n) { return Ships[n]; }
    Board& operator=(const Board &rhs);
    bool operator==(const Board&);
    bool operator!=(const Board&);
private:
    Ship gameBoard[BOARD_SIZE][BOARD_SIZE];
    Ship Ships[numOfShips];
};

#endif /* Board_h */

Board.cpp:

#include <stdio.h>
#include "battleship.hpp"

Board::Board(const Board &rhs) {
    for(int i = 0; i < BOARD_SIZE; i++) {
        for(int k = 0; k < BOARD_SIZE; k++) {
            gameBoard[i][k] = rhs.gameBoard[i][k];
        }
    }
    for(int i = 0; i < numOfShips; i++) {
        Ships[i] = rhs.Ships[i];
    }
}

//x = hit ship
//o = ship on board
//m = empty space
void Board::printBoard() {
    std::cout << "x = hit ship\no = ship on board\nm = empty space\n";
    std::cout << border;
    std::cout << "   ";
    for(int i = 0; i < BOARD_SIZE; i++) {
        std::cout << '|' << alphabet[i] << "|";
        if(i != BOARD_SIZE - 1) { std::cout << ' '; }
    }
    std::cout << "\n";
    for(int j = 0; j < BOARD_SIZE; j++) {
        std::cout << j << " - ";
        for(int k = 0; k < BOARD_SIZE; k++) {
            if(gameBoard[j][k].getShipNumber() != -1 && gameBoard[j][k].getFiredUpon()) {
                std::cout << 'x';
            }
            else if(!gameBoard[j][k].getStatus()) { std::cout << 'm'; } //empty spaces
            else { std::cout << 'o'; } //ships that are afloat
            if(k != BOARD_SIZE - 1) { std::cout << "   "; }
        }
        std::cout << "\n";
    }
    std::cout << border;
}

Board::Board() {
    Ships[0] = Ship("Carrier", 5, sunk, 0);
    Ships[1] = Ship("Battleship", 4, sunk, 1);
    Ships[2] = Ship("Cruiser", 3, sunk, 2);
    Ships[3] = Ship("Submarine", 3, sunk, 3);
    Ships[4] = Ship("Destroyer", 2, sunk, 4);
}

Ship& Board::operator()(int x, int y) {
    return gameBoard[y][x];
}

//returns bool based on if coords are valid direction
bool Board::isValidDirection(int x1, int y1, int x2, int y2) {
    if(x1 == x2 || y1 == y2) { return true; }
    return false;
}

//returns bool based on if range is occupied or not
bool Board::rangeIsOccupied(int x1, int y1, int x2, int y2) {
    //if horizontal
    if(y1 == y2) {
        while(x1 != x2) {
            if(gameBoard[y1][x1].getStatus()) {
                return true;
            }
            if(x1 > x2) {
                x1--;
            }
            else {
                x1++;
            }
        }
        if(gameBoard[y1][x1].getStatus()) {
            return true;
        }
    }

    //if vertical
    else {
        while(y1 != y2) {
            if(gameBoard[y1][x1].getStatus()) {
                return true;
            }
            if(y1 > y2) {
                y1--;
            }
            else {
                y1++;
            }
        }
        if(gameBoard[y1][x1].getStatus()) {
            return true;
        }

    }
    return false;
}

bool Board::fitsOnBoard(int x1, int y1, int x2, int y2) {
    if((x1 >= 0 && x1 < BOARD_SIZE) && (x2 >= 0 && x2 < BOARD_SIZE) && (y1 >= 0 && y1 < BOARD_SIZE) && (y2 >= 0 && y2 < BOARD_SIZE)) { return true; }
    return false;
}

bool Board::isSunk(int shipNumber) {
    return !Ships[shipNumber].getStatus();
}

bool Board::allShipsPlaced() {
    for(int i = 0; i < numOfShips; i++) {
        if(!Ships[i].getStatus()) {
            return false;
        }
    }
    return true;
}

bool Board::allShipsSunk() {
    for(int i = 0; i < BOARD_SIZE; i++) {
        for(int k = 0; k < BOARD_SIZE; k++) {
            if(gameBoard[i][k].getStatus()) {
                return false;
            }
        }
    }
    return true;
}

bool Board::operator==(const Board& rhs) {
    for(int i = 0; i < BOARD_SIZE; i++) {
        for(int k = 0; k < BOARD_SIZE; k++) {
            if(gameBoard[i][k] != rhs.gameBoard[i][k]) {
                return false;
            }
        }
    }
    for(int r = 0; r < numOfShips; r++) {
        if(Ships[r] != rhs.Ships[r]) {
            return false;
        }
    }
    return true;
}

bool Board::operator!=(const Board& rhs) {
    return !(*this == rhs);
}

Board& Board::operator=(const Board &rhs) {
    if(this != &rhs) {
        for(int i = 0; i < BOARD_SIZE; i++) {
            for(int k = 0; k < BOARD_SIZE; k++) {
                gameBoard[i][k] = rhs.gameBoard[i][k];
            }
        }
        for(int i = 0; i < numOfShips; i++) {
            Ships[i] = rhs.Ships[i];
        }
    }
    return *this;
}

void Board::setShip(int shipNumber, int x1, int y1, int x2, int y2) {
    //checks if valid placement first
    if(!rangeIsOccupied(x1, y1, x2, y2) && fitsOnBoard(x1, y1, x2, y2) && isValidDirection(x1, y1, x2, y2)) {
        Ship shipToPlace = Ship(Ships[shipNumber].getName(), Ships[shipNumber].getSize(), afloat, shipNumber);
        //place ship
        while((x1 != x2 || y1 != y2)) {
            gameBoard[y1][x1] = shipToPlace;
            gameBoard[y1][x1].setShipNumber(shipNumber);
            if(x1 > x2) {
                x1--;
            }
            else if(x2 > x1) {
                x1++;
            }
            else if(y1 > y2) {
                y1--;
            }
            else if(y2 > y1) {
                y1++;
            }
        }
        gameBoard[y1][x1] = shipToPlace;
        Ships[shipNumber].setStatus(afloat);
    }
    else {
        return;
    }
}

void Board::randomizeFleet() {
    srand(static_cast<unsigned int>(time(NULL)));
    for(int i = 0; i < numOfShips; i++) {
        int startX = rand() % BOARD_SIZE;
        int startY = rand() % BOARD_SIZE;
        int endX = startX;
        int endY = startY;

        //go vert
        if(rand() % 2 == 0) {
            //go up
            if(rand() % 2 == 0) {
                endX -= (Ships[i].getSize() - 1);
            }
            //go down
            else {
                endX += (Ships[i].getSize() - 1);
            }
        }
        //go horiz
        else {
            //go left
            if(rand() % 2 == 0) {
                endY -= (Ships[i].getSize() - 1);
            }
            //go right
            else {
                endY += (Ships[i].getSize() - 1);
            }
        }
        setShip(i, startX, startY, endX, endY);
        //don't incrment i unless ship is placed
        if(!Ships[i].getStatus()) {
            i--;
        }
    }
}

void Board::printRemainingShips() {
    for(int i = 0; i < numOfShips; i++) {
        if(!Ships[i].getStatus()) {
            std::cout << Ships[i].getShipNumber() << ". " << Ships[i].getName() << " | Size = " << Ships[i].getSize() << "\n";
        }
    }
}

Ship.h:

#ifndef Ship_h
#define Ship_h

enum Afloat {afloat = true, sunk = false};

class Ship {
public:
    Ship() { }
    Ship(std::string nName, int nSize, Afloat nStatus, int nShipNumber) { name = nName; size = nSize; status = nStatus; shipNumber = nShipNumber;}
    Ship(const Ship& rhs);
    ~Ship() { }
    int getSize() { return size; }
    void setSize(int nSize) { size = nSize; }
    std::string getName() { return name; }
    void setName(std::string nName) { name = nName; }
    bool getStatus() { return status; }
    void setStatus(Afloat nStatus) { status = nStatus; }
    bool getFiredUpon() { return firedUpon; }
    void setFiredUpon(bool f) { firedUpon = f; }
    int getShipNumber() { return shipNumber; }
    void setShipNumber(int n) { shipNumber = n; }
    Ship& operator=(const Ship&);
    bool operator==(const Ship&);
    bool operator!=(const Ship&);
private:
    int shipNumber = -1;
    std::string name = "";
    int size = 0;
    //true = sunk, false = afloat
    Afloat status = sunk;
    bool firedUpon = false;
};

#endif /* Ship_h */

Ship.cpp:

#include <stdio.h>
#include "battleship.hpp"

Ship::Ship(const Ship& rhs) {
    shipNumber = rhs.shipNumber;
    name = rhs.name;
    size = rhs.size;
    status = rhs.status;
    firedUpon = rhs.firedUpon;
}

Ship& Ship::operator=(const Ship &rhs) {
    if(this != &rhs) {
        shipNumber = rhs.shipNumber;
        name = rhs.name;
        size = rhs.size;
        status = rhs.status;
        firedUpon = rhs.firedUpon;
    }
    return *this;
}

bool Ship::operator==(const Ship& rhs) {
    if((name != rhs.name) || (size != rhs.size) || (status != rhs.status)) {
        return false;
    }
    return true;
}

bool Ship::operator!=(const Ship& rhs) {
    return !(*this == rhs);
}
\$\endgroup\$
  • \$\begingroup\$ I would recommend against having variables in a header file. Every source that includes the header file gets an instance of the variable. \$\endgroup\$ – Thomas Matthews Apr 19 '17 at 19:57
4
\$\begingroup\$

Don't use a constructor function as main entry point

It looks odd that you have implemented all of your game's functionality in the constructor of your Game class. Constructor functions are meant for the initialization of resources and member variables of a class.

The way you are using it would make it hard to reuse the class for providing extensions via inheritance for example.

I'd rather expect you provide a member function like

void Game::run() {
   // ...
}

that implements the games main loop.

Also the user interactions like

std::cout << "Please select a gamemode (1/2):\n: ";
std::cin >> gamemode;

should be placed outside of the class (e.g. in the main() function), and the result should be passed to the constructor as parameter and stored as member variable.

Use forward declarations instead of relying on the order of included headers

You didn't show which headers you had #included before the main() function, but to get that working it must be something like

#include "Player.h"
#include "Game.h"

because in the Game.h header you refer to the Player class without including Player.h there.

Anyways it would be better to forward declare Player in the Game.h header and use references like

 class Player;

 class Game {
 public:
     Game(Player& p1, Player& p2) : player1(p1), player2(p2) {}
     // ...
 private:
     Player& player1;
     Player& player2;         
 };

Use clear semantics

Having a member variable

Ship gameBoard[BOARD_SIZE][BOARD_SIZE];

in the Board class looks odd. It's unclear for me what's its purpose.

Don't wrap booleans in an enum

enum Afloat {afloat = true, sunk = false};

looks odd again and doesn't add any semantical value.

Rather provide a property for the Ship class like

class Ship {
public:
    Ship() : sunken_(false) {}
    bool sunken() const { return sunken_; }
    // ...
private:
    bool sunken_;
}

Use const correctness

The signature of the equality test operator should be

bool Ship::operator==(const Ship& rhs) const {
                                    // ^^^^^
    // ...
}

since that function doesn't change the state of a Ship instance.

Use the Single Responsibility Principle for OOP designs

You implement the fire<() function at the Game game class. That's not the right place IMO.

Let's sort out responsibilities:

  • Game
    • Keeps 2 Players
    • Implements a main playing loop to let attack/fire Players alternately in each round
    • Checks if all ships associated at a Players Board are sunken, and the game ends after a round
  • Player
    • Keeps a Board with a list of their own ships (not visible for other players)
    • Keeps a ShadowBoard to track their successful and unsuccessful attack attempts
    • Provides a function to fire at the enemy
    • In case of a CPU type player keeps a strategy algorithm, which coordinate should be attacked next
  • Board

    • Keeps the overall size of the gameboard
    • Keeps a list of Ships
    • Provides a function to add Ships by the associated Player
    • Provides a function that allows to attack a specific coordinate and returns a result that indicates

      • No hit
      • Hit
      • Hit and ship sunken

      The result is tracked at the attacking players ShadowBoard and can be used by the strategy algorithm to determine the next coordinate to attack

  • Ship

    • Keeps its coordinates on the associated Board
    • Tracks which coordinates were hit by an attack
    • If all possible coordinates were hit, state changes to sunken() == true
  • ShadowBoard

    • Keeps track of the attack attempt results of the associated Player
  • Coordinate

    • Something like

      struct Coordinate {
          unsigned x;
          unsigned y;
          std::istream& get(std::istream& is) {
             std::string input;
             if(is >> input & input.size() >= 2) {
                x = toupper(input[0]) - 'A';
                y = input[1] - `0`;
             }                   
             return is;
          }
      };
      
    • Provides a function to translate a user input like A3, J5, G7, etc. to plain XY coordinates

  • Strategy
    • Keeps track of the ShadowBoard of the associated Player
    • Keeps track of the most recent attack attempt (Coordinate)
    • Implements an algorithm to propose the next attack attempt (could be proposed to a human player, would be simply attempted by the CPU type player)

That's just a rough sketch how I would organize the classes, their relations and responsibilities.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you so much. This answer is incredibly helpful! I have a few questions if you don't mind. Should I place all user interactions (cin) within the main instead of what will soon be my run function of the game, or just the gamemode request? Also for the Board class, is the naming of the variable the issue or is there another issue I'm not seeing? I could rename it to shipCoordinates[][] or something similar? The array is what makes up the board, as individual ships. Thank you so much again, this was immensely helpful. \$\endgroup\$ – Carl Apr 19 '17 at 17:40
  • 1
    \$\begingroup\$ @Carl You may place the user interactions to configure your Game and Player classes into the main() function. The other interactions could be done in the mentioned Game::run() function, or even into another helper class. Thie latter would give you the ability, take inputs e.g. from a file for automation (testing). The Ship instances should keep their coordinates themselves, not the Board class. \$\endgroup\$ – πάντα ῥεῖ Apr 19 '17 at 17:44
  • \$\begingroup\$ Should I store the x,y coordinates in the ship class then? And then in my board class have an array of ships, of size board_size*board_size? \$\endgroup\$ – Carl Apr 19 '17 at 17:53
  • \$\begingroup\$ @Carl You should store the coorodinates of your ships in your Ship class yes. Might be sufficient to have head coords, length and orientation (horizontal/vertical). You already have a list of Ships kept in your Board class. TBH your whole design isn't very object oriented. You should learn about the Single Responsibility Principle. \$\endgroup\$ – πάντα ῥεῖ Apr 19 '17 at 17:59
  • \$\begingroup\$ @Carl I've extended my answer to explain more about my last comment. \$\endgroup\$ – πάντα ῥεῖ Apr 19 '17 at 18:48
0
\$\begingroup\$
int main() {
    Game();    // If Game is a function then make it one.
               // Don't hide a class behind a temporary variable
    return 0;
}

Would look better like this:

int main()
{
    std::unique_ptr<Player>   player1 = getPlayer(); // returns PlayerHuman or PlayerComputer object.
    std::unique_ptr<Player>   player2 = getPlayer();
    Game     game(*player1, *player2);
    game.play();
}
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ The whole organization regarding OOP design is screwed up. There's a lot more to say. Unusual answer for you ;-). \$\endgroup\$ – πάντα ῥεῖ Apr 19 '17 at 19:18

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.