-1

Sorry if the code is too long. I just wanted to give a detailed description of the situation I am facing

I'm coding a game called Battle Ship. The following code is a simpler version, I did eliminate all the unnecessary logic, cause I just want to indicate the problem with the magic numbers.

Here are my struct and enum

// the dimension of the 10 + 2 game grid for detection of the neighboring ship 
// (its usage comes later in the game, not at this stage)
const int SIZE = 12;

// the number of warships
const int NBSHIP = 5;

string ships[NBSHIP] = {"Carrier", "Cruiser", "Destroyer", "Submarine", "Torpedo" };

// available warships and their size on the grid
enum Ship {
  CARRIER=5,
  CRUISER=4,
  DESTROYER=3,
  SUBMARINE=3,
  TORPEDO=2,
  NONE=0
};

// the different states that a grid cell can take and their display
enum State {
  HIT='x',
  SINK='#',
  MISS='o',
  UNSHOT='~'
};

// a grid cell
// the ship present on the space
// the state of the box
struct Cell {
  Ship ship;
  State state;
};

// the player with
// his name
// his score to determine who lost
// his game grid
struct Player {
  string name;
  int score;
  Cell grid[SIZE][SIZE];
};

// the coordinates of the cell on the grid
// its column from 'A' to 'J'
// its line from 1 to 10
struct Coordinate {
  char column;
  int line;
};

// the placement of the ship on the grid
// its coordinates (E5)
// its direction 'H' horizontal or 'V' vertical from the coordinate
struct Placement {
  Coordinate coordi;
  char direction;
};

Basically, at the beginning of the game, I have to initialize a grid with the appropriate state for each cell (in this case, UNSHOT and NONE). Then I have to display the grid and start placing the ships. The weird thing here is I have to use "magic numbers" to place the ships in the correct position according to the player's input. But I don't even know why I need it as well as how to get rid of it.

Utilization of magic number appears in placeShip function.

void initializeGrid (Cell aGrid[][SIZE])
{
    for (int i = 0; i < SIZE - 2; i++)
    {
        for (int j = 0; j < SIZE - 2; j++)
        {
          aGrid[i][j].ship = NONE;
          aGrid[i][j].state = UNSHOT;
        }
    }
}

void displayGrid(Player aPlayer)
{
    cout << endl;
    cout << setw(10) << aPlayer.name << endl;

    // Letter coordinates
    char a = 'A';
    cout << "  "; 
    for (int i = 0; i < SIZE - 2 ; i++)
    {
        cout << " " << char (a+i);
    }
    cout << endl;

    // Number coordinates
    for (int i = 0; i < SIZE - 2; i++)
    {
        // Player
        if(i + 1 >= 10) // To align the first column
            cout << i + 1;
        else
            cout << " " << i + 1;

        for (int j = 0; j < SIZE - 2; j++)
        {
            if (aPlayer.grid[i][j].ship) // Check if there are ships
                cout << " " << (aPlayer.grid[i][j].ship);
            else
                cout << " " << char (aPlayer.grid[i][j].state);
        }
        cout << endl;
    }
}

void placeShip(Cell aGrid[][SIZE], Placement aPlace, Ship aShip)
{
    if (aPlace.direction == 'h' || aPlace.direction == 'H')
    {
        for (int i = 0; i < aShip; i++) // To change the value of a cell according to the size of the boat
        {
          // Utilization of magic number
          aGrid[aPlace.coordi.line - 9][aPlace.coordi.column + i - 1].ship = aShip; // Horizontal so we don't change the line
        }
    }
    else if (aPlace.direction == 'v' || aPlace.direction == 'V')
    {
        for (int i = 0; i < aShip; i++)
        {
          // Utilization of magic number
          aGrid[aPlace.coordi.line + i - 9][aPlace.coordi.column - 1].ship = aShip; // Vertical so we don't change the column
        }
    }
}

void askPlayerToPlace(Player& aPlayer)
{
    Ship battleships[NBSHIP] = { CARRIER, CRUISER, DESTROYER, SUBMARINE, TORPEDO};

    for (int i = 0; i < NBSHIP; i++)
    {
        string stringPlace;
        string stringShip = ships[i];
        Ship aShip = battleships[i];
        string inputDirection;
        Coordinate coordi;

        cout << "\n" << aPlayer.name << ", time to place your carrier of length " 
        << to_string(battleships[i]) 
        << " (" << ships[i] << ")\n" 
        << "Enter coordinates (i.e. B5): ";
        cin >> stringPlace;
        coordi = { stringPlace[0], int(stringPlace[1]) - 48 };

        cout << "Direction: ";
        cin >> inputDirection;
  
        Placement aPlace = {
            coordi,
            inputDirection[0]
        };

        placeShip(aPlayer.grid, aPlace, aShip);
        displayGrid(aPlayer);
    }
}

int main()
{
    Player aPlayer;

    cout << "Player's name: ";
    getline (cin, aPlayer.name);

    initializeGrid(aPlayer.grid);
    displayGrid(aPlayer);

    askPlayerToPlace(aPlayer);

    return 0;
}
Nhung
  • 1
  • 3
  • What's the problem? Some reasonably named constant definitions, _et voilá_. – πάντα ῥεῖ Dec 13 '20 at 10:43
  • 1
    Sometimes, magic numbers are fine if it serves just one specific purpose. – justANewb stands with Ukraine Dec 13 '20 at 10:44
  • FYI you should avoid the use of UPPERCASE for variable names or enums. Uppercase is generally used for macros and there is a risk of conflicts that are hard to debug/find. – t.niese Dec 13 '20 at 10:50
  • @πάνταῥεῖ In fact, those are given by my teachers, this is our mid-term project and we don't have the right to modify the constant, struct and enum. And can you clarify your answer? Why are those constant definitions causing me to use magic numbers? – Nhung Dec 13 '20 at 10:50
  • @justANewbie yes, I know but in this case, it got me into trouble coding the game later on: / – Nhung Dec 13 '20 at 10:56
  • @Nhung I didn't say you should modify the existing ones, but ***define new ones***. If you can tell that e.g. `2` which appears several times in calulations, is the _global margin_ constant for some position calculation, define another constant `const int POS_MARGIN = 2;` and use that in _your_ code. – πάντα ῥεῖ Dec 13 '20 at 10:56
  • @t.niese _"Uppercase is generally used for macros ..."_ Source please? I tend to disagree, all caps for constant definitions are completely fine. – πάντα ῥεῖ Dec 13 '20 at 11:28
  • @πάνταῥεῖ if `#define` is used in libraries then these are mostly written in uppercase, there are some exceptions like microsoft headers in which the e.g. have defines for `min` and `max` which is even worse because then you get compile errors when using `std::min` and `std::max` if you forgot to define `NOMINMAX`. So using uppercase in combination with other libraries increase the risk of such conflicts that in the best case result in compile errors, or in the worst case unexpected results. – t.niese Dec 13 '20 at 11:43
  • Magic numbers are a problem when you have multiple numbers that depend on each other, if you use the same value multiple times but with different meanings. Or when it is not clear from reading the code what the meaning of those values is. E.g. does `10` od `if(i + 1 >= 10)` depend on `SIZE`. Does the `9` of `aPlace.coordi.line - 9` depend on `SIZE`. Will your code still work if you change the value of `SIZE` to something different to `12`? – t.niese Dec 13 '20 at 12:11
  • @t.niese I updated my question to clarify the meaning of value **10** in `if (i + 1 >=10)`. And no, it does not depend on `SIZE`. On the other hand, the value **9** in `aPlace.coordi.line - 9` and **-1** in `aPlace.coordi.column - 1` change according to `SIZE`. For example if I change `SIZE` to **14**, I have to change the code to `aPlace.coordi.line - 8` and `aPlace.coordi.column + 1`. – Nhung Dec 13 '20 at 13:46
  • I also tried changing the values of `SIZE` from 11 to 17, and with that modification, I had to change those "magic numbers" as well. But I still can't figure out the meaning of those random values and why do I need them to display the ships in the correct position. – Nhung Dec 13 '20 at 13:48

1 Answers1

1

The weird thing here is I have to use "magic numbers" to place the ships in the correct position according to the player's input.

If these are numbers, which tend to appear frequently you should define another constant for these like:

const int GRID_SIZE_MARGIN = 2;

and use it

void initializeGrid (Cell aGrid[][SIZE]) {
    for (int i = 0; i < SIZE - GRID_SIZE_MARGIN; i++) {
        for (int j = 0; j < SIZE - GRID_SIZE_MARGIN; j++) {
          aGrid[i][j].ship = NONE;
          aGrid[i][j].state = UNSHOT;
        }
    }
}

If there are other numbers, which appear in calculations, but can't be reasonably named, it's OK to leave the numeric values. I'll try to give you an example:

The formula to calculate degrees from radians is

deg = rad * 180° / π

For π we have a constant definition (PI), but is 180 a magic number which deserves a constant definiton?
How should we name it? HUNDREDEIGHTY_DEGREES??

So it's not always reasonable to get rid of numeric constants appearing in code.


But I don't even know why I need it as well as how to get rid of it.

That's probably part ot the task, to find out.

πάντα ῥεῖ
  • 1
  • 13
  • 116
  • 190