Simple console application to practice OOP and file layout

Question

I am not used to work with OOP so I decided to make a simple console "walk" game to focus on the OOP and the general game design process. I tried to make it as robust as possible but I feel like I could have build the hierarchy of the classes and functions in a more systematic way. Please think of this as a much larger project. I would appreciate if you could criticize and offer a more reliable way of ordering the objects and of course other mistakes and performance issues.

Coordinate.hpp

#ifndef COORD_H
#define COORD_H
struct Coordinate{
 int x;
 int y;
};
#endif

Game.hpp

//Clear the console for different operating systems 
#ifdef _WIN32
#define CLEAR "cls"
#else
#define CLEAR "clear"
#endif
#ifndef GAME_H
#define GAME_H
#include "Player.hpp"
#include "Map.hpp"
class Game{
public:
 Game();
private:
 void setup();
 void loop(Player p, Map m);
 char processInput(Player p);
 void update(Player &p, char movement, Map m);
 void draw(Player p, Map m);
 void clear();
};
#endif

Map.hpp

#ifndef MAP_H
#define MAP_H
#include <string>
#include <vector>
#include <iostream>
class Map{
public:
 Map(int h, int w);
 int getWidth();
 int getHeight();
 std::vector<std::string> getLevel();
private:
 std::vector<std::string> level; 
 int width;
 int height;
 void initMap(int h, int w);
};
#endif

Player.hpp

#ifndef PLAYER_H
#define PLAYER_H
#include <iostream>
#include "Coordinate.hpp"
#include "Map.hpp"
class Player{
public:
 Player();
 void movePlayer(int x, int y, Map m);
 Coordinate getCoord();
private:
 Coordinate coord;
};
#endif

Game.cpp

#include "Game.hpp"
#include "Player.hpp"
#include "Map.hpp"
Game::Game(){
 setup();
}
void Game::setup(){
 Player p;
 Map m(5, 5);
 Game::loop(p, m);
}
//Main Game Loop
/*
Clear the screen
Draw the map
Take input
Update the Map
*/
void Game::loop(Player p, Map m){
 while(true){
 Game::clear();
 Game::draw(p, m);
 char movement = Game::processInput(p);
 Game::update(p, movement, m);
 }
}
char Game::processInput(Player p){
 char movement;
 std::cin>>movement;
 tolower(movement);
 return movement;
}
void Game::update(Player &p, char movement, Map m){
 switch (movement)
 {
 case 'w'://up
 p.movePlayer(0,-1,m);
 break;
 case 'a'://left
 p.movePlayer(-1,0,m);
 break;
 case 's'://down
 p.movePlayer(0,1,m);
 break;
 case 'd'://right
 p.movePlayer(1,0,m);
 break; 
 default:
 break;
 }
}
void Game::draw(Player p, Map m){
 Map map = m;//Copy the layer without the player without changing the main level
 std::vector<std::string> level = map.getLevel();
 level[p.getCoord().y].replace(p.getCoord().x, 1, "P");//replace the # with P
 for(int i=0; i<map.getHeight(); i++){
 for(int j=0; j<map.getWidth(); j++){
 std::cout<<level[i][j];
 }
 std::cout<<std::endl;
 }
}
void Game::clear(){
 system(CLEAR);//works for Windows and Linux
}

Map.cpp

#include "Map.hpp"
Map::Map(int h, int w){
 initMap(h, w);
}
int Map::getWidth(){
 return width;
}
int Map::getHeight(){
 return height;
}
std::vector<std::string> Map::getLevel(){
 return level;
}
void Map::initMap(int h, int w){
 height = h;
 width = w;
 for(int i=0; i<height; i++){
 std::string str;
 level.push_back(str);
 for(int j=0;j<width;j++){
 level[i].append(".");
 }
 }
}

Player.cpp

#include "Player.hpp"
#include "Map.hpp"
Player::Player(){
 coord.x=0;
 coord.y=0;
}
void Player::movePlayer(int x, int y, Map m){
 if(!(coord.x + x >= m.getWidth() || coord.x + x < 0)){//If not at the map limit, move
 coord.x += x;
 }
 if(!(coord.y + y >= m.getHeight() || coord.y + y < 0)){
 coord.y += y;
 }
}
Coordinate Player::getCoord(){
 return coord;
}

main.cpp

/*ASCII Walk Simulator
P: Player
.: Walkable Path
Enter w,a,s,d to walk
*/
#include "Game.hpp"
int main(){
 Game game;
}

In terminal:

g++ *.cpp -o out

• JDługosz: Keep it up! Writing self-learning projects like this is how good programmers are made.

Answer 1

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

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you have attempted to address. The second is that it's a security hole. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
 std::cout << "\x1b[2J";
}

Use const where practical

I would not expect the getWidth or getHeight member functions of Map to alter the underlying map on which they operate, and indeed they do not. You should make this expectation explicit by using the const keyword:

int Map::getWidth() const {
 return width;
}

Pass by const reference where practical

The third argument to Player::movePlayer is a Map but that causes the entire map to be duplicated. Better would be to make it const Map & because it is not modified and it doesn't need to be duplicated.

Use standard functions where appropriate

The current Player::movePlayer() function is this (with the modification mentioned above):

void Player::movePlayer(int x, int y, const Map &m){
 if(!(coord.x + x >= m.getWidth() || coord.x + x < 0)){//If not at the map limit, move
 coord.x += x;
 }
 if(!(coord.y + y >= m.getHeight() || coord.y + y < 0)){
 coord.y += y;
 }
}

This is much easier to write using std::clamp():

void Player::movePlayer(int x, int y, const Map &m){
 coord.x = std::clamp(coord.x + x, 0, m.getWidth());
 coord.y = std::clamp(coord.y + y, 0, m.getHeight());
}

Prefer modern initializers for constructors

The constructor for Player is this:

Player::Player(){
 coord.x=0;
 coord.y=0;
}

Better would be to simply have a default constructor for the Coordinate and then you could omit the Player constructor and simply have the compiler write one.

struct Coordinate{
 int x{0};
 int y{0};
};

See C.48 for details.

Fix the bug

The Game::processInput has a few bugs in it. First, it passes Player p which is not used and should therefore not be passed. Second, it has these lines:

char movement;
std::cin>>movement;
tolower(movement);
return movement;

The tolower has no effect here. What you probably intended was this:

return tolower(movement);

The name is also not good. It isn't really processing the input, only getting it from the user.

Rethink the interface

The Game constructor doesn't just create the game but also runs it and never returns. Consequently, main looks like this:

int main(){
 Game game;
}

A human reader would likely wonder why a game object is created and never used. Better would be to have separate public functions: the constructor should just construct the object, and the loop (or perhaps play()) with a possibly separate check for end of game function.

A revised version of Game.hpp is this:

#ifndef GAME_H
#define GAME_H
#include "Player.hpp"
#include "Map.hpp"
class Game{
public:
 bool play();
 void draw() const;
private:
 Player p;
 Map m{5, 5};
};
#endif

Now main is this:

int main(){
 Game game;
 do {
 game.draw();
 } while (game.play());
}

Think of the user

I know this is just a preliminary study, but there is no way to exit the program gracefully. That's an easy fix. I rewrote and renamed your update like this:

bool Game::play() {
 bool playing{true};
 char movement;
 std::cin >> movement;
 switch (tolower(movement))
 {
 case 'w'://up
 p.movePlayer(0,-1,m);
 break;
 case 'a'://left
 p.movePlayer(-1,0,m);
 break;
 case 's'://down
 p.movePlayer(0,1,m);
 break;
 case 'd'://right
 p.movePlayer(1,0,m);
 break;
 case 'x': // exit
 playing = false;
 default:
 break;
 }
 return playing;
}

Simplify your constructors

There is no reason for the Map constructor to call initMap. Instead the functionality of initMap should be incorporated into the constructor directly. Further, rather than stepping through every character, you can use std::vector and std::string constructor forms which take a count and a value.

Map::Map(std::size_t h, std::size_t w)
 : width{w}
 , height{h}
 , level{height, std::string(width, '.')}
{
}

Note that I've also converted width and height to std::size_t type. There would be little sense in specifying a negative value for either.

Answer 2

In addition to the points Edward noted,

Your #include guard names are too simple. In a real program, they might very easily clash with things from other libraries. If you're writing for yourself, just use #pragma once. If you use portable guards, don't use a simple word like that! Always use a globally unique symbol; e.g. based on a UUID.

---

The style in C++ is to put the * or & with the type, not the identifier. This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.

---

Write this down on a small card and stick it next to your monitor:

C++ has value semantics

If you pass a complex class object, like vector or your own classes, you copy it! Normally pass such things by const &.

---

void Game::loop(Player p, Map m){
 while(true){
 Game::clear();
 Game::draw(p, m);
 char movement = Game::processInput(p);
 Game::update(p, movement, m);
 }
}

If you are in the member function of Game, why are you qualifying all your calls with Game::? That is the normal scope you are working in! At best, this is not necessary and makes readers wonder what's happening that's not normal.

---

The Game class has no state. Would it make sense to put the Map and the player objects as part of that Game? Consider what it would take to have multiple instances of Game in existence at the same time.