Sign in to follow this  

Code Review - Battleship

This topic is 1734 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

So, I have been learning the basics of C++ by writing a simple Battleships program (with the aid of tutorials from and this Youtube channel).

Well, I say simple, but to be honest it's taken me quite a bit longer than I was expecting. I guess speed is something that comes with practice, but the program seems to work anyway.


The program has basic AI, so that when the computer player hits a human player's ship once, it will try to find the rest of that ship.

The ships are also randomly positioned.


Here  it is.


I am posting this on here because I am thinking of doing something more complex (a roguelike, using ncurses). Since I'm only teaching myself, as far as I know I might be doing things inefficiently, or even doing everything completely wrong. So I want to make sure I've got the basics down first.


One thing i was unsure about was in the computer player class. There are 4 functions sharing a bunch of variables, and I defined them as members of that class. I thought that passing pointers to them all from function to function wouldn't be as clear and would look messy, but this means that they take up memory even when they're not being used.


If anyone could scan through it and tell me anything I'm doing wrong, that would be really helpful. Thanks.

Share this post

Link to post
Share on other sites

You need to focus on code organisation and formatting at this stage. Split each class into its own [url=]translation unit[/url] with a header and accompanying source file and then include them where appropriate.


You should also be a little bit more liberal with spacing your code out; the extreme density makes it difficult to follow.


For example:

if(adjustment==1){adjustment=-distance; foundEnd1=true; //switch adjustment to other side
}else if(adjustment==-1){adjustment=distance; foundEnd2=true; //an end has been found
}else if(adjustment==distance){adjustment=-1; foundEnd1=true;
}else{adjustment=1; foundEnd2=true;}


Compared with:

if (adjustment == 1) {
    adjustment = -distance;
    foundEnd1 = true; //switch adjustment to other side
} else if (adjustment == -1) {
    adjustment = distance;
    foundEnd2 = true; //an end has been found
} else if (adjustment == distance) {
    adjustment =-1;
    foundEnd1 = true;
} else {
    adjustment = 1;
    foundEnd2 = true;


You don't have to format your code in this exact style, it's just an example of spacing your code out to make it more readable.


As a tip, you don't need to compare bools to true or false explicitly.



The shorter way is simply:



There are a few other points that could be made but I think you should focus on getting the code structured into a more readable form before you're bombarded by minutiae.

Share this post

Link to post
Share on other sites

As Indifferent said your code is bad formatted.

Organize one class per file is usually a good practice, let you think better about the responsability of each class and about dependency.

Use a .h for the declaration of the class and a compile unit for the definition.


After you have subdivided your code in more compile unit is usually more readable.

You can use astyle ( to auto-format your code, or eclipse auto-format feature, or CTRL-K CTRL-F if you are using visual studio.


There are some errors/bad practice that is better to avoid

- Player class have a virtual table, but don't have a virtual destructor (

You never allocate anything on heap, but it's still a good practice.

- Player::go() should be a pure virtual function, it's the interface of player, and is useless if instanced

- You forgot to include <cstdlib> there srand is defined. It doesn't compile on gcc otherwise

- std::endl is evil. endl flush the output of the stream and it's really slow. Use it only when you really need it. (

- polluting the namespace with unsing namespace std is evil too (


Now let's check the code organization.

You have two board, a board is a grid of cell, and even if the data are private you still have total access to the internal representation.

There is no logic in this class, only the drawing function and data storage.

Note: Use standard name for setter and getter function.

Why is called addSpace? It doesn't add anything, it simply set a value on a cell



are better and more standard names.


Ship is.. nothing. It contain only the length and a pointer to board. This class also have the logic to place randomly itself on the board.

Well, this is really strange. Why board don't have a function Place ship?

Board is a more appropriate place, in this way Ship don't have to know the internal struct of board. Ship have to know that the board is 10 x 10 cell. If you change the size of the board you also have to rewrite the Ship class.

Board should contain an array of ship, in this way it can give more information to the user. Is then easy to know how many ship are destroed or give the user a better feedback

"Excellent, you destroed the Cruiser, only 3 ship of 5 remaing".


Player is the virtual interface for concrete Human player and AI.

Internally have two Board, a misterious X, Y variables and the current score.

To be used properly you must call arrangeShip(), if you don't call arrangeShip() the player can't play a proper game.. or better the other player will never hit anything and will never reach 17 point to win. Even better you have to call arranShip only ONCE. If you call arrangeShip more than once you have a lot of ship and the opponent will reach 17 point earlier, and the opponent will win, but you still have ship on your board. Mother of god! blink.png

As you can see arrangeShip must not be a public function, but must be private and called on the costructor. Or expose a public function resetPlayer that remove all the ship from the board, reset the score and rearrange the ships.

Also player suffer for the same problem of Ship, player must know the internal representation of board to know how to place his ships.


Hit have the same problem.

Why you didn't put a function of Board called Hit / Fire / Shoot?


if (boad->Fire(x, y))




} else




In this case Player must know that Board store character and that use '#' to store ship. :(



Human is a Player that do the following in his gameplay

A) read coordinate

   why you call it setXY? Yep, it set two attribute called X and Y.. but why? Because the hit function is wrong. go is more readable in this way

virtual void go()
  int X, Y;
  readCoordinates(&X, &Y);
  opponentBoard->Fire(X, Y);

This will remove the orrible protected attributes X and Y.

Note that protected attributes are like public for derived classes, so, you don't have any control over them and you can't put any restriction on them. A MadPlayer want to set X to 10000, he can.


B) Update score.

  Using magic numbers is just bad, why 17? Because you know that the sum of the possible score is 17. If you want to add a ship how many place of code you want to touch?


Note: ConvertLetter don't use any representation of human, so is not a method of player but a simple external function.



Computer go method have same problems. Need to know the representation of board, and have to read from board where he fired to check where he hit.

Considering the Cyclomatic complexity ( of the AI I'm not surprised that it took "a bit longer than I was expecting." But I'm not sure that I want to read it.. sorry. But at least this function is in the right place.


Then came the gameplay and the main loop.

You create two board, and two player (I already said that arrangeShip is wrong).

Then print a UI and call Go function on current player, check winning condition and than swap sides.

This is pretty staighforward, but why you check the winning condition on player? The game is won when the player destroy all the opponent ship.

I think that this is more readable.

Player* currentPlayer(&human);
Player* opponent(&computer);

  if (currentPlayer->getOpponentBoard()->numberOfRemainingShip() == 0)
     printf("Player%s won\n", currentPlayer->getName());
  std::swap(currentPlayer, opponent);


And that's all.

Don't forget that in real world you will write program with other people, so don't understimate the power of a solid interface and the good name of a variable.

If I found you code, try to use your Player class (without any documentation) I have to read all the code to discover how it work, and this is a huge waste of time/money in production.

Share this post

Link to post
Share on other sites
Sign in to follow this