11
\$\begingroup\$

This is a battleship console game I wrote in C++.

#include <string>
#include <iostream>
#include <process.h>
#include <cstdlib>
#include <time.h>
using namespace std;




int const ROW = 3;
int const COL = 3;


int subX, subY;


int guesses = 5;


int board[ROW][COL];


bool hasWon = false;


void winScreen()
{
    cout << "You won with " << guesses << " guesses left! :) " << endl;
}


void generateGrid()
{
    for (int x = 0; x < ROW; x++)
    {
        for (int y = 0; y < COL; y++)
        {
            //0 = empty
            //1 = has guessed
            //2 = sub


            //empty board
            board[x][y] = 0;
            cout << "-" << x << "," << y << "- ";
        }
        cout << endl;
    }
}


void generateSubCoords()
{
    subX = 0 + (rand() % ROW);
    subY = 0 + (rand() % COL);
    //2 = sub
    board[subX][subY] = 2;
}


void displayGrid(int changeX, int changeY)
{

    cout << "_______________________________________" << endl;
    for (int x = 0; x < ROW; x++)
    {
        for (int y = 0; y < COL; y++)
        {
            if (x == changeX && y == changeY)
            {
                if (board[x][y] != 2)
                {
                    board[x][y] = 1;
                }
            }

            if (board[x][y] == 0)
            {
                cout << "-" << x << "," << y << "- ";
            }
            else if (board[x][y] == 1)
            {
                cout << "-X,X- ";
            }

            if (board[x][y] == 2 )
            {
                if (x == changeX && y == changeY)
                {
                    cout << "-HIT- ";
                }
                else
                {
                    cout << "-" << x << "," << y << "- ";
                }
            }

        }
        cout << endl;
    }
}


void playerGuess()
{

    int guessX, guessY;


    bool goodInput = false;

    cout << endl;
    do {
        cout << guesses << " guesses left. " << endl;


        //get X pick
        cout << "X: ";
        cin >> guessX;
        if (guessX < 0 || guessX>2)
        {
            goodInput = false;
            cin.clear();
            cin.ignore(256, '\n');
            cout << "Out of Range!" << endl;
            //display grid again with X's on board where guessed
            break;
        }
        else
        {
            goodInput = true;
        }




        //get Y pick
        cout << "Y: ";
        cin >> guessY;
        if (guessY < 0 || guessY>2)
        {
            goodInput = false;
            cin.clear();
            cin.ignore(256, '\n');
            cout << "Out of Range!" << endl;
            //display grid again with X's on board where guessed
            break;
        }
        else
        {
            goodInput = true;
        }





        displayGrid(guessX, guessY);
        cout << endl;
        guesses--;


    } while (!goodInput);


    //if guessed right
    if (guessX == subX && guessY == subY)
    {
        hasWon = true;
        winScreen();
    }
}




void main()
{
    srand(time(NULL));
    cout << "BATTLE-SHIP 2000" << endl << endl;
    int board[ROW][COL];


    generateGrid();
    generateSubCoords();



    do
    {
        if (hasWon)
        {
            break;
        }


        playerGuess();
    } while (guesses > 0);

    //if lost
    if (!hasWon)
        cout << "Sorry, you lost. :(" << endl;

    cout << "The sub was located at -" << subX << "," << subY << "-." << endl;
}

I really feel like I'm overcomplicating things. I feel like I overuse do whiles and I should try to use a catch and throw option or something a little more creative. Any suggestions would be appreciated.

\$\endgroup\$
  • 2
    \$\begingroup\$ Can you submit your test code? \$\endgroup\$ – djechlin Dec 8 '16 at 5:19
17
\$\begingroup\$

Using namespace std

Please review this post on why you should not use using namespace std: Why is “using namespace std” considered bad practice?

Enums

//0 = empty
//1 = has guessed
//2 = sub

You should use an enum to represent those values:

enum SquareType {
    Empty = 0,
    Guessed = 1,
    Sub = 2
};

Now, instead of doing the confusing:

if (board[x][y] == 0)
{
    ...
}
else if (board[x][y] == 1)
{
    ...
}
if (board[x][y] == 2)
{
    ...
}

You can do:

if (board[x][y] == SquareType.Empty)
{
    ...
}
else if (board[x][y] == SquareType.Guessed)
{
    ...
}
if (board[x][y] == SquareType.Sub)
{
    ...
}

Style

You have way too much vertical space in your program. You should really only use 1 empty line at a time.

Your bracing is inconsistent. Either always use them (preferred), or use them as little as possible.

Input

You never use the do-while loop in playerGuess. If you carefully check the runtime, you break directly out of the loop every time you have bad input. That gives the loop a chance to continue if, and only if, you get two good inputs, but getting two good inputs automatically invalidates the continuation condition.

Notice how similar the two sections in the following code is. Duplication everywhere.

//get X pick
cout << "X: ";
cin >> guessX;
if (guessX < 0 || guessX>2)
{
    goodInput = false;
    cin.clear();
    cin.ignore(256, '\n');
    cout << "Out of Range!" << endl;
    //display grid again with X's on board where guessed
    break;
}
else
{
    goodInput = true;
}

//get Y pick
cout << "Y: ";
cin >> guessY;
if (guessY < 0 || guessY>2)
{
    goodInput = false;
    cin.clear();
    cin.ignore(256, '\n');
    cout << "Out of Range!" << endl;
    //display grid again with X's on board where guessed
    break;
}
else
{
    goodInput = true;
}

You should join these into a single input function:

int input(string prompt)
{
    int guess = -1;

    do {
        cout << prompt;
        cin >> guess;
        if (guess < 0 || guess > 2)
        {
            guess = -1;
            cin.clear();
            cin.ignore(256, '\n');
            cout << "Out of Range!" << endl;
        }
    } while (guess == -1)

    return guess;
}

Then call it like:

//get X pick
guessX = input("X: ")

//get Y pick
guessY = input("Y: ")

This will also ensure the function gets good input back the first time input() returns.

main()

Your main() function should only be the entrance to your program. You typically should not use main() to do work--create a function who's sole responsibility is doing that work. Right now, you gave main() two responsibilities--starting the program and running it.

Exceptions

Only use exceptions for exceptional behavior, not for control flow. Getting bad input is never exceptional--it is to be expected. Simple programs like this should almost never throw exceptions deliberately.

\$\endgroup\$
  • \$\begingroup\$ You used the wrong enum syntax: SquareType.Guessed should be just Guessed for the plain enum you declared. \$\endgroup\$ – JDługosz Dec 8 '16 at 7:53
  • \$\begingroup\$ In C++11, it's better to use enum class than enum. The syntax is SquareType::Guessed. enum class cannot be implicitly converted to/from int types which is generally a good thing. \$\endgroup\$ – Jack Deeth Dec 12 '16 at 20:49
11
\$\begingroup\$

Here are a number of things that may help you improve your program. The other reviews have provided some good points, so I'll try not to duplicate what's already written there.

Only use int main()

The standard defines main having a particular return type and that type is int. It is never proper to write

void main()     // <-- BAD CODE!

Instead, it should be

int main()      // correct signature

See this answer for all of the details and citations.

Eliminate unused variables

Unused variables are a sign of poor quality code, and you don't want to write poor quality code. In this code, board within main is unused. Your compiler is smart enough to tell you about this if you ask it nicely.

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea, whether programming for desktop machines or for embedded systems. For global variables such as board, ROW, COL and guesses, consider wrapping them in an object to keep them together and coherent.

Consider using a better random number generator

You are currently using

subX = 0 + (rand() % ROW);
subY = 0 + (rand() % COL);

The major problem with this approach is that the low order bits of the random number generator are not particularly random, so neither with subX be. On my machine, there's a slight but measurable bias toward 0 with that. A better solution, if your compiler and library supports it, would be to use the C++11 `std::uniform_int_distribution. It looks complex, but it's actually pretty easy to use. here's how Bjarne Stroustrup did it:

// random number generator from Stroustrup: 
// http://www.stroustrup.com/C++11FAQ.html#std-random
int rand_int(int low, int high)
{
    static std::default_random_engine re {};
    using Dist = std::uniform_int_distribution<int>;
    static Dist uid {};
    return uid(re, Dist::param_type{low,high});
}

Using this in your code would look like this:

subX = rand_int(0, ROW);
subY = rand_int(0, COL);

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of your program (which does not depend on the underlying OS). Specifically, generateGrid creates the initial grid and displays it. I would advocate separating those into two different functions, and specifically class member function.

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Eliminate "magic numbers"

This code has a number of "magic numbers," that is, unnamed constants such as 2, 5, 256, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "2" and then trying to determine if this particular 2 is relevant to the desired change or if it is some other constant that happens to have the same value.

Use only required #includes

The code has the line "include <process.h>" which has two problems. First, it's not portable. Second, it's simply not needed. Only include files that are actually needed.

\$\endgroup\$
4
\$\begingroup\$

A few things that stand out, to get you started.

  • You're not using any classes, instead you're using global state. Encapsulating some of your logic and state into classes would help to give you program more structure and make bits of it easier to reuse.
  • In main, you're declaring a local board variable, which you're not using:

    int board[ROW][COL];
    

    This has the same name as your global board, so I'm guessing that you've moved its scope during your development. The local should be removed if you're not using it.

  • In your playerGuess function you have a do{}while(!goodInput) loop. However, when you get bad input you immediately abort the loop anyway by breaking. This suggests that the method doesn't really behave the way you're expecting it to, or you're just using the loop as a means of aborting the processing like a goto. If you want to abort the function, you could simply return and then remove the entire do/while loop.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.