3
\$\begingroup\$

this is the second version of the Terminal Based Game I'm working on thanks to @user673679 for the suggestions, I did not implemented everything you said but it was really fun working on your corrections and ideas.
so a brief walkdown on what the game is about, I'm representing each entity with its own character: (floor - #, player - @, point - $, portal entrance - ^, portal exit - >).
the player can walk around the game field, the field is split into boards of equal size, and the player sees only the boards they visited, also there are invisible mines that will cause you to lose the game if you accidentally step on it, if a player collects a point he gets well a score point and theirs vision of the boards resets only to the one that the players stands on and a new mine is added.
that's the gist of the game, I will walkthrough the code and try to explain as best as I can, and there are (I hope) pretty descriptive comments on the code I left, you can also see my previous post about this project if you want.

Point
this only has a few lines of code and it is only a header file, it is used as a coordinates system to place and manipulate objects like the player, portals and other things.

header

#pragma once

struct Point // represents the current location of a game object, x and y are in relation to the board
{
public:
    int boardNum;
    int x;
    int y;
    
    bool operator == (const Point& point)
    {
        return boardNum == point.boardNum && x == point.x && y == point.y;
    }
};

Board
this class represents a single board, I used a vector instead of a matrix buy I set it up to behave like a matrix.

header

#pragma once
#include <vector>
#include <iostream>

class Board
{
public:
    int length;
    int width;
    std::vector<char> board;
    Board(int length, int width);

    char GetCh(int x, int y);
    void SetCh(int x, int y, char ch);



    int Size();
    void PrintRow(int y);
    void PrinBlankRow();
};

cpp

#include "Board.h"

Board::Board(int length, int width) : length(length), width(width)
{
    for (int i = 0; i < length * width; i++)
        board.push_back('#');
}

char Board::GetCh(int y, int x)
{
    return board.at(y * length + x);
}

void Board::SetCh(int y, int x, char ch)
{
    board.at(y * length + x) = ch;
}

int Board::Size()
{
    return length * width;
}


void Board::PrintRow(int y)
{
    for (int i = 0; i < length; i++)
        std::cout << board.at(y * length + i);
}
void Board::PrinBlankRow()
{
    for (int i = 0; i < length; i++)
        std::cout << ' ';
}

here I have methods to get a char from the board, set a chare on the board and print statements.
Player
this class represents the player and is mainly used to store theirs location and health although for now the mines one shot so I'm not really using the health here.

header

#pragma once
#include "Includes.h"
#include "Point.h"

#define PLAYER_HEALTH 3
class Player
{
private:
    Point currentLocation;
    int health;

public:
    Player();
    ~Player();

    int GetX();
    int GetY();
    void SetX(int x);
    void SetY(int y);
    void SetCurrentBoard(int boardNum);
    void SetCurrentLocation(Point& newLocation);
    int GetCurrentBoard();
    int GetHealth();
    Point GetCurrentLocation();

    void TakeDamage(int damageAmount);
    

    void Move(int direction);

    void ResetPlayer();

};

cpp

#include "Player.h"

Player::Player() 
{
    currentLocation = { 0, 1, 1 };
    health = PLAYER_HEALTH;
}

Player::~Player()
{
     
}

int Player::GetX() 
{
    return currentLocation.x;
}
int Player::GetY() 
{
    return currentLocation.y;
}
void Player::SetX(int _x) 
{
    currentLocation.x = _x;
}
void Player::SetY(int _y) 
{
    currentLocation.y = _y;
}
void Player::SetCurrentLocation(Point& newLocation)
{
    currentLocation = newLocation;
}
int Player::GetCurrentBoard() 
{
    return currentLocation.boardNum;
}
Point Player::GetCurrentLocation()
{
    return currentLocation;
}

void Player::SetCurrentBoard(int field)
{
    currentLocation.boardNum = field;
}
int Player::GetHealth()
{
    return health;
}

void Player::TakeDamage(int damageAmount)
{
    health -= damageAmount;
}

void Player::Move(int direction) //changes the location based on the direction
{
    switch (direction)
    {
    case KEY_UP:
        currentLocation.y = (currentLocation.y - 1);
        break;
    case KEY_DOWN:
        currentLocation.y = (currentLocation.y + 1);
        break;
    case KEY_RIGHT:
        currentLocation.x = (currentLocation.x + 1);
        break;
    case KEY_LEFT:
        currentLocation.x = (currentLocation.x - 1);
        break;
    default:
        break;
    }
}

void Player::ResetPlayer()
{
    currentLocation.boardNum = 0;
    currentLocation.x = 1;
    currentLocation.y = 1;
    health = PLAYER_HEALTH;
}

here the gist of the methods here are to get and set the player fields and some like reset the player when the game restarts and move the player in a direction
GameField
here is most of the logic on printing the field in the right order as well as being responsible for placement of game objects and their behaviour I don't know if I should make at least a struct for each of them but for now this class stores the score points, the mines, the player and the portals as well as a vector of boards.

header

#include "Board.h"
#include "Includes.h"
#include <set>
#include <vector>
#include <algorithm>
#include <iostream>





//typedef std::vector<std::vector<char>> Board;
class GameField
{
private:
    int boardLength; // board and field dimansions
    int boardWidth;
    int fieldLength;
    int fieldWidth;

    Board board; // a single board
    std::vector<Board> boardList; // list 
    std::vector<Board> initialBoardList; // a representaion of the field but without the player @ absrating the point he stands on

    std::vector<Point> portalsIn;
    std::vector<Point> portalsOut;
    std::vector<Point> allPoints;
    Point collectibleLocation; // the board and coordinate for a $ to collect
    std::vector<Point> mines; // the board and coordinate for a mine to appear

    std::set<int> visibleBoards; // board numbers that are visible to the player
    Player& player;

public:
    GameField(int boardWidth, int boardLength, int fieldLength, int fieldWidth, Player& player);
    ~GameField();

    void UpdateField(); //updates the board based on changes made
    void PrintField();
    void TeleportToRandPlace();

    const Board& GetBoard();
    const std::vector<Board>& GetBoardList();
    const std::vector<Board>& GetInitialBoardList();
    void SetPreviousBoard(int boardNum)
    {
        boardList.at(boardNum) = initialBoardList.at(boardNum);
    }
    // getters
    int GetBoardLength();
    int GetBoardWidth();
    int GetFieldLength();
    int GetFieldWidth();
    Player GetPlayer();
    Point GetCollectiobleLocation();
    std::vector<Point> GetMines();  
    std::vector<Point> GetPortalsIn();
    std::vector<Point> GetPortalsOut();

    void AddVisibleBoard(int board); // adds a new visible board the player just visited

    const Point& GenPoint();
    void GenScorePoint();
    void GenMinePoint();
    void GenPortals();

    void ResetVisibleBoards();
};

cpp

#include "GameField.h"

GameField::GameField(int boardWidth, int boardLength, int fieldLength, int fieldWidth, Player& player) :
    player(player), boardLength(boardLength), boardWidth(boardWidth), fieldLength(fieldLength), fieldWidth(fieldWidth), board(boardLength, boardWidth)
{
    for (int i = 0; i < fieldLength * fieldWidth; i++)
    {
        boardList.push_back(board);
    }

    initialBoardList = boardList;
    GenScorePoint();
    GenPortals();
    boardList.at(collectibleLocation.boardNum).SetCh(collectibleLocation.y, collectibleLocation.x, '$'); // puts the $ on the random locationg provided bu GenNewPoint()
    visibleBoards.insert({ 0 }); // the first visible board is 0 thats where the player starts
    boardList.at(0).SetCh(player.GetY(), player.GetX(), PLAYER_ICON); // places an indicator for the player
}

GameField::~GameField()
{
}



const Point& GameField::GenPoint() // generates a random point that does not collide with other points
{
    Point randPoint;
    do
        randPoint = { GenRandomNumber(0, fieldLength * fieldWidth - 1), GenRandomNumber(0, boardLength - 1), GenRandomNumber(0, boardWidth - 1) };
    while (std::count(allPoints.begin(), allPoints.end(), randPoint) || randPoint == player.GetCurrentLocation());
    allPoints.push_back(randPoint);
    return randPoint;
}
void GameField::GenScorePoint() // generate a new $
{
    initialBoardList.at(collectibleLocation.boardNum).SetCh(collectibleLocation.y, collectibleLocation.x, '#'); // erases the previous $ point that the player collected
    collectibleLocation = GenPoint();
    initialBoardList.at(collectibleLocation.boardNum).SetCh(collectibleLocation.y, collectibleLocation.x, '$'); //places the $ on the field
    GenMinePoint();

}
void GameField::GenMinePoint() // generates a new mine
{
    Point newMine = GenPoint();
    mines.push_back(newMine);
    
}
void GameField::GenPortals() // generates pairs of portals based on the TELEPORTER_AMOUNT
{
    for (int i = 0; i < TELEPORTER_AMOUNT; i++)
    {
        Point inPortal = GenPoint();
        Point outPortal = GenPoint();
        portalsIn.push_back(inPortal);
        portalsOut.push_back(outPortal);
        initialBoardList.at(inPortal.boardNum).SetCh(inPortal.y, inPortal.x, '^');
        initialBoardList.at(outPortal.boardNum).SetCh(outPortal.y, outPortal.x, '>');

    }
}

void GameField::TeleportToRandPlace()
{
    int randPortalOut = GenRandomNumber(0, TELEPORTER_AMOUNT - 1); // chooses at random a teleporter to teleport to
    player.SetCurrentLocation(portalsOut.at(randPortalOut)); // changes the coordinates of the player to that of the exit portal
    AddVisibleBoard(player.GetCurrentBoard()); // adds the board the exit portal in as a visible board to the player
}

void GameField::ResetVisibleBoards()
{
    visibleBoards.clear(); //resets visible board memory
    visibleBoards.insert(player.GetCurrentBoard()); // adds the current player board back to memory
}

void GameField::AddVisibleBoard(int board)
{
    visibleBoards.insert({ board }); // adds a new visible board a player can see happens when a player visits a new board
}

void GameField::UpdateField()
{
    for(int i = 0; i < boardList.size(); i++)
        boardList.at(i) = initialBoardList.at(i);

    boardList.at(player.GetCurrentBoard()).SetCh(player.GetY(), player.GetX(), PLAYER_ICON); // place the player again at the new position
}

int GameField::GetBoardLength()
{
    return boardLength;
}
int GameField::GetBoardWidth()
{
    return boardWidth;
}

void GameField::PrintField()
{
    UpdateField(); // updates the board if any changes were made
    for (int i = 0; i < fieldWidth; i++) // runs on the field and prints the ones that the player visited and space on the one that the player did not
    {
        for (int j = 0; j < boardWidth; j++)
        {

            for (int k = 0; k < fieldLength; k++)
            {
                int index = i * fieldLength + k; // calculating the index of the current board im on
                if (!std::binary_search(visibleBoards.begin(), visibleBoards.end(), index))
                {
                    boardList.at(index).PrinBlankRow();
                }
                else
                    boardList.at(index).PrintRow(j);
            }
            std::cout << '\n';
        }
    }

}
//Getters
int GameField::GetFieldLength()
{
    return fieldLength;
}
int GameField::GetFieldWidth()
{
    return fieldWidth;
}
std::vector<Point> GameField::GetMines()
{
    return mines;
}
Point GameField::GetCollectiobleLocation()
{
    return collectibleLocation;
}

const Board& GameField::GetBoard()
{
    return board;
}
const std::vector<Board>& GameField::GetBoardList()
{
    return boardList;
}
const std::vector<Board>& GameField::GetInitialBoardList()
{
    return initialBoardList;
}
Player GameField::GetPlayer()
{
    return player;
}
std::vector<Point> GameField::GetPortalsIn()
{
    return portalsIn;
}
std::vector<Point> GameField::GetPortalsOut()
{
    return portalsOut;
}

here I have fields like the matrix of the Board class I represent it also as a vector, the player location, and vectors of locations of different game objects, I also have a vector of all the Points that are in use so when I generate an object it will not collide with other ones. the constructor is pretty straight forward in my opinion I just fill the field with boards according to the provided length and width of the field, I also have an initialBoardList in where I store the exact same board but without the player to know if a player is standing on something, I might change it to just a variable that changes when a player moves to store the what they stand on but I'm to lazy for that now.
GenPoint() generates a random Point for different objects to use, it generates a random point until it no longer finds one in the vector of all points that's how I avoid objects spawning at the same point.
then I have 3 different generators for the portals mines and score points the use GenPoint().
TeleportToRandPlace() teleports to a random exit portal when the player stands on an entrance portal.
UpdateField() updates the field each time it is printed to the initialBoardList and then sets the player location to '@'.
PrintField() runs UpdateField() first then goes into a for loop that goes through the boards top to bottom, inside I have a for loop that runs on one line of a board, inside that loop I have a for loop that runs on each board from left to right, if the player visited the board then it prints the line of that board and blank spaces if they did not, in the end it prints only the boards the player visited so they can make snake like shapes when moving, or even see boards in different non connected places if the player teleports.
Rogue Rogue is the place where I have my main function also one that checks for bounds, one that checks if the player moved boards if so set their x and y to the right coordinates, one that deletes everything from the screen so I can print the next frame in place and the last one deletes the blinking cursor.
cpp

#include <stdlib.h>
#include <iostream>
#include <conio.h>
#include<windows.h>
#include "GameField.h"
#include "Includes.h"
#include "MiscFunctions.h"



void cls();
bool CheckIfMovementLegal(GameField& field, Player& player, int direction);
bool MoveBoard(GameField& field, Player& player, int direction);
void ShowConsoleCursor(bool showFlag);

int main()
{
    ShowConsoleCursor(false);

    Player player;
    int playerHealth = player.GetHealth();
    GameField *field = new GameField(BOARD_WIDTH, BOARD_LENGTH, FIELD_LENGTH, FIELD_WIDTH, player);
    field->PrintField();
    
    int points = 0;
    while (true)
    {
        cls();
        field->PrintField();

        std::vector<Point> portalsIn = field->GetPortalsIn();
        if (std::count(portalsIn.begin(), portalsIn.end(), player.GetCurrentLocation()))
        {
            field->TeleportToRandPlace();
            cls();
            field->PrintField();
        }
        
        std::vector<Point> mines = field->GetMines();
        if (std::count(mines.begin(), mines.end(), player.GetCurrentLocation())) // checks if a player stepped on a mine
            player.TakeDamage(3);

        if (player.GetHealth() <= 0)
        {
            cls();
            std::cout << "You finished with " << points << " points. GG!" << '\n';
            std::cout << "type 'r' to restart or 'q' to quite" << '\n';
            std::string userInput;
            std::cin >> userInput;

            if (userInput == "r")
            {
                /*
                player.ResetPlayer(playerHealth);
                field.GenNewPoint();
                field.ResetVisibleBoards();
                */
                delete field;
                player.ResetPlayer();
                field = new GameField(BOARD_WIDTH, BOARD_LENGTH, FIELD_LENGTH, FIELD_WIDTH, player);
                cls();
                field->PrintField();
            }
            else
                break;

        }
        if (field->GetCollectiobleLocation() == player.GetCurrentLocation()) // check if a player picked a $
        {
            field->GenScorePoint(); // generate new $ and mine
            field->ResetVisibleBoards(); // remove memory of visible boards
            cls();
            field->PrintField();
            points++;
        }
        int direction = _getch();
        if (CheckIfMovementLegal(*field, player, direction)) //checks if movement is legal then if need to be board switch and then moves player in direction
        {
            if (!MoveBoard(*field, player, direction))
                player.Move(direction);
            cls();
            field->PrintField();
        }

    }

    return 0;
}

bool MoveBoard(GameField& field, Player& player, int direction)
{
    int width = field.GetBoardList().at(player.GetCurrentBoard()).width;
    int length = field.GetBoardList().at(player.GetCurrentBoard()).length;
    int previousBoardNum = player.GetCurrentBoard();
    bool isMovedToNewBoard = false;
    switch (direction) // checks if player on the edge of the current boards if so then move to the next board in that direction
    {
    case KEY_UP:
        if (player.GetY() == 0)
        {
            isMovedToNewBoard = true;
            field.SetPreviousBoard(previousBoardNum);
            int newBoardNum = player.GetCurrentBoard() - field.GetFieldLength();
            field.AddVisibleBoard(newBoardNum);
            player.SetCurrentBoard(newBoardNum);
            player.SetY(width - 1);
        }
        break;

    case KEY_DOWN:
        if (player.GetY() == width - 1)
        {
            isMovedToNewBoard = true;
            field.SetPreviousBoard(previousBoardNum);
            int newBoardNum = player.GetCurrentBoard() + field.GetFieldLength();
            field.AddVisibleBoard(newBoardNum);
            player.SetCurrentBoard(newBoardNum);
            player.SetY(0);
        }
        break;

    case KEY_RIGHT:
        if (player.GetX() == length - 1)
        {
            isMovedToNewBoard = true;
            field.SetPreviousBoard(previousBoardNum);
            int newBoardNum = player.GetCurrentBoard() + 1;
            field.AddVisibleBoard(newBoardNum);
            player.SetCurrentBoard(newBoardNum);
            player.SetX(0);
        }
        break;

    case KEY_LEFT:
        if (player.GetX() == 0)
        {
            isMovedToNewBoard = true;
            field.SetPreviousBoard(previousBoardNum);
            int newBoardNum = player.GetCurrentBoard() - 1;
            field.AddVisibleBoard(newBoardNum);
            player.SetCurrentBoard(newBoardNum);
            player.SetX(length - 1);
        }
        break;
    default:
        break;
    }

    return isMovedToNewBoard;
}

bool CheckIfMovementLegal(GameField& field, Player& player, int direction)
{
    int fieldLength = field.GetFieldLength();
    int fieldWidth = field.GetFieldWidth();
    int currentBoard = player.GetCurrentBoard();
    int boardWidth = field.GetBoard().width;
    int boardLength = field.GetBoard().length;
    bool flag = true;
    switch (direction) // checks if player on the edge of the field if so check if player on the edge of current board (boards are set in a single linear vector)
    {
    case KEY_UP:
        for (int i = 0; i < fieldLength; i++)
        {
            if (i == currentBoard)
            {
                flag = false;
                break;
            }
        }

        if (flag)
            return true;
        if (player.GetY() > 0)
            return true;

        break;

    case KEY_DOWN:
        for (int i = (fieldLength * fieldWidth) - 1; i > (fieldLength * fieldWidth) - fieldLength - 1; i--)
        {
            if (i == currentBoard)
            {
                flag = false;
                break;
            }
        }
        if (flag)
            return true;
        if (player.GetY() < boardWidth - 1)
            return true;

        break;
    case KEY_LEFT:
        for (int i = 0; i < fieldLength * fieldWidth; i += fieldLength)
        {
            if (i == currentBoard)
            {
                flag = false;
                break;
            }
        }
        if (flag)
            return true;
        if (player.GetX() > 0)
            return true;
        break;
    case KEY_RIGHT:
        for (int i = fieldLength - 1; i < fieldLength * fieldWidth; i += fieldLength)
        {
            if (i == currentBoard)
            {
                flag = false;
                break;
            }
        }
        if (flag)
            return true;
        if (player.GetX() < boardLength - 1)
            return true;
        break;
    default:
        break;
    }

    return false;
}

void ShowConsoleCursor(bool showFlag)
{
    HANDLE out = GetStdHandle(STD_OUTPUT_HANDLE);

    CONSOLE_CURSOR_INFO cursorInfo;

    GetConsoleCursorInfo(out, &cursorInfo);
    cursorInfo.bVisible = showFlag; // set the cursor visibility
    SetConsoleCursorInfo(out, &cursorInfo);
}

void cls()
{
    // Get the Win32 handle representing standard output.
    // This generally only has to be done once, so we make it static.
    static const HANDLE hOut = GetStdHandle(STD_OUTPUT_HANDLE);

    CONSOLE_SCREEN_BUFFER_INFO csbi;
    COORD topLeft = { 0, 0 };

    // std::cout uses a buffer to batch writes to the underlying console.
    // We need to flush that to the console because we're circumventing
    // std::cout entirely; after we clear the console, we don't want
    // stale buffered text to randomly be written out.
    std::cout.flush();

    // Figure out the current width and height of the console window
    if (!GetConsoleScreenBufferInfo(hOut, &csbi)) {
        // TODO: Handle failure!
        abort();
    }
    DWORD length = csbi.dwSize.X * csbi.dwSize.Y;

    DWORD written;

    // Flood-fill the console with spaces to clear it
    FillConsoleOutputCharacter(hOut, TEXT(' '), length, topLeft, &written);

    // Reset the attributes of every character to the default.
    // This clears all background colour formatting, if any.
    FillConsoleOutputAttribute(hOut, csbi.wAttributes, length, topLeft, &written);

    // Move the cursor back to the top left for the next sequence of writes
    SetConsoleCursorPosition(hOut, topLeft);
}

each action I run cls() and PrintField() also on the start of the while loop, I check for inputs if I got one I move the player in the input direction I set it up for the arrow keys.
for last I have MiscFunctions and Includes the first is functions that do not belong in a single file but I use them and includes are names for variables that I use accords the game so I wont have "magic numbers" and instead give them meaning for more readability.
MiscFunctions

cpp

 #include "MiscFunctions.h"

int GenRandomNumber(int start, int end)
{
    std::random_device rd; // obtain a random number from hardware
    std::mt19937 gen(rd()); // seed the generator
    std::uniform_int_distribution<> distr(start, end); // define the range
    return distr(gen);
}

the header includes just the declaration of this function.
Includes

header

#pragma once

#define KEY_UP 72
#define KEY_DOWN 80
#define KEY_LEFT 75
#define KEY_RIGHT 77

#define TELEPORTER_AMOUNT 3

#define BOARD_WIDTH 5
#define BOARD_LENGTH 5
#define FIELD_LENGTH 5
#define FIELD_WIDTH 3

#define PLAYER_ICON '@'
#define PORTAL_ICON '^'

and that's it, I'd appreciate any advice, corrections and ideas, Thanks!

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

I couldn't read everything, but I have several things to say:

  1. Beautiful usage of std::count here: while (std::count(allPoints.begin(), allPoints.end(), randPoint) || randPoint == player.GetCurrentLocation());
  2. I believe that the board's vector type shouldn't be chars, but integers or enum values. It'll separate the gui details from the logical details. I think it's better to use chars only when you print.
  3. I believe you can do that without storing twice a vector of Board. You can do it without using initialBoardList.
  4. I personally don't like using #define in cpp context. You can use globals like inline constexpr int var = 5;. Macros and definition can be used for other things like separations between treatments for different operation systems, or for reflection design pattern when there is no other way with metaprogramming.

Overall great job! Good luck!

\$\endgroup\$
2
\$\begingroup\$

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

Use the required #includes

Within GameField.h, the code refers to Point and Player but does not include the corresponding #include files.

Eliminate function prototypes by ordering

If you put the cls, CheckIfMovemenLegal, MoveBoard, and ShowConsoleCursor implementations above main in the source code, you don't need the function prototypes.

Don't return a reference to a local object

In the GameField::GenPoint() function, the function is declared like this:

const Point& GameField::GenPoint()

That means it's returning a const reference, but the code is actually implemented like this:

{
    Point randPoint;
    // code omitted
    return randPoint;
}

The problem is because randPoint is declared within the function, it's a local variable, so the destructor is called when the function exits, and now the caller has an invalid reference. In this case just the object rather than a reference. In other words, change the declaration to this:

Point GameField::GenPoint()

See F.43 for further explanation.

Fix spelling errors

Mostly things are spelled correctly, but there are a few things that are incorrect and could be fixed. First, the code has GetCollectiobleLocation() instead of GetCollectibleLocation() and PrinBlankRow() instead of PrintBlankRow(). These kinds of typos don't bother the compiler at all, but they will bother human readers of the code and make it a little more difficult to understand and maintain. The other error I noticed is when the game is over:

std::cout << "type 'r' to restart or 'q' to quite" << '\n';

You probably mean "quit" instead of "quite" here.

Use better naming

Mostly, I think your naming is good. You have reasonable names for objects and functions (notwithstanding the spelling errors mentioned above) but it seems that Includes.h is not a good name for an include file. Also TELEPORTER_AMOUNT is probably less precise than NumberOfTeleporters.

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef PLAYER_H
#define PLAYER_H
// file contents go here
#endif // PLAYER_H

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8

Don't define an empty destructor

The current ~GameField and ~Player, etc. are empty. Better would be to simply omit them. See C.30. Further, if you define Player data members like this, with default values:

Point currentLocation =  {0, 1, 1};
int health = PLAYER_HEALTH;

you can omit an explict constructor.

Don't write getters and setters

C++ isn't Java and writing getter and setter functions for every C++ class is not good style. For example, the Player class has GetX and SetX functions. Since anything can therefore either set or get, it would make more sense to me to simply provide access to the underlying Point and declare that public rather than private. Alternatively, you could define functions a bit more idiomatically like this:

int x() const { return currentLocation.x; }
int y() const { return currentLocation.y; }
void x(int value) { currentLocation.x = value; }
void y(int value) { currentLocation.y = value; }

Minimize interfaces to only what is needed

The GameField class has GetBoardLength() and GetBoardWidth() functions, but neither are ever called. This strongly implies that the special functions are not needed and should be omitted.

Prefer to keep data members private

It's generally bad practice to allow other code to reach inside objects and grab or alter data. For that reason, classes like Board should probably not have any public data members. Where you have closely related classes, you can declare them as friend to allow access to private members, or you can nest them as private so that, for example, the Board class is only defined within the GameField class. See C.9 for more details.

Use constexpr instead of #define for constants

In C++, there is really almost no reason to use #define BOARD_WIDTH 5 and the like. Instead, you can use constexpr int BoardWith = 5;` See ES.31

Use static data members for compile-time constants

Every Board is the same size and that size is determined at compile time. For that reason, instead of having the BOARD_WIDTH constant mentioned above, I'd recommend creating some constexpr static data members like this:

class Board
{
public:
    constexpr static int length = 5;
    constexpr static int width = 5;
    constexpr static char empty = '#'
    constexpr int Size() const { return length * width; }
/* etc. */
private:
    std::vector<char> board = std::vector<char>(Size(), empty);
};

Not only are these compile-time constants both efficient and typed, but they allow you to eliminate an explicit constructor by using a default value for board. One could do the same for the Field dimensions.

Use const where practical

A number of places in the code should have the const keyword added. For example instead of this:

int GetFieldLength();
int GetFieldWidth();

write this:

int GetFieldLength() const;
int GetFieldWidth() const;

Remove redundant code

The first field->PrintField() isn't really needed, because a cls() and field->PrintField() are executed at the top of the loop.

Write member initializers in declaration order

The GameField class has this constructor

GameField::GameField(int boardWidth, int boardLength, int fieldLength, int fieldWidth, Player& player) :
    player(player), boardLength(boardLength), boardWidth(boardWidth), fieldLength(fieldLength), fieldWidth(fieldWidth), board(boardLength, boardWidth)

That looks fine, but in fact, player will be initialized last rather than first because members are always initialized in declaration order and player is declared last in this class. To avoid misleading another programmer, you should swap the order of those such that it says instead:

GameField::GameField(int boardWidth, int boardLength, int fieldLength, int fieldWidth, Player& player)
    : boardLength(boardLength)
    , boardWidth(boardWidth)
    , fieldLength(fieldLength)
    , fieldWidth(fieldWidth)
    , board(boardLength, boardWidth)
    , player(player) 
{ /* rest of initializer */ }

This way the initialization actually proceeds from left to right as one might expect at first glance.

Do a complete restart

If the player restarts rather than quits, they keep their points from the previous game. I don't know if that's intended or not, but I would guess not. One way to handle this would be to implement the next suggestion.

Separate the game loop into a separate function

It appears that the only reason you're using new for field is to allow restarting the game. A better way to do this, generally, is to avoid having to call new and delete and simply let the scope of variables dictate when and how they're created and destroy. If you separate most of your loop into a separate function which returns the points earned, which I've called play in this case, your main could be as simple as this:

int main()
{
    ShowConsoleCursor(false);
    for (bool playing{true}; playing; ) {
        auto points = play();
        cls();
        std::cout << "You finished with " << points << " points. GG!" 
                << "\ntype 'r' to restart or 'q' to quit\n"; 
        std::string userInput;
        std::cin >> userInput;
        playing = userInput == "r";
    }
}

Functions should only do what they say

The GameField::GenScorePoint() function also generates a mine and the GameField::PrintField() function also does an Update. Much better would be to only have each function does what it claims to do.

Reconsider the data structures

It makes sense that the GameField has a collection of Board objects, but many of the other fields seem unwieldy and unnecessary. For example, the std::set<int> visibleBoards seems like a poor choice of data structure. Instead, if you made visible a boolean attribute of each Board, it wouldn't be necessary to do a binary search every time the field is printed. Similarly, allPoints is similar. Its only use is within GenPoint but one could instead simply select a random Board from within boardList and then select random coordinate within that. To be specific, the function could be this:

Point GameField::GenPoint()
{
    Point randPoint;
    do
        randPoint = { GenRandomNumber(0, GameField::size() - 1), GenRandomNumber(0, Board::length - 1), GenRandomNumber(0, Board::width - 1) };
    while (isOccupied(randPoint) || randPoint == player.GetCurrentLocation());
    return randPoint;
}

With two helper functions within GameField:

char get(const Point& p) const { return boardList[p.boardNum].GetCh(p.x, p.y); }
bool isOccupied(const Point &p) const { return get(p) != Board::empty; }

Another option would be to restructure things so that the Board or GameField is composed of individual cell objects such as Portal or Collectible that could more automatically handle interactions with a Player object that encounters them.

Define a static function to run the game

Rather than having some of the game in the main file and some in GameField.cpp, I'd suggest that a much neater way to do things would be to rewrite it so that there is a single static function play(). Using that, the main file becomes just:

#include "GameField.h"
#include <iostream>

int main() {
    for (bool playing{true}; playing; ) {
        auto points = GameField::play();
        cls();
        std::cout << "You finished with " << points << " points. GG!" << '\n';
        std::cout << "type 'r' to restart or 'q' to quit" << '\n';
        std::string userInput;
        std::cin >> userInput;
        playing = userInput == "r";
    }
}
```
\$\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.