Dungeon Crawl game for the terminal Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern) Announcing the arrival of Valued Associate #679: Cesar Manara Unicorn Meta Zoo #1: Why another podcast?AI bot Java dungeon gameAPI for Dungeon GeneratorRuby object oriented Snakes and Ladders implementationFoundation for 2D dungeon crawler game“Dungeon Game” solutionText-based Dungeon Crawl gameA little C++ based 2D game engine I madeA mine sweeping game for the terminalThe Last Lost Bubble Shooter - a Board game for childrenSDL2/C++ Pong Game

Co-worker has annoying ringtone

What does it mean that physics no longer uses mechanical models to describe phenomena?

Dyck paths with extra diagonals from valleys (Laser construction)

Why weren't discrete x86 CPUs ever used in game hardware?

What would you call this weird metallic apparatus that allows you to lift people?

Intuitive explanation of the rank-nullity theorem

How does Belgium enforce obligatory attendance in elections?

How to identify unknown coordinate type and convert to lat/lon?

What is the difference between a "ranged attack" and a "ranged weapon attack"?

Has negative voting ever been officially implemented in elections, or seriously proposed, or even studied?

How to compare two different files line by line in unix?

How to report t statistic from R

Did Mueller's report provide an evidentiary basis for the claim of Russian govt election interference via social media?

What to do with repeated rejections for phd position

Amount of permutations on an NxNxN Rubik's Cube

Why are vacuum tubes still used in amateur radios?

Draw 4 of the same figure in the same tikzpicture

What are the discoveries that have been possible with the rejection of positivism?

What is the meaning of 'breadth' in breadth first search?

Project Euler #1 in C++

Why does it sometimes sound good to play a grace note as a lead in to a note in a melody?

How did Fremen produce and carry enough thumpers to use Sandworms as de facto Ubers?

AppleTVs create a chatty alternate WiFi network

Why can't I install Tomboy in Ubuntu Mate 19.04?



Dungeon Crawl game for the terminal



Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern)
Announcing the arrival of Valued Associate #679: Cesar Manara
Unicorn Meta Zoo #1: Why another podcast?AI bot Java dungeon gameAPI for Dungeon GeneratorRuby object oriented Snakes and Ladders implementationFoundation for 2D dungeon crawler game“Dungeon Game” solutionText-based Dungeon Crawl gameA little C++ based 2D game engine I madeA mine sweeping game for the terminalThe Last Lost Bubble Shooter - a Board game for childrenSDL2/C++ Pong Game



.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;








15












$begingroup$


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++)
if ((xPos == trapsInMap[i].position.xPosition &&
yPos == trapsInMap[i].position.yPosition)
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.










share|improve this question











$endgroup$


















    15












    $begingroup$


    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++)
    if ((xPos == trapsInMap[i].position.xPosition &&
    yPos == trapsInMap[i].position.yPosition)
    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.










    share|improve this question











    $endgroup$














      15












      15








      15





      $begingroup$


      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++)
      if ((xPos == trapsInMap[i].position.xPosition &&
      yPos == trapsInMap[i].position.yPosition)
      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.










      share|improve this question











      $endgroup$




      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++)
      if ((xPos == trapsInMap[i].position.xPosition &&
      yPos == trapsInMap[i].position.yPosition)
      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.







      c++ game console homework






      share|improve this question















      share|improve this question













      share|improve this question




      share|improve this question








      edited Sep 8 '18 at 9:54









      mdfst13

      17.9k62257




      17.9k62257










      asked Sep 7 '18 at 23:39









      ZanzagZanzag

      1013




      1013




















          2 Answers
          2






          active

          oldest

          votes


















          18












          $begingroup$

          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;






          share|improve this answer











          $endgroup$












          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:55










          • $begingroup$
            Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one.
            $endgroup$
            – Angew
            Sep 8 '18 at 19:16










          • $begingroup$
            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.
            $endgroup$
            – user1118321
            Sep 8 '18 at 20:51


















          14












          $begingroup$

          That's an awesome little game!



          User experience



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




          Random hangs



          Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens randomly! We might 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 on 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 me 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 its 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 get 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're creating an anonymous struct, which looks pretty much the same as an existing struct (Location). You're 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 at least -Wall -Wextra -pedantic and edit your code until there are no warnings.



          [60-61] Weakly typed enums



          Here, you're 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 like 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 an std::mt19937 generator using an 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 = dist(gen);


          Since the seed is now stored in an 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>(dist(gen));



          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 elsewhere. 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 when 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 this 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!






          share|improve this answer











          $endgroup$












          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:54










          • $begingroup$
            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
            $endgroup$
            – Kerndog73
            Sep 8 '18 at 10:02










          • $begingroup$
            @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)
            $endgroup$
            – Peter Cordes
            Sep 9 '18 at 3:52











          Your Answer






          StackExchange.ifUsing("editor", function ()
          StackExchange.using("externalEditor", function ()
          StackExchange.using("snippets", function ()
          StackExchange.snippets.init();
          );
          );
          , "code-snippets");

          StackExchange.ready(function()
          var channelOptions =
          tags: "".split(" "),
          id: "196"
          ;
          initTagRenderer("".split(" "), "".split(" "), channelOptions);

          StackExchange.using("externalEditor", function()
          // Have to fire editor after snippets, if snippets enabled
          if (StackExchange.settings.snippets.snippetsEnabled)
          StackExchange.using("snippets", function()
          createEditor();
          );

          else
          createEditor();

          );

          function createEditor()
          StackExchange.prepareEditor(
          heartbeatType: 'answer',
          autoActivateHeartbeat: false,
          convertImagesToLinks: false,
          noModals: true,
          showLowRepImageUploadWarning: true,
          reputationToPostImages: null,
          bindNavPrevention: true,
          postfix: "",
          imageUploader:
          brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
          contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
          allowUrls: true
          ,
          onDemand: true,
          discardSelector: ".discard-answer"
          ,immediatelyShowMarkdownHelp:true
          );



          );













          draft saved

          draft discarded


















          StackExchange.ready(
          function ()
          StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f203325%2fdungeon-crawl-game-for-the-terminal%23new-answer', 'question_page');

          );

          Post as a guest















          Required, but never shown

























          2 Answers
          2






          active

          oldest

          votes








          2 Answers
          2






          active

          oldest

          votes









          active

          oldest

          votes






          active

          oldest

          votes









          18












          $begingroup$

          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;






          share|improve this answer











          $endgroup$












          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:55










          • $begingroup$
            Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one.
            $endgroup$
            – Angew
            Sep 8 '18 at 19:16










          • $begingroup$
            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.
            $endgroup$
            – user1118321
            Sep 8 '18 at 20:51















          18












          $begingroup$

          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;






          share|improve this answer











          $endgroup$












          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:55










          • $begingroup$
            Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one.
            $endgroup$
            – Angew
            Sep 8 '18 at 19:16










          • $begingroup$
            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.
            $endgroup$
            – user1118321
            Sep 8 '18 at 20:51













          18












          18








          18





          $begingroup$

          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;






          share|improve this answer











          $endgroup$



          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;







          share|improve this answer














          share|improve this answer



          share|improve this answer








          edited Sep 8 '18 at 20:52

























          answered Sep 8 '18 at 5:06









          user1118321user1118321

          11k11145




          11k11145











          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:55










          • $begingroup$
            Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one.
            $endgroup$
            – Angew
            Sep 8 '18 at 19:16










          • $begingroup$
            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.
            $endgroup$
            – user1118321
            Sep 8 '18 at 20:51
















          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:55










          • $begingroup$
            Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one.
            $endgroup$
            – Angew
            Sep 8 '18 at 19:16










          • $begingroup$
            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.
            $endgroup$
            – user1118321
            Sep 8 '18 at 20:51















          $begingroup$
          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.
          $endgroup$
          – Zanzag
          Sep 8 '18 at 8:55




          $begingroup$
          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.
          $endgroup$
          – Zanzag
          Sep 8 '18 at 8:55












          $begingroup$
          Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one.
          $endgroup$
          – Angew
          Sep 8 '18 at 19:16




          $begingroup$
          Note that designated initialisers (such as .position = /*...*/ ) are a C construct, not a C++ one.
          $endgroup$
          – Angew
          Sep 8 '18 at 19:16












          $begingroup$
          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.
          $endgroup$
          – user1118321
          Sep 8 '18 at 20:51




          $begingroup$
          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.
          $endgroup$
          – user1118321
          Sep 8 '18 at 20:51













          14












          $begingroup$

          That's an awesome little game!



          User experience



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




          Random hangs



          Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens randomly! We might 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 on 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 me 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 its 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 get 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're creating an anonymous struct, which looks pretty much the same as an existing struct (Location). You're 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 at least -Wall -Wextra -pedantic and edit your code until there are no warnings.



          [60-61] Weakly typed enums



          Here, you're 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 like 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 an std::mt19937 generator using an 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 = dist(gen);


          Since the seed is now stored in an 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>(dist(gen));



          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 elsewhere. 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 when 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 this 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!






          share|improve this answer











          $endgroup$












          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:54










          • $begingroup$
            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
            $endgroup$
            – Kerndog73
            Sep 8 '18 at 10:02










          • $begingroup$
            @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)
            $endgroup$
            – Peter Cordes
            Sep 9 '18 at 3:52















          14












          $begingroup$

          That's an awesome little game!



          User experience



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




          Random hangs



          Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens randomly! We might 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 on 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 me 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 its 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 get 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're creating an anonymous struct, which looks pretty much the same as an existing struct (Location). You're 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 at least -Wall -Wextra -pedantic and edit your code until there are no warnings.



          [60-61] Weakly typed enums



          Here, you're 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 like 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 an std::mt19937 generator using an 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 = dist(gen);


          Since the seed is now stored in an 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>(dist(gen));



          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 elsewhere. 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 when 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 this 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!






          share|improve this answer











          $endgroup$












          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:54










          • $begingroup$
            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
            $endgroup$
            – Kerndog73
            Sep 8 '18 at 10:02










          • $begingroup$
            @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)
            $endgroup$
            – Peter Cordes
            Sep 9 '18 at 3:52













          14












          14








          14





          $begingroup$

          That's an awesome little game!



          User experience



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




          Random hangs



          Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens randomly! We might 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 on 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 me 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 its 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 get 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're creating an anonymous struct, which looks pretty much the same as an existing struct (Location). You're 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 at least -Wall -Wextra -pedantic and edit your code until there are no warnings.



          [60-61] Weakly typed enums



          Here, you're 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 like 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 an std::mt19937 generator using an 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 = dist(gen);


          Since the seed is now stored in an 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>(dist(gen));



          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 elsewhere. 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 when 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 this 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!






          share|improve this answer











          $endgroup$



          That's an awesome little game!



          User experience



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




          Random hangs



          Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens randomly! We might 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 on 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 me 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 its 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 get 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're creating an anonymous struct, which looks pretty much the same as an existing struct (Location). You're 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 at least -Wall -Wextra -pedantic and edit your code until there are no warnings.



          [60-61] Weakly typed enums



          Here, you're 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 like 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 an std::mt19937 generator using an 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 = dist(gen);


          Since the seed is now stored in an 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>(dist(gen));



          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 elsewhere. 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 when 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 this 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!







          share|improve this answer














          share|improve this answer



          share|improve this answer








          edited 9 mins ago

























          answered Sep 8 '18 at 6:33









          Kerndog73Kerndog73

          616317




          616317











          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:54










          • $begingroup$
            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
            $endgroup$
            – Kerndog73
            Sep 8 '18 at 10:02










          • $begingroup$
            @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)
            $endgroup$
            – Peter Cordes
            Sep 9 '18 at 3:52
















          • $begingroup$
            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.
            $endgroup$
            – Zanzag
            Sep 8 '18 at 8:54










          • $begingroup$
            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
            $endgroup$
            – Kerndog73
            Sep 8 '18 at 10:02










          • $begingroup$
            @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)
            $endgroup$
            – Peter Cordes
            Sep 9 '18 at 3:52















          $begingroup$
          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.
          $endgroup$
          – Zanzag
          Sep 8 '18 at 8:54




          $begingroup$
          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.
          $endgroup$
          – Zanzag
          Sep 8 '18 at 8:54












          $begingroup$
          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
          $endgroup$
          – Kerndog73
          Sep 8 '18 at 10:02




          $begingroup$
          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
          $endgroup$
          – Kerndog73
          Sep 8 '18 at 10:02












          $begingroup$
          @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)
          $endgroup$
          – Peter Cordes
          Sep 9 '18 at 3:52




          $begingroup$
          @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)
          $endgroup$
          – Peter Cordes
          Sep 9 '18 at 3:52

















          draft saved

          draft discarded
















































          Thanks for contributing an answer to Code Review Stack Exchange!


          • Please be sure to answer the question. Provide details and share your research!

          But avoid


          • Asking for help, clarification, or responding to other answers.

          • Making statements based on opinion; back them up with references or personal experience.

          Use MathJax to format equations. MathJax reference.


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




          draft saved


          draft discarded














          StackExchange.ready(
          function ()
          StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f203325%2fdungeon-crawl-game-for-the-terminal%23new-answer', 'question_page');

          );

          Post as a guest















          Required, but never shown





















































          Required, but never shown














          Required, but never shown












          Required, but never shown







          Required, but never shown

































          Required, but never shown














          Required, but never shown












          Required, but never shown







          Required, but never shown







          Popular posts from this blog

          名間水力發電廠 目录 沿革 設施 鄰近設施 註釋 外部連結 导航菜单23°50′10″N 120°42′41″E / 23.83611°N 120.71139°E / 23.83611; 120.7113923°50′10″N 120°42′41″E / 23.83611°N 120.71139°E / 23.83611; 120.71139計畫概要原始内容臺灣第一座BOT 模式開發的水力發電廠-名間水力電廠名間水力發電廠 水利署首件BOT案原始内容《小檔案》名間電廠 首座BOT水力發電廠原始内容名間電廠BOT - 經濟部水利署中區水資源局

          Prove that NP is closed under karp reduction?Space(n) not closed under Karp reductions - what about NTime(n)?Class P is closed under rotation?Prove or disprove that $NL$ is closed under polynomial many-one reductions$mathbfNC_2$ is closed under log-space reductionOn Karp reductionwhen can I know if a class (complexity) is closed under reduction (cook/karp)Check if class $PSPACE$ is closed under polyonomially space reductionIs NPSPACE also closed under polynomial-time reduction and under log-space reduction?Prove PSPACE is closed under complement?Prove PSPACE is closed under union?

          Is my guitar’s action too high? Announcing the arrival of Valued Associate #679: Cesar Manara Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern)Strings too stiff on a recently purchased acoustic guitar | Cort AD880CEIs the action of my guitar really high?Μy little finger is too weak to play guitarWith guitar, how long should I give my fingers to strengthen / callous?When playing a fret the guitar sounds mutedPlaying (Barre) chords up the guitar neckI think my guitar strings are wound too tight and I can't play barre chordsF barre chord on an SG guitarHow to find to the right strings of a barre chord by feel?High action on higher fret on my steel acoustic guitar