BASIC Computer Games: Battle in C++

Question

As a C++ newbie, I am practicing my C++ skill by implement games in BASIC Computer Games and here is my C++ version for Battle.

Suggestions I want

Any suggestion are welcome!

Current Project code from my work place full of C styles in C++ (which is not a good thing), I want my code to be C++ style, and to be more C++11 or modern C++ style. And I was a Python coder, I am still not know well about things in C++ like static, pointer and so on.

About My Code

In my Battle implementation, I hard code the game map part, I don't have a good idea about this still XD. So let's skip it first, that is my TODO

void InitMap()
{
 // TODO: random map algorithm
 m_gameMap = {
 {0,0,0,2,2,6},
 {0,4,4,4,6,0},
 {5,0,0,6,0,0},
 {5,0,6,0,0,3},
 {5,1,0,0,0,3},
 {5,0,1,0,0,3},
 };
}

Game is simple, user input a coordinate to hit, and then tell the user result.

When a ship is down(all coordinates about this one are hit), tell the user whole situation about game right now.

if (iAttack > 0) {
 std::cout << "A Direction Hit On Ship Number " << iAttack << std::endl;
 if (!game.IsShipStillSurvive(iAttack)) {
 std::cout << "And you sunk it. Hurrah for the good guys." << std::endl;
 std::cout << "So far the bad gut have lost" << std::endl;
 game.ShipLost(iDestroyerLost, iCruisersLost, iAircraftLost);
 std::cout << iDestroyerLost << " Destroyer(s), " << iCruisersLost;
 std::cout << " Cruiser(s), And " << iAircraftLost << " Aircraft Carrier(s)." << std::endl;
 }
} else {
 std::cout << "Splash!" << std::endl;
}

Code

#include <iostream>
#include <vector>
#include <map>
#include <string>
#include <sstream>
typedef enum {
 SHIP_TYPE_DESTROYER,
 SHIP_TYPE_CRUISERS,
 SHIP_TYPE_AIRCRAFT,
} ShipType;
typedef unsigned int ShipNumber ;
struct Coordinate {
 unsigned int x;
 unsigned int y;
};
class Game
{
public:
 Game()
 {
 m_shipLiveInfo.clear();
 for (auto iter = m_shipNumberInfo.begin(); iter != m_shipNumberInfo.end(); ++iter) {
 unsigned int iLive = 0;
 auto fleetIter = m_shipFleetInfo.find(iter->second);
 if (fleetIter != m_shipFleetInfo.end()) {
 iLive = fleetIter->second;
 }
 m_shipLiveInfo.insert(std::pair<ShipNumber, unsigned int>(iter->first, iLive));
 }
 InitMap();
 }
 virtual ~Game() {}
 void Attack(const Coordinate& coordinate, ShipNumber& iAttackShip)
 {
 iAttackShip = 0;
 if (coordinate.x <= m_iMapWidth && coordinate.y <= m_iMapHeight && coordinate.x > 0 && coordinate.y > 0) {
 iAttackShip = m_gameMap[coordinate.x - 1][coordinate.y - 1];
 std::cout << "Attack " << iAttackShip << std::endl;
 m_gameMap[coordinate.x - 1][coordinate.y - 1] = 0;
 auto iter = m_shipLiveInfo.find(iAttackShip);
 if (iter != m_shipLiveInfo.end()) {
 --(iter->second); 
 }
 }
 }
 bool IsShipStillSurvive(ShipNumber& iShip)
 {
 std::map<ShipNumber, unsigned int>::iterator iter = m_shipLiveInfo.find(iShip);
 if (iter == m_shipLiveInfo.end()) {
 return true;
 }
 return (iter->second > 0);
 }
 bool IsGameOver()
 {
 for (auto iter = m_shipLiveInfo.begin(); iter != m_shipLiveInfo.end(); ++iter) {
 if (iter->second > 0) {
 return false;
 }
 }
 return true;
 }
 
 void ShipLost(unsigned int& iDestroyer, unsigned int& iCruisers, unsigned int& iAircraft)
 {
 for (auto iter = m_shipLiveInfo.begin(); iter != m_shipLiveInfo.end(); ++iter) {
 auto shipIter = m_shipNumberInfo.find(iter->first);
 if (shipIter == m_shipNumberInfo.end()) {
 continue;
 }
 ShipType shipType = shipIter->second;
 if (iter->second == 0) {
 switch (shipType) {
 case SHIP_TYPE_DESTROYER:
 ++iDestroyer;
 break;
 case SHIP_TYPE_CRUISERS:
 ++iCruisers;
 break;
 case SHIP_TYPE_AIRCRAFT:
 ++iAircraft;
 break;
 }
 }
 }
 }
 static const unsigned int m_iMapWidth = 6;
 static const unsigned int m_iMapHeight = 6;
private:
 
 std::vector<std::vector<ShipNumber>> m_gameMap;
 std::map<ShipNumber, unsigned int> m_shipLiveInfo;
 static const std::map<ShipType, unsigned int> m_shipFleetInfo;
 static const std::map<unsigned int, ShipType> m_shipNumberInfo;
 void InitMap()
 {
 // TODO: random map algorithm
 m_gameMap = {
 {0,0,0,2,2,6},
 {0,4,4,4,6,0},
 {5,0,0,6,0,0},
 {5,0,6,0,0,3},
 {5,1,0,0,0,3},
 {5,0,1,0,0,3},
 };
 }
};
const std::map<ShipNumber, ShipType> Game::m_shipNumberInfo = {
 {1, SHIP_TYPE_DESTROYER},
 {2, SHIP_TYPE_DESTROYER},
 {3, SHIP_TYPE_CRUISERS},
 {4, SHIP_TYPE_CRUISERS},
 {5, SHIP_TYPE_AIRCRAFT},
 {6, SHIP_TYPE_AIRCRAFT},
};
const std::map<ShipType, unsigned int> Game::m_shipFleetInfo = {
 {SHIP_TYPE_DESTROYER, 2},
 {SHIP_TYPE_CRUISERS, 3},
 {SHIP_TYPE_AIRCRAFT, 4}
};
int main(int argc, char** argv)
{
 Game game = Game();
 Coordinate cor;
 std::string strPosition;
 std::vector<unsigned int> positionNums;
 ShipNumber iAttack;
 bool isFirstStart = true;
 unsigned int iDestroyerLost = 0;
 unsigned int iCruisersLost = 0;
 unsigned int iAircraftLost = 0;
 while (!game.IsGameOver()) {
 iDestroyerLost = 0;
 iCruisersLost = 0;
 iAircraftLost = 0;
 
 if (isFirstStart) {
 std::cout << "Start Game" << std::endl;
 isFirstStart = false;
 } else {
 std::cout << "Try Again" << std::endl;
 }
 positionNums.clear();
 std::cin >> strPosition;
 std::stringstream ss(strPosition);
 for (unsigned int i; ss>>i;) {
 positionNums.push_back(i);
 if (ss.peek() == ',') {
 ss.ignore();
 }
 }
 if (positionNums.size() < 2) {
 std::cout << "InValid Input" << std::endl;
 continue;
 }
 cor.x = positionNums[0];
 cor.y = positionNums[1];
 if (cor.x > Game::m_iMapWidth || cor.x == 0 || cor.y > Game::m_iMapHeight || cor.y == 0) {
 std::cout << "InValid Input" << std::endl;
 continue;
 }
 game.Attack(cor, iAttack);
 if (iAttack > 0) {
 std::cout << "A Direction Hit On Ship Number " << iAttack << std::endl;
 if (!game.IsShipStillSurvive(iAttack)) {
 std::cout << "And you sunk it. Hurrah for the good guys." << std::endl;
 std::cout << "So far the bad gut have lost" << std::endl;
 game.ShipLost(iDestroyerLost, iCruisersLost, iAircraftLost);
 std::cout << iDestroyerLost << " Destroyer(s), " << iCruisersLost;
 std::cout << " Cruiser(s), And " << iAircraftLost << " Aircraft Carrier(s)." << std::endl;
 }
 } else {
 std::cout << "Splash!" << std::endl;
 }
 }
 
 return 0;
}

• Toby Speight: Are you really stuck with C++11? That's pretty archaic these days.
• Aries_is_there: @TobySpeight xD, Good point, now is 2021. I should go for C++20!

Answer 1

The code is not bad, but I see some things that may help you improve it.

Check your spelling

There are some typos and peculiar capitalization in the strings displayed for the user. For instance "gut" should probably be "guys," "InValid" should be "Invalid" and "Direction Hit" should be "direct hit." A little extra effort on that helps the user.

Omit autoconstructed functions

The code currently has this line:

virtual ~Game() {}

It's reasonable to make the destructor virtual if you expect to derive from it, but that's not the case here, so I would recommend simply omitting it and letting the compiler generate the default destructor.

Use const where practical

In many cases, such as IsGameOver(), the code doesn't and shouldn't alter the underlying Game object, and so it should be declared const:

bool IsGameOver() const;

Prefer std::array to std::vector if sizes are fixed at compile time

Because we know the sizes at compile time, everywhere you've used a std::vector, I'd suggest using a std::array instead. For this code it probably won't make much of a difference, but it's useful to get into the habit of using the most economical data type that is practical.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Rethink your use of objects

The Game class is a good idea, but information about ships is scattered in multiple places. For example, the m_shipNumberInfo has a map of ship number to ship type, and then m_shipFleetInfo has a map of ship type to ship length. Both make references to the enum ShipType. For this, I'd suggest instead using objects. One could start with this:

class Ship {
public:
 enum ShipType { DESTROYER=2, CRUISER=3, AIRCRAFT=4 };
 Ship(ShipType t) : myType{t}, remaining{t} {}
 unsigned hit() { return remaining ? --remaining : 0; }
 // return true if ship is still alive
 bool operator()() const { return remaining; }
 // data members
 ShipType myType;
 unsigned remaining;
};

Now all of the information about a ship is contained in a single place. We can then make this class private within Game and keep track of them with a default initialization like this:

std::array<Ship, 6> ships = { 
 Ship::ShipType::DESTROYER, 
 Ship::ShipType::DESTROYER, 
 Ship::ShipType::CRUISER, 
 Ship::ShipType::CRUISER, 
 Ship::ShipType::CARRIER, 
 Ship::ShipType::CARRIER,
};

Many things become much simpler now. For example:

bool IsShipStillSurvive(ShipNumber& iShip) const
{
 return ships.at(iShip - 1)();
}
bool IsGameOver() const
{
 return std::none_of(ships.begin(), ships.end(), [](const Ship& s){return s();});
}

Think of the user

The game gives very little instruction or feedback, making it a bit difficult to play. I'd suggest adding some instructions and also providing a map for the user at each turn to show places where attacks had already been made and which ones resulted in hits.

Consider additional objects

In addition to the Ship object mentioned above, it might also be nice to separate a Board object from the Game and to use a Model-View-Container approach. Also, the Coordinate could be a public object within Game and the input and validation could be a member function of that class rather than in main.

Use C++20 features

One handy feature of C++20 is the ability to return a std::tuple. So instead of this:

game.ShipLost(iDestroyerLost, iCruisersLost, iAircraftLost);

We can instead write this:

auto [iDestroyerLost, iCruisersLost, iAircraftLost] = game.ShipLost();

The corresponding function can be simplified using std::count_if:

std::tuple<unsigned, unsigned, unsigned > ShipLost()
{
 unsigned iDestroyer = std::count_if(ships.begin(), ships.end(), [](const Ship& s){ 
 return s.myType == Ship::ShipType::DESTROYER && !s(); });
 unsigned iCruisers = std::count_if(ships.begin(), ships.end(), [](const Ship& s){ 
 return s.myType == Ship::ShipType::CRUISER && !s(); });
 unsigned iAircraft = std::count_if(ships.begin(), ships.end(), [](const Ship& s){ 
 return s.myType == Ship::ShipType::CARRIER && !s(); });
 return std::make_tuple(iDestroyer, iCruisers, iAircraft);
}

Answer 2

First, your "TODO" to populate the map is good. This is an example of top-down decomposition, and it is good to block out your code and start with a stub.

---

typedef enum {
 SHIP_TYPE_DESTROYER,
 SHIP_TYPE_CRUISERS,
 SHIP_TYPE_AIRCRAFT,
} ShipType;
typedef unsigned int ShipNumber ;

typedef enum? This isn't C. Write enum ShipType { ⋯ }; or even an enum class depending on your needs. You don't need to typedef what are "tags" in C to get type names; they already are type names in C++.

It's good to give your types symbolic names even if they are not "strong types", but don't use the typedef keyword anymore, at all. Write this as:

using ShipNumber = unsigned int;

or better yet, use the <stdint> types like uint32_t.

---

Why are you making your destructor virtual? You have no (other) virtual functions in the class, and you don't use polymorphism in any way. You are using the class itself, not derived classes.

---

Game game = Game();

Just:

Game game;

is what you need. Do you understand that Game is created on the stack, not the heap? I wonder if you're thinking Java-like because other variables are not being initialized this way.

---

Use signed integer types, not unsigned, unless you are doing bit manipulation, rely on overflow rolling over, or really need one more bit of range.

---

Coordinate cor;
std::string strPosition;
std::vector<unsigned int> positionNums;

These are defined as part of a bunch at the beginning of main. But, you don't need them to preserve their value across loop iterations; in fact, you call positionNums.clear() near the top of the loop. Declare variables where you first need them.

---

Abstract out the users input/interaction

Instead of mixing cin >> stuff throughout the huge loop that does everything,
⭐ write functions to do one thing as a single level of abstraction farther down.⭐
Your game loop should read like an outline, with named functions expressing high-level steps, not blobs of code doing those steps.

As for the input in particular, using cin>> is nasty in that it goes against best practices of defining variables where you need them, initializing them, and hopefully making them const. And then you have to parse the input and deal with errors, all "deeper" levels of detail than you need. The main loop should state simply:

const Coordinate cor = get_play();

This whole blob of code:

 positionNums.clear();
 std::cin >> strPosition;
 std::stringstream ss(strPosition);
 for (unsigned int i; ss>>i;) {
 positionNums.push_back(i);
 if (ss.peek() == ',') {
 ss.ignore();
 }
 }
 if (positionNums.size() < 2) {
 std::cout << "InValid Input" << std::endl;
 continue;
 }
 cor.x = positionNums[0];
 cor.y = positionNums[1];

is detail that does not belong expanded out here in the main loop.

It's also good to abstract out the input/interaction as a separate concern, anyway. In the BASIC port you can use terminal input as you have here, but you might replace that with some kind of GUI or game controls later, simply by changing the implementation of these well-specified functions and not affecting the core game play logic at all.

---

void Attack(const Coordinate& coordinate, ShipNumber& iAttackShip)

I'm supposing the second must be "out" parameters, and this function is providing this information to the caller.

Strenuously avoid "out" parameters.
Return things using function return values.

if (coordinate.x <= m_iMapWidth && coordinate.y <= m_iMapHeight && coordinate.x > 0 && coordinate.y > 0) {
 // the entire meat of the function
 }

Write "to the left margin". Don't keep opening up deeper and deeper levels of nesting. What you have here is a precondition. Since this is a global constraint (the entire map) it should have been checked already, as part of the input function, and this function can assume it's passed a valid value of coordinate.

But just to illustrate, preconditions should return or throw, as a prelude to the real body of the function. Rather than opening another nesting level:

if (!mapbounds.contains(coordinate)) return 0;

And notice that checking to see if a point is within a rectangle is a general reusable operation. You make it more complex and less "high level meaning" by keeping your map size in two separate variables rather than another coordinate pair.

---

[coordinate.x - 1][coordinate.y - 1]

You are repeatedly adjusting the indexes when you access your game data. Use internal coordinate values that match the board's representation. That is, zero-based. The adjustment should be done as part of the user I/O. In fact, on a real Battleship board, you use letters for one of the axis, so it would actually be A through K or however big the board is. So, the User I/O mapping is more general than just subtracting 1; it can be a completely different representation. One axis can subtract 1 from a input number, the other mapps the letter to a number. Likewise, displaying the coordinate is a User I/O issue and the output functions should take the same coordinate that's used by the data structure.