2
\$\begingroup\$

Hi, I am making a Minesweeper game using SFML 2.0.
 I am just a beginner in C++ and started using the framework for 2 weeks now, so it is safe to say I may have made lots of mistake especially when it comes to data types or algorithm implementations.

Before I start, I'll have to explain this two setting variables to avoid confusions:
CELLS_OFFSET = where the grid of cells position will generate. (can be used to center the cells)
CELLS_SIZE = What is the row and columns length. (not the scale value of the cell sprites)

I would really appreciate if you could take your time and review these (sort of messy) codes, and telling any mistake or bad practices^^


Settings.hpp

const unsigned short CELLS_SIZE = 14;
const unsigned short CELLS_BOMBS = 24;

int8_t CELLS_DATA[CELLS_SIZE][CELLS_SIZE];  // The hidden cell values
int8_t CELLS_SDATA[CELLS_SIZE][CELLS_SIZE]; // The cells on top of the hidden cell values

const sf::Vector2i CELLS_OFFSET = sf::Vector2i(202, 16); // The position where the grid of cells will generate

Cell.hpp & Cell.cpp

// Cell.hpp
#include <SFML/Graphics.hpp>

#ifndef CELL_HPP
#define CELL_HPP

class Cell
{
    private:
        sf::Sprite sprite;
        sf::Texture texture;

    public:
        Cell();

        void change(int8_t index);
        void draw(sf::RenderWindow &window, sf::Vector2i position, sf::Vector2i offset);
};

#endif // CELL_HPP

// Cell.cpp
#include "Cell.hpp"

Cell::Cell()
{
    this->texture.loadFromFile("Assets/Cells.png");
    this->sprite.setTextureRect(sf::IntRect(0, 0, 16, 16));
    this->sprite.setTexture(texture);
}

void Cell::change(int8_t index)
{
    this->sprite.setTextureRect(sf::IntRect(16 * index, 0, 16, 16));
}

void Cell::draw(sf::RenderWindow &window, sf::Vector2i position, sf::Vector2i offset)
{
    this->sprite.setPosition(position.x * 32 + offset.x, position.y * 32 + offset.y);
    this->sprite.setScale(2, 2);
    window.draw(this->sprite);
}

Main.cpp

#include <SFML/Graphics.hpp>

#include "Cell.hpp"
#include "Settings.hpp"

int main()
{
    sf::RenderWindow window(sf::VideoMode(852, 480), "Minesweeper", sf::Style::Titlebar | sf::Style::Close);
    window.setVerticalSyncEnabled(true);

    // Define the cell class and randomly seed the bombs
    Cell cell; srand(time(0));

    // Fill Bomb Cells
    for (short x = 0; x < CELLS_BOMBS; x++)
    {
        int8_t &cell_location = CELLS_DATA[rand() % CELLS_SIZE][rand() % CELLS_SIZE];

        if (cell_location == 9)
            x--;
        else
            cell_location = 9;
    }

    // Fill Number Cells
    for (short x = 0; x < CELLS_SIZE; x++) for (short y = 0; y < CELLS_SIZE; y++)
    {
        int8_t &cell_location = CELLS_DATA[x][y];
        if (cell_location == 9) continue; // Skip if this cell is a bomb

        for (int8_t rx = -1; rx < 2; rx++) for (int8_t ry = -1; ry < 2; ry++) {
            // Skip if this cell is out of bounds
            if (x + rx > CELLS_SIZE - 1 || y + ry > CELLS_SIZE - 1 || x + rx < 0 || y + ry < 0 || (rx == 0 && ry == 0)) continue;

            // Increases the number if the neighbor cell has a bomb
            if (CELLS_DATA[x + rx][y + ry] == 9) cell_location++;
        }
    }

    // Game Loop
    while (window.isOpen())
    {
        sf::Vector2i cell_position = sf::Vector2i(std::floor((sf::Mouse::getPosition(window).x - CELLS_OFFSET.x) / 32.0), std::floor((sf::Mouse::getPosition(window).y - CELLS_OFFSET.y) / 32.0)); // I would appreciate if someone optimize this line
        bool cell_bounds = cell_position.x >= 0 && cell_position.x < CELLS_SIZE && cell_position.y >= 0 && cell_position.y < CELLS_SIZE; // Used to check if the cursor position is inside the grid of cells

        int8_t &cell_location = CELLS_DATA[cell_position.x][cell_position.y];
        int8_t &cell_slocation = CELLS_SDATA[cell_position.x][cell_position.y];

        sf::Event event; while (window.pollEvent(event))
        {
            // Cells Input
            if (event.type == sf::Event::MouseButtonPressed && cell_bounds) switch(event.mouseButton.button)
            {
                case sf::Mouse::Left: // Cell Opening
                    if (cell_slocation != 0) continue;
                    cell_slocation = -1;

                    // Flood Fill (BFS)
                    if (cell_location != 0) continue;
                    {
                        std::vector<sf::Vector2i> queue = {cell_position};

                        while (true)
                        {
                            // Get the first queue vector2 for checking it's neighbors
                            sf::Vector2i queue_first_position = sf::Vector2i(queue.at(0).x, queue.at(0).y);

                            // Loop through neigboring cells
                            for (int8_t rx = -1; rx < 2; rx++) for (int8_t ry = -1; ry < 2; ry++) {
                                // The cell must not be on out of bounds
                                if (queue_first_position.x + rx > CELLS_SIZE - 1 || queue_first_position.y + ry > CELLS_SIZE - 1 || queue_first_position.x + rx < 0 || queue_first_position.y + ry < 0 || (rx == 0 && ry == 0)) continue;

                                int8_t &neighbor_location = CELLS_DATA[queue_first_position.x + rx][queue_first_position.y + ry];
                                int8_t &neighbor_slocation = CELLS_SDATA[queue_first_position.x + rx][queue_first_position.y + ry];

                                // The cell must be already opened, and it is not a bomb
                                if (neighbor_slocation == -1 || neighbor_location == 9) continue;

                                // Open the cell
                                neighbor_slocation = -1;

                                // The cell must empty
                                if (neighbor_location != 0) continue;

                                // Add the cell's vector2 to queue
                                queue.push_back(sf::Vector2i(queue_first_position.x + rx, queue_first_position.y + ry));
                            }

                            // Remove the first element in the queue for checking if done filling
                            queue.erase(queue.begin());

                            if (queue.size() <= 0) break;
                        }
                    }

                    break;

                case sf::Mouse::Right: // Cell Flagging
                    if (cell_slocation == 1)
                        cell_slocation = 0;
                    else if (cell_slocation == 0)
                        cell_slocation = 1;
                    break;
            }

            if (event.type == sf::Event::Closed) window.close();
        }

        // Cells Render
        for (short x = 0; x < CELLS_SIZE; x++) for (short y = 0; y < CELLS_SIZE; y++)
        {
            switch(CELLS_SDATA[x][y])
            {
                case 0:  cell.change(10); break;        // Unopened Cell
                case 1:  cell.change(11); break;        // Flagged Cell
                default: cell.change(CELLS_DATA[x][y]); // Numbered Cell
            }

            cell.draw(window, sf::Vector2i(x, y), CELLS_OFFSET);
        }

        // Cells Highlight
        if (cell_bounds && ((cell_slocation == -1 && cell_location != 0) || (cell_slocation != -1)))
        {
            sf::RectangleShape highlight(sf::Vector2f(32, 32));
            highlight.setFillColor(sf::Color(255, 255, 255, 30));
            highlight.setPosition(cell_position.x * 32 + CELLS_OFFSET.x, cell_position.y * 32 + CELLS_OFFSET.y);
            window.draw(highlight);
        }

        window.display();
        window.clear();
    }

    return EXIT_SUCCESS;
}

Regarding about the code, I kept using this data type int8_t to store the value of the cells. I wonder if it that's a good practice or not.


Preview enter image description here

\$\endgroup\$
3
  • \$\begingroup\$ Hi, I just edited my post and added the source code of the Cells.hpp. My apologies! \$\endgroup\$ Commented Oct 23, 2022 at 14:57
  • \$\begingroup\$ Micro-review: you have written uint8_t throughout, but the C++ type is called std::uint8_t (and isn't available on platforms that don't have a type that's exactly 8 bits). You may want to make a using alias for the type you want. \$\endgroup\$ Commented Oct 23, 2022 at 15:18
  • \$\begingroup\$ Thank you, I'm glad you mentioned about that before I start using it everywhere. If that's the case, how would I represent 8 bit data type for storing the cell value in array? \$\endgroup\$ Commented Oct 23, 2022 at 16:40

1 Answer 1

Reset to default
2
\$\begingroup\$

Congratulations making a working game! As you already suspected, there are indeed some things that can be improved:

Rethink the structure of your code

You have had some idea about organizing your code in different files and using classes. However, the vast majority of the code is all in main(). You should really try to add more structure to your code. As a rule of thumb, try not to make functions longer than 20 lines of code, and split them up if they do. Code and functionality that belongs together should be put in its own class. You did this for Cell, but you could also have created a class Board that represents the whole board.

Another trick when writing a function is to write a few lines that describe in a high level what the function does, and then create more functions that implement the low-level details, which in turn call even lower-level functions if necessary. Consider main(): it should look like:

int main()
{
    // 1. Initialize
    // 2. Run game loop
    // 3. Cleanup
}

Now turn this into a real function, and for each step only use a few lines of code, or delegate to a (member) function if it would be more. So for example:

int main()
{
    // 1. Initialize
    sf::RenderWindow window(sf::VideoMode(852, 480), "Minesweeper", sf::Style::Titlebar | sf::Style::Close);
    window.setVerticalSyncEnabled(true);
    Board board;

    // 2. Run game loop
    while (window.isOpen())
    {
         handleEvents(window, board);
         renderGame(window, board);
    }

    // 3. Cleanup
    return EXIT_SUCCESS;
}

For the above code to work, a class Board has to be created that fills bomb and number cells in its constructor, and a function handleEvents() has to be created that reads window events and updates the board accordingly. That function can look like:

void handleEvents(sf::RenderWindow &window, Board &board)
{
    sf::Event event;

    while (window.pollEvent(event))
    {
        switch(event.type)
        {
        case sf::MouseButtonPressed:
            handleMouseButtonPressed(window, board, event);
            break;

        case sf::Event::Closed:
            window.close();
        }
    }
}

In turn, a function handleMouseButtonPressed() has to be created that checks which mouse button is pressed, looks at the coordinates (which are also part of the event.mouseButton, no need to call sf::Mouse::getPosition()), and updates the board.

Note that these small functions are easy to read and understand. They have names that describe what they do, so there is less need to write comments to explain what your code is doing.

Avoid long lines of code

Some of your lines are very long, in particular the one that calculates which cell is under the mouse pointer. Try to reduce the length of lines, either by adding line breaks, by splitting one statement into multiple statements, or by moving the complicated parts into their own function. For example:

sf::Vector2i get_cell_position(int x, int y)
{
    x = (x - CELLS_OFFSET.x) / 32;
    y = (y - CELLS_OFFSET.y) / 32;
    return {x, y};
}
...
sf::Vector2i cell_position = get_cell_position(event.mouseButton.x, event.mouseButton.y);

Also, some lines of code contain multiple statements, like the nested for-loops. A common rule in coding style guides is to never have more than one statement on a single line. This keeps lines shorter, but also avoids surprises if there is another statement on a line, but it is not visible because it was too far to the right.

Make the most of the libraries you use

The above can be further simplified, if you make more use of sf::Vector2i: the latter allows you to do simple arithmetic as well, so you can write:

sf::Vector2i get_cell_position(sf::Vector2i mouse_position)
{
    return (mouse_position - CELLS_OFFSET) / 32;
}
...
sf::Vector2i mouse_position = {event.mouseButton.x, event.mouseButton.y};
sf::Vector2i cell_position = get_cell_position(mouse_position);

Avoid hardcoding and magic numbers

In Settings.hpp, you made an effort to give names to some constants, such as the size of the board and the number of bombs. However, there are many constants that are not named in your code, such as the size of a cell (32), the value that indicates a cell is a bomb (9), and so on. Try to find all these numbers and make constants for them. The advantage is that it makes it easier for someone else reading the code to know what a number represents, and also allows you to change a constant in a single location, without having to go through all the code to search and replace those constants.

Even if you name all the constants, consider whether some things should be constant. It might make sense for the size of a cell, since you want it to match the size of the image you have for it, but what about the window size, or the size of the board or the number of bombs?

Try to see if you can make more things variable. This might require some more extensive changes to your code; for example the CELLS_DATA[][] and CELLS_SDATA[][] arrays no longer compile if CELLS_SIZE is not a constant. You might have to use std::vector or something similar to store cells instead.

Create a struct for the cell's state

You have a class Cell that handles drawing a cell, but there is also the state a cell is in. This is currently stored in two arrays as int8_ts. However, consider creating a struct that holds all the information of the state of a single cell. Also, consider creating an enum that encodes whether a cell is hidden, visible or flagged:

enum class CellState {
    HIDDEN,
    FLAGGED,
    VISIBLE,
};

struct CellData
{
    int8_t neighbours;
    CellState state;
};

Then you only need one array:

CellData cells[CELLS_SIZE][CELLS_SIZE];

Naming things

Some things in your code have names that are confusing. For example, CELLS_SIZE sounds like the size of a cell, but it's actually the size of the board. So BOARD_SIZE would be a better name. CELLS_BOMBS should be renamed to NUMBER_OF_BOMBS, N_BOMBS or BOMB_COUNT.

When adding bombs to the board, you use x as the loop counter. However, x is usually associated with an x-coordinate. I would use i instead, it's the well-known name for a generic loop index.

Make sure names are clear, concise and unambiguous.

The first click should be safe

Many versions of Minesweeper ensures that the first time you click on the board, there is no bomb under the mouse. The reason is that it doesn't make sense to put a flag as the first move, and it would be perceived as unfair as the first move you make could cause you to lose without any mistake from the player.

There are several ways to implement this; if the first click is on a bomb, you could remove the bomb or place it elsewhere, and fix up the neighbor counts. Alternatively, you can delay placing bombs until the player has clicked somewhere.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.