Here are some things that may help you improve your code.
Use the required #include
s
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.