5
\$\begingroup\$

I have built a light version of a battleships program and would love for anyone to critique it. The game does not feature OOP, AI nor multiple boards. I aimed more for functionality than correct variable names and other related things just to get it working temporarily so I'm sorry if some things are named badly.

The player will only simply continue to guess on a singular board to see if their shots hit the target amount of ship until the game is completed.

I attempted to make the program adjust dynamically to what difficulty the player has set. For example, if the player were to choose easy, then the game board has 2x2 squares. Medium has 4x4 and Hard has 8x8.

// Battleships.cpp : Defines the entry point for the console application.
//
#include "stdafx.h"
#include <ctime>
#include <iostream>
#include <string>
#include <vector>

namespace game
{
    int rows = 0;
    int columns = 0;
    int maxNumbShips = 0;

    const char WATER = '~';
    const char HIT = 'X';
    const char MISS = '/';
    const char SHIP = 'S';
    const char EMPTY = ' ';
}

void initBoard(std::vector<std::vector<char>>& playingBoard);
void displayBoard(std::vector<std::vector<char>> playingBoard);
void initShips(std::vector<std::vector<char>>& ships);
bool isGameOver(int shipsDestroyed);
void shoot(std::vector<std::vector<char>>& playingBoard, std::vector<std::vector<char>>& ships, int& shipsDestroyed, int& turns);
int enterRowPosition();
int enterColPosotion();
void selectDifficulty();

int main()
{
    std::vector<std::vector<char>> playingBoard;
    std::vector<std::vector<char>> ships;

    int shipsDestroyed = 0; //How many ships have been destroyed by the player
    int turns = 0;

    selectDifficulty();
    initBoard(playingBoard);
    initShips(ships);
    displayBoard(playingBoard);

    while (!isGameOver(shipsDestroyed))
    {
        shoot(playingBoard, ships, shipsDestroyed, turns);
        displayBoard(playingBoard);
    }

    std::cout << "You destroyed all of the ships in: " << turns << " turns.";

    std::string barn;
    std::cin >> barn;
    return 0;
}

void initBoard(std::vector<std::vector<char>>& playingBoard)
{
    std::vector<char> tempVector;
    for (int i = 0; i < game::rows; i++)
    {
        tempVector.push_back(game::WATER);
    }

    for (int i = 0; i < game::columns; i++)
    {
        playingBoard.push_back(tempVector);
    }
}

void initShips(std::vector<std::vector<char>>& ships)
{
    std::vector<char> tempVector;
    for (int i = 0; i < game::rows; i++)
    {
        tempVector.push_back(game::EMPTY);
    }

    for (int i = 0; i < game::columns; i++)
    {
        ships.push_back(tempVector);
    }

    srand(time(NULL));
    bool emptyPosition = false;

    for (int ship = 0; ship < game::maxNumbShips; ship++)
    {
        do
        {
            int x = rand() % game::rows;
            int y = rand() % game::columns;

            if (ships[x][y] == game::SHIP)
            {
                emptyPosition = false;
            }
            else
            {
                ships[x][y] = game::SHIP;
                emptyPosition = true;
            }
        } while (!emptyPosition);
    }
}

void displayBoard(std::vector<std::vector<char>> playingBoard)
{
    std::cout << "\n";
    for (int row = 0; row < game::rows; row++)
    {
        std::cout << "\t";
        for (int col = 0; col < game::columns; col++)
        {
            std::cout << playingBoard[row][col];
            std::cout << "\t";
        }
        std::cout << "\n";
    }
}


bool isGameOver(int shipsDestroyed)
{
    return shipsDestroyed >= game::maxNumbShips;
}

void shoot(std::vector<std::vector<char>>& playingBoard, std::vector<std::vector<char>>& ships, int& shipsDestroyed, int& turns)
{
    int row = enterRowPosition();
    int col = enterColPosotion();

    if (ships[row][col] == game::SHIP)
    {
        playingBoard[row][col] = game::HIT;
        shipsDestroyed++;
    }

    else
    {
        playingBoard[row][col] = game::MISS;
    }
    turns++;
}

int enterRowPosition()
{
    int row = 0;
    do
    {
        std::cout << "Enter row: ";
        std::cin >> row;
    }
    while (row < 0 || row >= game::rows);

    return row;
}

int enterColPosotion()
{
    int col = 0; 
    do
    {
        std::cout << "Enter column: ";
        std::cin >> col;
    } while (col < 0 || col >= game::columns);

    return col;
}

void selectDifficulty()
{
    int choice = 0;
    std::cout << "Select difficulty.\n";
    std::cout << "1: Easy.\n2: Medium. \n3: Hard.\n";
    std::cin >> choice;

    switch (choice)
    {
    case 1:
        game::rows = 2;
        game::columns = 2;
        game::maxNumbShips = 1;
        break;

    case 2:
        game::rows = 4;
        game::columns = 4;
        game::maxNumbShips = 4;
        break;

    case 3:
        game::rows = 8;
        game::columns = 8;
        game::maxNumbShips = 8;
        break;

    default:
        std::cout << "Please enter valid information.";
        break;
    }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Remove unnecessary headers

#include "stdafx.h"
#include <string>

You don't need both of these headers. The first is a Windows-only header (which limits portability of your application) and the other is only used on a single instance that is completely unimportant to the execution of the program. If you don't want the console to quit after the game ends, just do:

char c;
cin >> c;

That way, you're avoiding including a huge header for such a trivial task.

Remove function prototypes

For such a simple program like this, function prototypes are not necessary; we generally like to keep function prototypes around only in header files (for documentation and encapsulation purposes). Moreover, they introduce a new source of maintenance for you if your function signatures ever change. Just move the main function to the bottom of the file and keep all the other functions above it.

Think about making this OOP

As you said, you were in a rush making this but I would strongly recommend revisiting this and making at least the board a class with some good setters/getters.

Code Redundancy / Game Design

void initBoard(std::vector<std::vector<char>>& playingBoard)
{
    std::vector<char> tempVector;
    for (int i = 0; i < game::rows; i++)
    {
        tempVector.push_back(game::WATER);
    }

    for (int i = 0; i < game::columns; i++)
    {
        playingBoard.push_back(tempVector);
    }
}

void initShips(std::vector<std::vector<char>>& ships)
{
    std::vector<char> tempVector;
    for (int i = 0; i < game::rows; i++)
    {
        tempVector.push_back(game::EMPTY);
    }

    for (int i = 0; i < game::columns; i++)
    {
        ships.push_back(tempVector);
    }

// ...
}

This is the exact same code repeated twice. Factor it out as a helper function then call it to obtain a ships vector and a playingBoard vector. Here is an example (since C++11):

std::vector<std::vector<char>> CreateBoard(char fill_value)
{
    std::vector<char> vec{game::rows, fill_value};
    return {game::columns, vec};
}

This constructs a vector of game::rows chars that are initialized to fill_value. Then it returns a vector of those that is game::columns big. You also avoid the need for awkward references in the parameters this way.

In initShips

std::vector<std::vector<char>> initShips()
{
    auto ships = CreateBoard(game::EMPTY);

    /// randomly put ships in ships :)

    return ships;
}

Now, you have no need for the initBoard function either. In main, you can just do:

auto playingBoard = CreateBoard(game::WATER);
auto ships = initShips();

Simplify game logic

srand(time(NULL));
bool emptyPosition = false;

for (int ship = 0; ship < game::maxNumbShips; ship++)
{
    do
    {
        int x = rand() % game::rows;
        int y = rand() % game::columns;

        if (ships[x][y] == game::SHIP)
        {
            emptyPosition = false;
        }
        else
        {
            ships[x][y] = game::SHIP;
            emptyPosition = true;
        }
    } while (!emptyPosition);
}

Here, you already have emptyPosition set to false. There's no need to reassign it to that. Just have the value change if a certain criterion is met:

for (int ship = 0; ship < game::maxNumbShips; ship++)
{
    do
    {
        int x = rand() % game::rows;
        int y = rand() % game::columns;

        if (ships[x][y] != game::SHIP)
        {
             emptyPosition = true;
             ships[x][y] = game::SHIP;
        }
    } while (!emptyPosition);
}

Unnecessary Copy

Your displayBoard function should be taking in the vector by const reference as you don't need a copy of the board and you're not modifying it.

Error Handling

Think about what happens if the user enters the number "4" on the difficulty selection menu. As it is, the game will proceed and not do anything. This isn't how a game is meant to flow. I would make sure that the user entered a valid choice at that menu and maybe provide an option to quit if they really want to exit the program.

\$\endgroup\$
3
  • \$\begingroup\$ Sorry, I don't understand why that is. \$\endgroup\$
    – Bizkit
    Commented Jan 15, 2016 at 19:29
  • \$\begingroup\$ Oh my. Sorry for not checking this as answered, it completely escaped me that I posted this on here. Thank you so much for your help! \$\endgroup\$
    – Ryan Swann
    Commented Jan 21, 2016 at 15:56
  • \$\begingroup\$ I think you meant vec(game::rows, fill_value);, since you specifically don't want to construct from initializer list. \$\endgroup\$ Commented Mar 22, 2017 at 15:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.