Followers 0

# Code Review - Battleship

## 2 posts in this topic

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.

0

##### Share on other sites

You need to focus on code organisation and formatting at this stage. Split each class into its own [url=http://stackoverflow.com/questions/1106149/what-is-a-translation-unit-in-c]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


Compared with:

if (adjustment == 1) {
foundEnd1 = true; //switch adjustment to other side
} else if (adjustment == -1) {
foundEnd2 = true; //an end has been found
} else if (adjustment == distance) {
foundEnd1 = true;
} else {
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.

if(foundDirection==true)
if(foundDirection==false)


The shorter way is simply:

if(foundDirection)
if(!foundDirection)


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.

1

## Create an account or sign in to comment

You need to be a member in order to leave a comment

## Create an account

Sign up for a new account in our community. It's easy!

Register a new account