Jump to content

View more

Image of the Day

Inventory ! Va falloir trouver une autre couleur pour le cadre D: #AzTroScreenshot #screenshotsaturday https://t.co/PvxhGL7cOH
IOTD | Top Screenshots

The latest, straight to your Inbox.

Subscribe to GameDev.net Direct to receive the latest updates and exclusive content.

Sign up now

Code Review - Battleship

4: Adsense

Old topic!

Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
2 replies to this topic

#1 inanhour&   Members   


Posted 18 April 2013 - 08:10 AM

So, I have been learning the basics of C++ by writing a simple Battleships program (with the aid of tutorials from thenewboston.org 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.

#2 Indifferent   Members   


Posted 18 April 2013 - 03:40 PM

You need to focus on code organisation and formatting at this stage. Split each class into its own translation unit 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.

#3 Vegan   Members   


Posted 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.

Old topic!

Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.