[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.
```
[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.
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.
```
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.
```
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.