10
\$\begingroup\$

Below my first code in c++. Its running fine. I feel I could improve a few things in my code. Please feel free to comment

#include <iostream>
#include <ctime>
#include "windows.h" 

using namespace std;

class BattleShips {    
public:           
    static const int rows = 2;
    static const int elements = 2;

    int maxships = 1;
    int score = 0;

    char matrix[rows][elements];
    char matrixAttackedPlaces[rows][elements];
    
    void Clear()
    {
        for (int i = 0; i < rows; i++)
        {
            for (int j = 0; j < elements; j++)
            {
                matrix[i][j] = '0';
                matrixAttackedPlaces[i][j] = '*';
            }
        }
    }

    void Show()
        
    {
        for (int i = 0; i < rows; i++)
        {
            for (int j = 0; j < elements; j++)
            {
                cout << matrix[i][j] << " ";
            }
            cout << endl;
        }
    }

    void ShowAttackedPlaces()

    {
        for (int i = 0; i < rows; i++)
        {
            for (int j = 0; j < elements; j++)
            {
                cout << matrixAttackedPlaces[i][j] << " ";
            }
            cout << endl;
        }
    }

    int NumberOfShips()
    {
        int c = 0;

        for (int i = 0; i < rows; i++)
        {
            for (int j = 0; j < elements; j++)
            {
                if (matrix[i][j] == 1)
                    c++;
            }
        }

        return c;
    }

    void SetShips()
    {
        int s = 0;
        while (s < maxships)
        {
            int x = rand() % rows;
            int y = rand() % elements;
            if (matrix[x][y] != '1')
            {
                s++;
                matrix[x][y] = '1';
            }
        }
    }

    bool Attack(int x, int y)
    {
        matrixAttackedPlaces[x][y] = 'X';
        if (matrix[x][y] == '1')
        {
            matrix[x][y] = '$';
            ++score;
            return true;
        }
        else matrix[x][y] = 'X';
        return false;
    }

};

void DrawSeparationLine(int x, char c) {
    for (int i = 0; i < x; i++) {
        cout << c << " ";
    }
    cout << endl;

}

void GameEndScreen() 
{
    system("CLS");
    DrawSeparationLine(9, 'x');
    DrawSeparationLine(9, '-');
    DrawSeparationLine(9, 'x');
    cout << "G A M E   O V E R \n";
    DrawSeparationLine(9, 'x');
    DrawSeparationLine(9, '-');
    DrawSeparationLine(9, 'x');
}


int main() {
    srand(time(NULL));
    BattleShips player1;
    BattleShips player2;

    player1.Clear();
    player1.SetShips();

    player2.Clear();
    player2.SetShips();

    int pos1, pos2;
    char prompt;
    int streak=0;
    
    while (1) {
        system("CLS");
        player1.ShowAttackedPlaces();
        DrawSeparationLine(player1.elements, '-');
        player2.Show();


        if (player1.score == player1.maxships || player2.score == player2.maxships)
        {
            if (player1.score == player1.maxships) {
                GameEndScreen();
                cout << "---- YOU  WON ----";
            }
            else {
                GameEndScreen();
                cout << "---- YOU LOSE ---";
            }
            cout << endl;
            DrawSeparationLine(9, 'x');
            DrawSeparationLine(9, '-');
            DrawSeparationLine(9, 'x');
            break;
        }


        //player turn

        cout << "\nScore: " << player1.score << " Streak: " << streak << endl;
        cout << "Please input location:\n"; cin >> pos1 >> pos2;
        if (player1.Attack(pos1, pos2))
        {
            streak++;
            cout << "You got me! :)" << endl;
        }
        else
        {
            cout << "Sorry no ship at that position!" << endl;
            streak = 0;
        }
        if (pos1 > player1.elements || pos2 > player1.rows)
            cout << "You didn't have a chance to hit anyway. You chose a field outside the map.\n";


        //CPU player turn
        cout << "It's time for CPU player.";
        Sleep(1000);
        cout << " .";
        Sleep(1000);
        cout << " .";
        cout<<endl;
        while (1)
        {
            pos1 = rand() % player1.rows;
            pos2 = rand() % player1.elements;
            if (player2.matrix[pos1][pos2] != 'X')
            {
                player2.Attack(pos1,pos2);
                break;
            }
        }

        cout << "CPU player attacked your base at positions: " << pos1 << ", " << pos2 << "\n";

        cout << "Do you want to surrender (y/n)? "; cin >> prompt;
        if (prompt == 'y')
            break;
        

    }
    
    return 0;
}
\$\endgroup\$
2
  • \$\begingroup\$ I like the code. On minor thing, I would put the class definition in a separate header file and include it. \$\endgroup\$
    – Bob
    Sep 16, 2022 at 2:27
  • \$\begingroup\$ It's very common these days to see dozens and dozens of lines of code with zero comments. 'In my day' this was a mortal sin. Serious question to other reviewers - Is this normal and acceptable these days? I get that code should be self-explanatory, but gee, a comment every few lines can go a long way to adding a lot more clarity, that is easier to absorb on a first read. Am I just old? \$\endgroup\$
    – spechter
    Sep 17, 2022 at 13:30

3 Answers 3

11
\$\begingroup\$

Here are some things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    std::cout << "\x1b[2J";
}

Prefer portable code to platform specific code

The code is currently calling Sleep which is a platform-dependent function. This could be made platform independent by instead using std::this_thread::sleep_for

Consider using a better random number generator

If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::uniform_real_distribution and friends in the <random> header.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Reconsider the object design

First, everything is declared public which is not really a good design. Make only the interfaces that you intend to provide public and only provide data members directly if any other part of the program is allowed to alter it. That's not the case here, so I'd expect that all of the data members would be private instead.

Second, the Clear and SetShips functions are always called sequentially. I'd suggest combining those into one and making it, for example a Board constructor. In modern C++, uninitialized objects are to be avoided. Strive instead for each object to be immediately usable after creation.

Third, some of your external functions should probably be member functions. Instead of this line:

if (player1.score == player1.maxships || player2.score == player2.maxships)

I would rather write this:

if (player1.wins() || player2.wins())

Use const where practical

A number of the functions don't (and shouldn't) alter the underlying object, such as ShowAttackedPlaces and NumberOfShips. Declare those as const functions like this:

int NumberOfShips() const  // this call does not alter the underlying object

Avoid using raw arrays

Instead of this:

char matrix[rows][elements];
char matrixAttackedPlaces[rows][elements];

I would recommend using std::array like this:

std::array<char, rows * elements> mine;
std::array<char, rows * elements> theirs;

Then your Clear function reduces to this:

mine.fill('0');
theirs.fill('*');

Speaking of that, however, see the next suggestion.

Eliminate "magic numbers"

This code does a pretty good job of avoiding "magic numbers," that is, unnamed constants that the reader of the code is left to figure out. However, we could do better, especially with the various string tokens that signify various states, as with '*' and '0' in the code above.

Consider using standard routines

C++ provides a great number of useful utilites such as std::for_each that you could use instead of your own hand-written sort function. Even if the current assignment does not allow their use, it's extremely useful to know about them and to use them preferentially to writing your own. They're almost always faster and less buggy and by using library software, you can develop your own code more quickly and accurately. I showed std::fill already, but many of the other routines could also be simplified. Here's one way rewrite NumberOfShips():

int NumberOfShips() const
{
    return std::accumulate(mine.begin(), mine.end(), 1);
}

Consider the user

It's not obvious how the user is supposed to enter coordinates. Consider an initial splash screen to explain the game a bit. Also, the game ending screen is rather cluttered and hard to read. It would also be helpful to the human player if the updated board display would show opponent's sunk ships. Also, if the player surrenders, the program just suddenly ends. Consider printing a message.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ "Using std::endl emits a \n and flushes the stream." is misleading. cppreference.com has this to say about it: "In many implementations, standard output is line-buffered, and writing '\n' causes a flush anyway" \$\endgroup\$
    – Ben Voigt
    Sep 16, 2022 at 18:09
  • 1
    \$\begingroup\$ @BenVoigt Well, the second flush is an empty flush than, so not quite as big a waste, maybe, but still a waste. \$\endgroup\$ Sep 17, 2022 at 9:02
5
\$\begingroup\$

OverView

Good start.
I like how you have encapsulate the code into a class.

What I don't like is all the work being done in main(). You could split this out into another class. What you call BattleShips I would call HumanPlayer (in the future we may want a ComputerPlayer) and I would have a class called BattleShipsGame that handles the mechanics of each players turn (the stuff you have in main()).

Additionally I would encapsulate all my code in its own namespace.

I would stay away from the old C rand() function. Its use has long been considered bad as the number it generates are horrible and the standard way of getting a number in the range 0-(n-1) is done by using the mod operator which makes the section even worse (as it will bias some of the early numbers in the range).

Use the more modern C++ random number library.
See: Generate random numbers uniformly over an entire range

Design

Personally I would do something like this:

namespace ThorsAnvil
{
    // If I were to design this it would look like.
    class Player
    {
         // Define a common interface for all players.
    };
    class HumanPlayer: public Player
    {
    };
    class ComputerPlayer: public Player
    {
    };
    
    class BattleShipsGame
    {
        Player&   p1;
        Player&   p2;
        public:
            BattleShipsGame(Player& p1, Player& p2)
                : p1(p1)
                , p2(p2)
            {}
    
            void play();
    };
}

int main()
{
    // Init Rand here.

    // Players
    HumanPlayer   p1;
    HumanPlayer   p2;

    // Game.
    BattleShipsGame game(p1, p2);
    game.play();
}

Code Review

Please don't do this:

using namespace std;

see: Why is "using namespace std;" considered bad practice?


Please put all your code in your own namespace.


The BattleShips class does not have a constructor.

class BattleShips {    

This means you have a two phase initialization. You create the object then you set the value. If I look down into your main I see:

    BattleShips player1;
    BattleShips player2;

    player1.Clear();
    player1.SetShips();

    player2.Clear();
    player2.SetShips();

Why not add a constructor that simply calls Clear() and SetShips() for you.

     BattleShips()
     {
         Clear();
         SetShips();
     }

This then clears up your main as creation and initialization are done in one place.


A couple of the methods on your Battleship class don't modify the object (they simply display it or calculate a value). Methods that don't change the state of the object should be marked const.

void Show()         const;
int NumberOfShips() const;

Prefer not to use std::endl it is usually the wrong thing to do. Normally it is sufficient to use "\n". The only difference is that std::endl forces a flush of the buffer. With user input the standard output is automatically flushed before user input is requested so there is no need to do it manually.

I would note that most complaints about slow output in C++ are caused by people flushing the buffer way to often (without knowing it). If you remove the flushes you can usually see a significant increase in speed.


You should use range based for loops more: They are easier to read:

        for (int i = 0; i < rows; i++)
        {
            for (int j = 0; j < elements; j++)
            {
                if (matrix[i][j] == 1)
                    c++;
            }
        }
        // Using range based loops:

        for (auto const& r: matrix)
        {
            for (auto const& el: r)
            {
                if (el == 1) {
                    c++;
                }
            }
        }

I am surprised at this function.

    int NumberOfShips()
    {
        int c = 0;

        for (int i = 0; i < rows; i++)
        {
            for (int j = 0; j < elements; j++)
            {
                if (matrix[i][j] == 1)
                    c++;
            }
        }

        return c;
    }

Looks like you are calculating the value each time you call. Seems like it would be more efficient to store this value.


OK. This works because maxships is very small (1) in comparison to the size of the matrix (4). But I can definitely see situations where this may start to fall apart (ie take a long time).

    void SetShips()
    {
        int s = 0;
        while (s < maxships)
        {
            int x = rand() % rows;
            int y = rand() % elements;
            if (matrix[x][y] != '1')
            {
                s++;
                matrix[x][y] = '1';
            }
        }
    }

A better technique may be to have a bag of all the possible values. Then select a random value from the bag (remove the selected item so it can not be chosen again). Then repeat until you have all the values you need.

        std::vector  randomBag;
        for (int i = 0; i < rows; ++i) {
            for (int j = 0; j < elements; ++j) {
                randomBag.push_back(i * elements + j);
            }
        }

        int s = 0;
        while (s < maxships)
        {
            int choice = rand() % randomBag.size();
            int value  = randomBag[choice];
            randomBag.erase(std::begin(randomBag) + choice);
            matrix[value / elements][value % elements] = '1';
        }
    }
    

Try and avoid using system() commands.

    system("CLS");

Sure you will need to use something OS specific but the standard libraries for most OS will be able to do this. Alternatively just print a lot of empty lines :-)


Try and declare one variable per line.

    int pos1, pos2;

It makes it that tiny bit easier for the reader.

Style Hints

Please be careful of these extra empty lines:

void ShowAttackedPlaces()

{

Some function have the opening on the same line:

 void DrawSeparationLine(int x, char c) {

While others have it on the next line:

void GameEndScreen() 
{

Please be consistent.

Personally the '{' is on its own line for namespace/class/function/method but will be on the same line for sub blocks (like after if/for/while).


Naming: You have a consistent style so nothing to complain about. Personally I attempt to have user defined types have an initial capitol letter. While functions and variables have an initial lower case variable. The reason I do that is I quickly want to be able to identify types (over functions) when reading the code.

   stuff(a, b);   // function call
   Stuff(c, d);   // object of type `Stuff` created.

may seem obvious here but when these are function parameters.

   doSomeStuff(stuff(a,b), Stuff(c,d));

\$\endgroup\$
1
\$\begingroup\$

Edward's and Martin's answers covered a lot; I agree with the vast majority of their points and won't repeat them here.

void Show() and void ShowAttackedPlaces() do exactly the same thing except with different array members. As long as the format is identical, you should only implement this printing loop once, and pass it a pointer or reference to the 2D array.

Syntax for declaring a pointer or reference arg to take a 2D array is a bit tricky, but you can sidestep that by using a std::array with manual 2D indexing, or a std::array<std::array<char, elements>, rows>. The latter puts the length constants into the std::array types, so you can use range-for loops.

You can use a typedef or equivalently using Gameboard = std::array<std::array<char, elements>, rows> in your class, so you you can write print_helper(const Gameboard &board); and call it from Show and ShowAttackedPlaces. Since it's tiny, only 4 bytes total, you could actually pass it by value.

This puts the board type in one place, letting you change the details of how you store it later. (It might also work to do something like typedef char Gameboard[2][2]; but I'd have to double check how well that works for passing it by reference.)


Use an enum or something for board meanings

You have one function that does if (matrix[i][j] == 1), but no code ever sets elements to 1. You have some code that sets them to '1' (0x31 in C++ implementations that use ASCII). As others pointed out, magic constants are bad, and even worse when you hard-code them at various points in your program.

static const char BOARD_SHIP = 1; or '1'; are both valid options. Using integer 1 for that and 0 for everything else lets you count ships by just summing the array (std::accumulate or std::reduce), not having to subtract n * '0' or something. OTOH, that choice of algorithm hard-codes the choice of 0 and 1, so maximum separation of choices would come from using std::count to count matches.

Using 0 and 1 would mean your print functions need to do std::cout << board[i][j] + '0';. But that's just printing; the rest of your code can use BOARD_EMPTY / BOARD_SHIP or whatever, and not need to know the specific value.

(Unless you want to do some serious low-level optimization like a chess engine would, and start storing the tiny board as a 4-byte integer (or std::bit_casting it) and counting ships by summing it with x += x>>16; / x += x>>8; / x = (uint8_t)x;, or even a bit-board where you store the 0/1 board as bits of a single integer, so the count of ships is just std::popcount. Which actually also works even with the set bits spread out across bytes.)

Different style choices for constant names can work, like EMPTY or SHIP since these constants are scoped to your class. Probably you want something to highlight the fact that they're constants, not variables. All-caps names are traditionally used for #define macros, and often not for static const int constants, but the names are so short that it feels to me like one should do something to make these board-state constants easy to distinguish from variables.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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