Point class:

• Consider using an existing math library (e.g. glm). It can be more complicated to set up initially, but provides things like simple math operators (e.g. p1 += p2) out of the box.

• Don't write setters and getters for all the member variables. Just make the variables public.

• Using aggregate initialization makes it unnecessary to define a constructor taking each member:

  Point p{ 1, 2, 3 };

• Use = default for the equality operator (C++ 20 onwards).

These changes mean we need much less code:

struct Point
{
    int x, y, z;

    friend bool operator==(Point const& a, Point const& b) = default;
};

Player class:

• Again, don't hide member variables behind getters and setters unless there's a reason to do so. Making location public saves us a huge amount of code.

• Since we're not imposing any extra conditions on health (such as making sure it's positive, or performing any other action here when the player takes damage), the health member can be public too.

• The int direction argument sent to move appears to be an enum. We should use an enum class for it so that we gain type-safety, and not pass it around as an integer.

So the Player class could perhaps be:

enum class Direction { UP, DOWN, LEFT, RIGHT };

struct Player
{
    Point location;
    int health;

    void Move(Direction dir);
};

Since location is now public, and all Move does is increment or decrement the relevant coordinate, we could simplify this further by removing the dependency of Player on Direction. i.e. we could remove the Move function as well, and do that logic elsewhere (since the directions are listed as KEY_UP, KEY_DOWN etc., somewhere closer to fetching the key input might be appropriate).

GameField class:

• Consider creating a Board class (or at least a typedef), as it will make the code easier to read (std::vector<Board> vs std::vector<std::vector<std::vector<char>>>

• It's common practice to store grids as a flat vector, rather than as a 2 dimensional vector. So instead of std::vector<std::vector<char>>, we could represent the grid as std::vector<char> with size width * height. When we need to access a value at coordinates (x, y), we calculate the corresponding index into this vector as board[y * width + x].

  This way, we only have to do a single memory allocation, and have much better cache locality. We can hide all of this (along with the width and height) inside the Board class suggested above.

  Note that if we made this class slightly more generic (say, a template<class T> class Grid { ... };) we could use a Grid<char> for our boards, and a Grid<Board> for our board list!

• GetBoard(), GetBoardList() and GetInitialBoardList() should return by const&, rather than by value. This can save us from copying a lot of extra data (e.g. in MoveBoard in Rogue.cpp, where you only need the size of the board list).

• Don't forget to mark member functions that don't change any member state (variables) as const. e.g. int GetBoardLength() const.

• Note that GetPlayer() would copy the player, even though GameField stores it by reference. This might lead to confusion. Since the function isn't used, it could just be deleted.

• GenRandomNumber seems to be reseeding the random number generator with every call. This is unnecessarily expensive. It would be better to store the std::mt19937 variable (the random number generator state) somewhere and reuse it.

• It doesn't look like the Gamefield header file needs all those includes. Most of them should be in the cpp file.

• GenNewPoint leaks memory. It keeps allocating new Points as mineToAdd, and never deletes them. There's no need to use heap allocation here. You should add an assignment operator to your Point class Point& operator=(Point const& other) = default. Or the compiler will generate one for you if you simplify the Point class to remove the unnecessary constructors, as suggested above.

Rogue.cpp:

• std::vector<Point> mines = field.GetMines(); Do we really want to copy the vector here? or should mines be a std::vector<Point> const&.

• Rather than providing Reset functions, and reusing existing classes, it's probably simpler and safer to reassign the actual variables:

  player = Player(3);
  gameField = GameField(5, 5, 3, 3, player);
  gameField.PrintBoard();
  

  This way there's no risk of forgetting to add a member variable to the Reset function if you change something inside one of the classes.

  It would also be a good idea to make those "magic numbers" named constants, e.g. const int PlayerStartHealth = 3;

• The movement / bounds checking seems... complicated. I don't have time to look at it right now... I'll try to get back to this later if no-one else comments on this aspect.