Dungeon Crawl game for the terminal

I am learning C++ and I attempted to do one exercise I found: Dungeon Crawl. The goal of this game is to reach the treasure by moving your character along the board.

The exercise asks not to use classes so I have tried to achieve some level of abstraction with structs and arrays. I have followed my university's style for comments.

structs

arrays

#include <climits>
#include <ctime>
#include <iostream>
#include <random>
#include <string>

/**
* DUNGEON: a simple game for the terminal. The objective of the
* game is that the player ("P") reaches the treasure ("X")
* avoiding the traps ("T") and the bandits ("B").
* Bandits move randomly each turn.
* */
int NUMBEROFTRAPS = 3;
int NUMBEROFBANDITS = 2;

// Represents a place in the board.
// xPosition is the x-axis index and yPosition is the y-axis index
struct Location
int xPosition;
int yPosition;
;

// Represents the player.
// It is guaranteed Player position is in the board.
// Position is altered through function movePlayer.
struct Player
Location position;
char symbol = 'P';
std::string name = "alvaro";
;

// Represents traps on the board
// It is guarateed Trap position is in the board.
struct Trap
Location position;
char symbol = 'T';
;

// Represents Bandits moving around the map.
// Position is altered through funtion moveBandit.
struct Bandit
Location position;
char symbol = 'B';
;

// Represents the treasure.
// The game ends as soon Player.position == Treasure.position
struct Treasure
Location position;
char symbol = 'X';
;

// Represents the board.
struct
int xDimension;
int yDimension;
board = .xDimension = 10, .yDimension = 10;

// Possible directions. WRONG_DIRECTION is used to report incorrect input
enum Direction RIGHT, LEFT, TOP, BOTTOM, WRONG_DIRECTION ;
enum Result VICTORY, DEFEAT ;

void drawBoard(Player, Trap, Bandit, Treasure);
void endGame(Result);
void movePlayer(Player &, Direction);
void moveBandit(Bandit &);
Direction askDirection();

int main()
std::srand(std::time(0));

// Treasure position is decided randomly.
Treasure treasure =
.position = .xPosition = std::rand() % board.xDimension,
.yPosition = std::rand() % board.yDimension;

// Traps are placed around the map. It is not guaranteed
// that traps position doesn't converge.
// In that case, the second trap can be assumed to not exist.
Trap trapsInMap[NUMBEROFTRAPS];
for (int i = 0; i < NUMBEROFTRAPS; i++)
int xPos = std::rand() % board.xDimension;
int yPos = std::rand() % board.yDimension;
Trap trap = .position = .xPosition = xPos, .yPosition = yPos;
trapsInMap[i] = trap;


// Bandits are placed around the map. It is not guaranteed
// that bandits position doesn't converge, but they will move
// anyway.
Bandit banditsInMap[NUMBEROFBANDITS];
for (int i = 0; i < NUMBEROFBANDITS; i++)
int xPos = std::rand() % board.xDimension;
int yPos = std::rand() % board.yDimension;
Bandit bandit = .position = .xPosition = xPos, .yPosition = yPos;
banditsInMap[i] = bandit;


// Player position on the 1st turn is randomly decided.
// It can not be the same of a bandit or a trap.
bool match = false;
int xPos;
int yPos;
do
xPos = std::rand() % board.xDimension;
yPos = std::rand() % board.yDimension;
for (int i = 0; i < NUMBEROFTRAPS; i++)
(xPos == banditsInMap[i].position.xPosition &&
yPos == banditsInMap[i].position.yPosition))
match = true;


while (match);

Player alvaro = .position = .xPosition = xPos, .yPosition = yPos;

// The order of the turn is the following:
// 1. Board is drawn.
// 2. User is asked for movement direction.
// 3. Player moves in the chosen direction.
// 4. Bandits move.
int maxTurnos = INT_MAX;
for (int i = 0; i <= maxTurnos; i++)
drawBoard(alvaro, trapsInMap, banditsInMap, treasure);
Direction direction;
do
direction = askDirection();
std::cout << std::endl;
while (direction == WRONG_DIRECTION);
movePlayer(alvaro, direction);
for (int i = 0; i < NUMBEROFBANDITS; i++)
moveBandit(banditsInMap[i]);

std::cout << "x1B[2Jx1B[H";



void drawBoard(
/* in */ Player player,
/* in */ Trap totalTraps,
/* in */ Bandit totalBandits,
/* in */ Treasure treasure)

// Draws a (board.xDimension * board.yDimension) grid.
// Elements are drawn using .location.?Dimensions.

// Precondition: 0 <= Player.xPosition <= board.xDimension &&
// 0 <= player.position.yPosition <= board.yDimension &&
// board.xDimension > 0 && board.yDimension > 0 &&
// Postcondition: The grid has been drawn.
// All elements have been drawn.
// If the player is in the same square than the treasure,
// the game ends with victory.
// If the player is in the same square than a bandit or
// a trap, the game ends with defeat.

bool squareDrawn = false;
for (int y = 0; y <= board.yDimension; y++)
for (int x = 0; x <= board.xDimension; x++)
// Traps are drawn
for (int z = 0; z <= NUMBEROFTRAPS; z++)
Trap trapToDraw = totalTraps[z];
if (trapToDraw.position.xPosition == x &&
trapToDraw.position.yPosition == y)
std::cout << trapToDraw.symbol;
squareDrawn = true;


// Bandits are drawn.
// In case of collision with a trap,
// only the second is drawn.
for (int z = 0; z <= NUMBEROFBANDITS; z++)
Bandit banditToDraw = totalBandits[z];
if (banditToDraw.position.xPosition == x &&
banditToDraw.position.yPosition == y && !squareDrawn)
std::cout << banditToDraw.symbol;
squareDrawn = true;



// Treasure is drawn. If position of treasure == position of player
// game ends with victory
if (x == treasure.position.xPosition &&
y == treasure.position.yPosition)
if (treasure.position.xPosition == player.position.xPosition &&
treasure.position.yPosition == player.position.yPosition)
endGame(VICTORY);


std::cout << "X";
continue;


if (x == player.position.xPosition && y == player.position.yPosition)
if (squareDrawn)
endGame(DEFEAT);
std::cout << "P";
continue;

// Empty square "." is drawn. It only gets printed if there is nothing
// on the square.
if (!squareDrawn)
std::cout << ".";
squareDrawn = false;

std::cout << std::endl;



Direction askDirection()

// Asks the user to input a direction and returns it.
// Precondition: -
// Poscondition:
// Return: a Direction value containing the direction chosen or
// WRONG_DIRECTION.

std::cout << "Select [L]eft, [R]ight, [T]op or [B]ottom: ";
char answer;
std::cin.get(answer);

Direction chosenDirection;
switch (std::toupper(answer))
case 'L':
chosenDirection = LEFT;
break;
case 'R':
chosenDirection = RIGHT;
break;
case 'T':
chosenDirection = TOP;
break;
case 'B':
chosenDirection = BOTTOM;
break;
default:
chosenDirection = WRONG_DIRECTION;

return chosenDirection;


void movePlayer(
/* inout */ Player &player, // Player of the game
/* in */ Direction direction) // Direction previously chosen.
// It is represented by a Direction object,
// different from WRONG_DIRECTION.

// Moves player in the chosen direction, by altering its coordinates. If the
// player would finish out of the board, no movement is made.

// Precondition: 0 <= Player.xPosension <= board.xDimension &&
// 0 <= player.position.yPosition <= board.yDimension &&
// board.xDimension > 0 && board.yDimension > 0 &&
// direction in LEFT; RIGHT; TOP; BOTTOM &&
// Postcondition: player coordinates have been altered &&
// player remains inside the board.

switch (direction)
case RIGHT:
if (player.position.xPosition < board.xDimension)
player.position.xPosition += 1;
break;
case LEFT:
if (player.position.xPosition > 0)
player.position.xPosition -= 1;
break;
case TOP:
if (player.position.yPosition > 0)
player.position.yPosition -= 1;
break;
case BOTTOM:
if (player.position.yPosition < board.yDimension)
player.position.yPosition += 1;
break;



void moveBandit(
/* inout */ Bandit &bandit) // Player of the game
// It is represented by a Direction object,
// different from WRONG_DIRECTION.

// Moves player in the chosen direction, by altering its coordinates. If the
// player would finish out of the board, no movement is made.

// Precondition: 0 <= Player.xPosension <= board.xDimension &&
// 0 <= player.position.yPosition <= board.yDimension &&
// board.xDimension > 0 && board.yDimension > 0 &&
// direction in LEFT; RIGHT; TOP; BOTTOM &&
// Postcondition: player coordinates have been altered &&
// player remains inside the board.


int direction = std::rand() % 4;
switch (direction)
case 0:
if (bandit.position.xPosition < board.xDimension)
bandit.position.xPosition += 1;
break;
case 1:
if (bandit.position.xPosition > 0)
bandit.position.xPosition -= 1;
break;
case 2:
if (bandit.position.yPosition > 0)
bandit.position.yPosition -= 1;
break;
case 3:
if (bandit.position.yPosition < board.yDimension)
bandit.position.yPosition += 1;
break;



void endGame(
/* in */ Result result) // Result of the game.
// It is either VICTORY or DEFEAT
// Cleans screen, prints a good bye message
// and ends the game.
// Precondition: a condition for ending the game has been found.
// Either player.position == bandit.position ||
// player.position == trap.position [DEFEAT]
// or player.position == treasure.position [VICTORY]
// Poscondition: game is ended. Greeting message is printed.



2 Answers
2

Overall, this is really well done. You've missed the usual traps of using magic numbers, not creating structures for related items, and other common things. So nice work! I think it could be improved with the following changes.

Types vs. Variables

You've created a type for Location which is great. Looking at the types for Player, Trap, Bandit, and Treasure, they're identical, except that Player has a name string, which is never used anywhere in the code. Given that, it makes sense to me to make a type something like GameEntity, and create variables for the player, traps, bandits, and treasures. Something like this:

Location

Player

Trap

Bandit

Treasure

Player

name

GameEntity

struct GameEntity
Location position;
char symbol;
;


In main, you'd create the variables like so:

int main()
std::srand(std::time(0));

GameEntity treasure =
std::rand() % board.xDimension,
std::rand() % board.yDimension,
TREASURESYMBOL
;

GameEntity trapsInMap[NUMBEROFTRAPS];
setEntityPositions(trapsInMap, NUMBEROFTRAPS, TRAPSYMBOL);

GameEntity banditsInMap[NUMBEROFBANDITS];
setEntityPositions(banditsInMap, NUMBEROFBANDITS, BANDITSYMBOL);

GameEntity alvaro;
setPlayerPosition(alvaro, PLAYERSYMBOL);


The function setEntityPositions() would just iterate over the array and set the position to a random position and the symbol to the passed-in symbol for each entity in the array. (And of course, you'll need to define TRAPSYMBOL, BANDITSYMBOL, PLAYERSYMBOL, and TREASURESYMBOL appropriately.)

setEntityPositions()

TRAPSYMBOL

BANDITSYMBOL

PLAYERSYMBOL

TREASURESYMBOL

Looping Improvements

I don't like the loop you have in main(). It posits that there is some maximum number of turns after which the game is done. But that's not the case. The game is done when the player falls into a trap, is robbed by a bandit, or finds the treasure. Additionally, the code that does stop the game is way down in a different function making it difficult for someone reading the code to figure out the ending condition.

main()

What I would do is have drawBoard() return either VICTORY, DEFEAT, or CONTINUE. If it returns VICTORY or DEFEAT call endGame(). I would not have endGame() call exit(). Instead, after it returned, I would exit the loop. So main() would continue (from what I have above) like this:

drawBoard()

VICTORY

DEFEAT

CONTINUE

VICTORY

DEFEAT

endGame()

endGame()

exit()

main()

int gameCondition = drawBoard(alvaro, trapsInMap, banditsInMap, treasure);
do
Direction direction;
do
direction = askDirection();
std::cout << std::endl;
while (direction == WRONG_DIRECTION);
movePlayer(alvaro, direction);
for (int i = 0; i < NUMBEROFBANDITS; i++)
moveBandit(banditsInMap[i]);

clearScreenAndMoveToHome();
gameCondition = drawBoard(alvaro, trapsInMap, banditsInMap, treasure);
while (gameCondition == CONTINUE);
endGame (gameCondition);



Document Obscure Behavior

You'll notice that I introduced a new function called from main() named clearScreenAndMoveToHome(). This is because this line is incomprehensible:

main()

clearScreenAndMoveToHome()

std::cout << "x1B[2Jx1B[H";


I don't do a lot of console-based programs, so I had no idea what this was. When I run it in my debugger, it outputs:

[2J[H...........

.........T[2J[H

When I run it in a terminal, it clears the screen sets the cursor to the upper left. By putting it into a function, you show clearly what it does. Furthermore, you can call it elsewhere and know you have the right string. When I run the app, it doesn't clear before drawing the board the first time. I would move the function call into drawBoard().

drawBoard()

Use More Functions

You've done a really good job breaking this into functions, but I think you could do even more. In drawBoard(), checking the current square against traps and bandits is essentially the same code. I would write it as something like this:

drawBoard()

for (int y...)
for (int x...
bool squareDrawn = checkSquareAgainstEntity(x, y, totalTraps, NUMBEROFTRAPS);

if (!squareDrawn)
squareDrawn = checkSquareAgainstEntity(x, y, totalBandits, NUMBEROFBANDITS);

// ... etc.




Then the checkSquareAgainstEntity() function would iterate over the passed in array:

checkSquareAgainstEntity()

bool checkSquareAgainstEntity(int x, int y, GameEntity* entities, int numEntities)

bool result = false;
for (int z = 0; (z < numEntities) && (!result); z++
GameEntity nextEntity = entities [ z ];
if (nextEntity.position.xPosition == x &&
nextEntity.position.yPosition == y)
std::cout << nextEntity.symbol;
result = true;


return result;



.position = /*...*/

That's an awesome little game!

Before we dive into the code, let's talk about the game itself.

Rand

Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens randomly! We'll find the source of this bug later.

rand

When presented with this:

...........
...........
...TX...T..
......P....
...........
B..........
........T..
...........
........B..
...........
...........
Select [L]eft, [R]ight, [T]op or [B]ottom:


It's not immediately clear what I'm supposed to do. The comment at line 7 was quite helpful.

/**
* DUNGEON: a simple game for the terminal. The objective of the
* game is that the player ("P") reaches the treasure ("X")
* avoiding the traps ("T") and the bandits ("B").
* Bandits move randomly each turn.
* */


I suggest that tell the player how to play the game by printing this comment.

This message tells the player which keys to press:

Select [L]eft, [R]ight, [T]op or [B]ottom:


I'm not sure why you chose to write "top" and "bottom" instead of "up" and "down". The player "moves up". The player doesn't "move top". Also, the keys LRTB aren't really natural for movement. Most players will be used to WASD (W-up, A-left, S-down, D-right). I suspect that if you don't tell players which keys to press, they will assume WASD. It's also more comfortable for the hands. You can put your left hand on WASD and your right hand on the enter key.

If I press "Z", this happens:

T........T.
....B......
........B..
T..........
...........
...........
...........
........P..
...........
........X..
...........
Select [L]eft, [R]ight, [T]op or [B]ottom: Z

Select [L]eft, [R]ight, [T]op or [B]ottom:
Select [L]eft, [R]ight, [T]op or [B]ottom:


So you leave behind my mistake, then you print out an empty line, then you tell what to select a direction twice. I suggest that you simply clear the line when the player makes a mistake.

OK, that's enough of that. This is CodeReview, not GameReview.

So you have these constants that aren't constant.

const

int NUMBEROFTRAPS = 3;
int NUMBEROFBANDITS = 2;


To declare a constant in C++, use the const keyword. Also, ALLCAPS is generally reserved for macros. So you should change that to this:

const

const int number_of_traps = 3;
const int number_of_bandits = 2;


Telling the compiler "this is a constant" might make your lines a little longer but the compiler can help you if you give it a better understanding of your program. If at some point in your program you do this:

number_of_traps = 7;


The compiler will tell you that you made a mistake.

To me, xPosition and yPosition are unnecessarily long. There's no need to put Position in the name because the struct is already a position. So you end up doing writing this like this:

xPosition

yPosition

Position

struct

Location position;
position.xPosition = 4;


You're writing "position" twice. You really should shorten these names to their essence and just use x and y.

x

y

Here's the Player struct:

Player

struct Player
Location position;
char symbol = 'P';
std::string name = "alvaro";
;


At no point did I ever see "alvaro" printed to the screen when I was playing this game. I'm not sure why it's name is there.

name

symbol never changes so it should be const. Trap, Bandit and Treasure all carry around this char symbol. Every Bandit instance is storing the same symbol. You really should put this duplicate information in one place. symbol should be static. This means that symbol can now be accessed as Player::symbol or Bandit::symbol. You can still access the symbol from the instance (player.symbol) but I suggest you use Player::symbol to avoid confusing readers. Putting this all together we this new Player struct.

symbol

const

Trap

Bandit

Treasure

char symbol

Bandit

symbol

symbol

static

symbol

Player::symbol

Bandit::symbol

player.symbol

Player::symbol

Player

struct Player
Location position;
static const char symbol = 'P';
;


You should use static const for the Trap, Bandit and Treasure structs as well.

static const

Trap

Bandit

Treasure

This is quite a peculiar bit of code:

struct
int xDimension;
int yDimension;
board = .xDimension = 10, .yDimension = 10;


You've creating an anonymous struct, which looks pretty much the same as an existing struct (Location). You creating a global variable board. I think you actually want to create a global constant. You're also using designated initializers .xDimension = 10, .yDimension = 10. If you enable all warnings by passing these flags to your compiler -Wall -Wextra -pedantic, your compiler should tell you that "designated initializers are a C99 feature". Designated initializers are not a C++ feature. You should use brace-initialization instead. I suggest that you replace that code with this:

Location

board

.xDimension = 10, .yDimension = 10

-Wall -Wextra -pedantic

const Location board_size = 10, 10;


In future, you should always pass -Wall -Wextra -pedantic and edit your code until there are no warnings.

-Wall -Wextra -pedantic

Here, your using ALLCAPS again. You're also using weakly typed enums.

enum Direction RIGHT, LEFT, TOP, BOTTOM, WRONG_DIRECTION ;
enum Result VICTORY, DEFEAT ;


Weakly typed basically means that this code is valid:

int dir = TOP;


Regular enums are another one of those C features that you should never use in C++. In C++, you should use strongly typed enums by putting class (or struct but most people just use class) after enum. This has two effects. Firstly, it stops you from just writing RIGHT or LEFT. It forces you to write Direction::RIGHT which is much clearer. Secondly, it disallows implicit casts to the underlying type. So those enums should be written as this:

class

struct

class

enum

RIGHT

LEFT

Direction::RIGHT

enum class Direction right, left, top, bottom, wrong ;
enum class Result victory, defeat ;


Here, you're declaring a function that takes a few arrays as parameters.

void drawBoard(Player, Trap, Bandit, Treasure);


Passing stack arrays to functions is another thing that you CAN do in C++ but you probably shouldn't. Consider replacing all of your usages of C arrays with a C++ container like std::vector<Trap> or std::array<Trap, number_of_traps>.

std::vector<Trap>

std::array<Trap, number_of_traps>

#include

rand

At line 4, you include the C++ random numbers header

#include <random>


But then at line 70, you seed the C random number generator with the C time function.

std::srand(std::time(0));


The C++ random header consists of generators and distributions. Pseudo-random number generators produce a stream of pseudo-random bits. Distributions take the random bits and distribute them across a range. I suggest that you seed a std::mt19937 generator using a std::random_device.

std::mt19937

std::random_device

std::random_device device;
std::mt19937 gendevice();


Now that you have seeded the std::mt19937 pseudo-random number generator, you start generating some numbers. Let's say you want to generate random ints between 0 and board_size.x - 1 inclusive. You would do this:

std::mt19937

int

0

board_size.x - 1

std::uniform_int_distribution<int> dist0, board_size.x - 1;
const int xPos = distgen;


Since the seed is now stored in a std::mt19937 object and not globally, you have to pass gen to all of the functions that need random variables.

std::mt19937

gen

There are a few very big functions. You should chop up your program into nice neat little functions that each to do one specific job. For example, drawBoard should call functions to drawPlayer, drawTreasure, drawBandit and drawTrap.

drawBoard

drawPlayer

drawTreasure

drawBandit

drawTrap

moveBandit and movePlayer are pretty much exactly the same. I suggest that you have one function for moving "things". (Just rename movePlayer to moveObject(Location &, Direction). You should also have a function for generating random directions:

moveBandit

movePlayer

movePlayer

moveObject(Location &, Direction)

Direction getRandDir(std::mt19937 &gen)
std::uniform_int_distribution<int> dist0, 3;
return static_cast<Direction>(distgen);



To move a bandit, moveObject(bandit.pos, getRandDir()).

moveObject(bandit.pos, getRandDir())

To move a player, moveObject(player.pos, askDir()).

moveObject(player.pos, askDir())

The function askDirection might return a valid direction or it might return WRONG_DIRECTION. Handling the case where the player inputs a bad direction is handled else. This doesn't really make sense to me. askDirection should always return a valid direction. askDirection should be responsible for dealing with the case where the player inputs an invalid direction.

askDirection

WRONG_DIRECTION

askDirection

askDirection

You're using std::cin.get to get one character at a time. This combined with handling the error outside of askDirection is the reason why this happens the player inputs an invalid character:

std::cin.get

askDirection

T........T.
....B......
........B..
T..........
...........
...........
...........
........P..
...........
........X..
...........
Select [L]eft, [R]ight, [T]op or [B]ottom: Z

Select [L]eft, [R]ight, [T]op or [B]ottom:
Select [L]eft, [R]ight, [T]op or [B]ottom:


You really should be getting the whole the line like this:

std::string input;
std::cin >> input;


This way, you can check if the player inputted too many characters:

if (input.size() != 1)
std::cout << "One character pleasen";



When the player inputs a bad character, you should move the cursor up, clear the line and try again. With ANSI escape codes, that's "x1B[1A" and "x1B[0K". I also suggest that you put these character sequences into constants to make the code more readable. You should put something like that right at the top of the file:

"x1B[1A"

"x1B[0K"

const char cursorUp = "x1B[1A";
const char clearLine = "x1B[0K";


This is how I would implement askDirection.

askDirection

Dir askDirection()
std::cout << "dir> ";
std::string input;
std::cin >> input;
if (input.size() == 1)
switch (std::toupper(input[0]))
case 'W':
return Dir::up;
case 'A':
return Dir::left;
case 'S':
return Dir::down;
case 'D':
return Dir::right;


std::cout << cursorUp << clearLine;
return askDirection();



I'm out of time! I hope you found some of my advice helpful!

enums

Thanks for contributing an answer to Code Review Stack Exchange!

But avoid …

Use MathJax to format equations. MathJax reference.

To learn more, see our tips on writing great answers.

Required, but never shown

Required, but never shown

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.