Skip to main content
add static function suggestion
Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

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";
    }
}
```

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";
    }
}
```
Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

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.