Jump to content

  • Log In with Google      Sign In   
  • Create Account


Member Since 18 Apr 2013
Offline Last Active Apr 19 2013 04:17 PM

#5054739 Code Review - Battleship

Posted by on 18 April 2013 - 06:18 PM

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 (http://astyle.sourceforge.net/astyle.html) 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 (http://www.parashift.com/c++-faq/virtual-dtors.html)

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. (http://bytes.com/topic/c/answers/133064-endl-n)

- polluting the namespace with unsing namespace std is evil too (http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-a-bad-practice-in-c)


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 (http://en.wikipedia.org/wiki/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.