Iyfjky

Question

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.

#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.
ttt

score 17 · Answer 1 · 2018-09-08 20:52:50Z

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:

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.)

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.

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:

 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:

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().

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:

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:

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;

Thank you for your suggestions. I also though about including some type of abstract entity class, but didn't get to materialize the idea. Looking at your code it does indeed look much more natural. — Sep 8 at 8:55
Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one. — Sep 8 at 19:16
That's a good point. I was mainly trying to fit with the style of the OP, but I forgot that that's not good C++ style. — Sep 8 at 20:51

Kerndog73 423113 · Answer 2 · 2018-09-08 06:33:33Z

That's an awesome little game!

User experience

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

`Rand`om hangs

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.

What am I supposed to do?

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.

Maybe use different movement keys

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.

I pressed the wrong 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.

The code

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

[13-14] Mutable constants

So you have these constants that aren't constant.

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 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.

[18-21] Long member variable names

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:

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.

[26-30] A mutable constant and an unused constant

Here's the Player struct:

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.

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.

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

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

[54-57] Initializing a global variable with designated initializers

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:

const Location board_size = 10, 10;

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

[60-61] Weakly typed enums

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:

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

[63] Passing C arrays to functions

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>.

[70] `#include` C++ random header but use C `rand` instead

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::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::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.

Big functions

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.

Identical functions

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:

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()).

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

Error checking

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.

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:

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:

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

This is how I would implement 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!

Hi! Thank you for all the suggestions, I didn't know there existed strongly typed enums. I will look into implementing them. I just have a question, why shouldn't designed initialization be used? Can any problem arise from using it? I find it is a quite handy syntax. — Sep 8 at 8:54
Designated initialisers are not a C++ feature. TheyÃ¢Â€Â™re not guaranteed to work properly. A standard compliant C++ compiler doesnÃ¢Â€Â™t have to support them. You should use 2, 4 instead of .x = 2, .y = 4 — Sep 8 at 10:02
@Kerndog73: when replying to someone in comments, don't forget to @notify them so they get a notice in their inbox. This happens automatically for you for comments on your answer, but not when you reply to other people. You can only notify one user per comment, so I'm going to ping @Zanzag so they see your comment (and backslash escape the others) — Sep 9 at 3:52

score 17 · Answer 3 · 2018-09-08 20:52:50Z

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:

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.)

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.

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:

 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:

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().

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:

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:

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;

Thank you for your suggestions. I also though about including some type of abstract entity class, but didn't get to materialize the idea. Looking at your code it does indeed look much more natural. — Sep 8 at 8:55
Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one. — Sep 8 at 19:16
That's a good point. I was mainly trying to fit with the style of the OP, but I forgot that that's not good C++ style. — Sep 8 at 20:51

Kerndog73 423113 · Answer 4 · 2018-09-08 06:33:33Z

That's an awesome little game!

User experience

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

`Rand`om hangs

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.

What am I supposed to do?

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.

Maybe use different movement keys

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.

I pressed the wrong 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.

The code

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

[13-14] Mutable constants

So you have these constants that aren't constant.

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 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.

[18-21] Long member variable names

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:

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.

[26-30] A mutable constant and an unused constant

Here's the Player struct:

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.

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.

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

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

[54-57] Initializing a global variable with designated initializers

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:

const Location board_size = 10, 10;

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

[60-61] Weakly typed enums

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:

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

[63] Passing C arrays to functions

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>.

[70] `#include` C++ random header but use C `rand` instead

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::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::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.

Big functions

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.

Identical functions

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:

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()).

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

Error checking

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.

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:

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:

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

This is how I would implement 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!

Hi! Thank you for all the suggestions, I didn't know there existed strongly typed enums. I will look into implementing them. I just have a question, why shouldn't designed initialization be used? Can any problem arise from using it? I find it is a quite handy syntax. — Sep 8 at 8:54
Designated initialisers are not a C++ feature. TheyÃ¢Â€Â™re not guaranteed to work properly. A standard compliant C++ compiler doesnÃ¢Â€Â™t have to support them. You should use 2, 4 instead of .x = 2, .y = 4 — Sep 8 at 10:02
@Kerndog73: when replying to someone in comments, don't forget to @notify them so they get a notice in their inbox. This happens automatically for you for comments on your answer, but not when you reply to other people. You can only notify one user per comment, so I'm going to ping @Zanzag so they see your comment (and backslash escape the others) — Sep 9 at 3:52

Category

Random preview

Dungeon Crawl game for the terminal

2 Answers 2

Types vs. Variables

Looping Improvements

Document Obscure Behavior

Use More Functions

User experience

Random hangs

What am I supposed to do?

Maybe use different movement keys

I pressed the wrong key

The code

[13-14] Mutable constants

[18-21] Long member variable names

[26-30] A mutable constant and an unused constant

[54-57] Initializing a global variable with designated initializers

[60-61] Weakly typed enums

[63] Passing C arrays to functions

[70] #include C++ random header but use C rand instead

Big functions

Identical functions

Error checking

Your Answer

Sign up or log in

Post as a guest

Post as a guest

2 Answers 2

2 Answers 2

Types vs. Variables

Looping Improvements

Document Obscure Behavior

Use More Functions

Types vs. Variables

Looping Improvements

Document Obscure Behavior

Use More Functions

Types vs. Variables

Looping Improvements

Document Obscure Behavior

Use More Functions

Types vs. Variables

Looping Improvements

Document Obscure Behavior

Use More Functions

User experience

Random hangs

What am I supposed to do?

Maybe use different movement keys

I pressed the wrong key

The code

[13-14] Mutable constants

[18-21] Long member variable names

[26-30] A mutable constant and an unused constant

[54-57] Initializing a global variable with designated initializers

[60-61] Weakly typed enums

[63] Passing C arrays to functions

[70] #include C++ random header but use C rand instead

Big functions

Identical functions

Error checking

User experience

Random hangs

What am I supposed to do?

Maybe use different movement keys

I pressed the wrong key

The code

[13-14] Mutable constants

[18-21] Long member variable names

[26-30] A mutable constant and an unused constant

[54-57] Initializing a global variable with designated initializers

[60-61] Weakly typed enums

[63] Passing C arrays to functions

[70] #include C++ random header but use C rand instead

Big functions

Identical functions

Error checking

User experience

Random hangs

2 Answers
2

`Rand`om hangs

[70] `#include` C++ random header but use C `rand` instead

2 Answers
2

2 Answers
2

`Rand`om hangs

[70] `#include` C++ random header but use C `rand` instead

`Rand`om hangs

[70] `#include` C++ random header but use C `rand` instead

`Rand`om hangs

[70] `#include` C++ random header but use C `rand` instead

`Rand`om hangs

[70] `#include` C++ random header but use C `rand` instead