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;
$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.
c++ game console homework
$endgroup$
add a comment |
$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.
c++ game console homework
$endgroup$
add a comment |
$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.
c++ game console homework
$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
c++ game console homework
edited Sep 8 '18 at 9:54
mdfst13
17.9k62257
17.9k62257
asked Sep 7 '18 at 23:39
ZanzagZanzag
1013
1013
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
$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;
$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
add a comment |
$begingroup$
That's an awesome little game!
User experience
Before we dive into the code, let's talk about the game itself.
Rand
om hangs
Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens rand
omly! 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 const
ant.
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 int
s 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!
$endgroup$
$begingroup$
Hi! Thank you for all the suggestions, I didn't know there existed strongly typedenums
. 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
add a comment |
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
);
);
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
var $window = $(window),
onScroll = function(e)
var $elem = $('.new-login-left'),
docViewTop = $window.scrollTop(),
docViewBottom = docViewTop + $window.height(),
elemTop = $elem.offset().top,
elemBottom = elemTop + $elem.height();
if ((docViewTop elemBottom))
StackExchange.using('gps', function() StackExchange.gps.track('embedded_signup_form.view', location: 'question_page' ); );
$window.unbind('scroll', onScroll);
;
$window.on('scroll', onScroll);
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
$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;
$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
add a comment |
$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;
$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
add a comment |
$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;
$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;
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
add a comment |
$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
add a comment |
$begingroup$
That's an awesome little game!
User experience
Before we dive into the code, let's talk about the game itself.
Rand
om hangs
Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens rand
omly! 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 const
ant.
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 int
s 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!
$endgroup$
$begingroup$
Hi! Thank you for all the suggestions, I didn't know there existed strongly typedenums
. 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
add a comment |
$begingroup$
That's an awesome little game!
User experience
Before we dive into the code, let's talk about the game itself.
Rand
om hangs
Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens rand
omly! 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 const
ant.
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 int
s 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!
$endgroup$
$begingroup$
Hi! Thank you for all the suggestions, I didn't know there existed strongly typedenums
. 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
add a comment |
$begingroup$
That's an awesome little game!
User experience
Before we dive into the code, let's talk about the game itself.
Rand
om hangs
Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens rand
omly! 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 const
ant.
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 int
s 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!
$endgroup$
That's an awesome little game!
User experience
Before we dive into the code, let's talk about the game itself.
Rand
om hangs
Sometimes, the executable hangs when I launch it. But only sometimes. It's almost as if it happens rand
omly! 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 const
ant.
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 int
s 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!
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 typedenums
. 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
add a comment |
$begingroup$
Hi! Thank you for all the suggestions, I didn't know there existed strongly typedenums
. 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
add a comment |
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.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
var $window = $(window),
onScroll = function(e)
var $elem = $('.new-login-left'),
docViewTop = $window.scrollTop(),
docViewBottom = docViewTop + $window.height(),
elemTop = $elem.offset().top,
elemBottom = elemTop + $elem.height();
if ((docViewTop elemBottom))
StackExchange.using('gps', function() StackExchange.gps.track('embedded_signup_form.view', location: 'question_page' ); );
$window.unbind('scroll', onScroll);
;
$window.on('scroll', onScroll);
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
var $window = $(window),
onScroll = function(e)
var $elem = $('.new-login-left'),
docViewTop = $window.scrollTop(),
docViewBottom = docViewTop + $window.height(),
elemTop = $elem.offset().top,
elemBottom = elemTop + $elem.height();
if ((docViewTop elemBottom))
StackExchange.using('gps', function() StackExchange.gps.track('embedded_signup_form.view', location: 'question_page' ); );
$window.unbind('scroll', onScroll);
;
$window.on('scroll', onScroll);
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
var $window = $(window),
onScroll = function(e)
var $elem = $('.new-login-left'),
docViewTop = $window.scrollTop(),
docViewBottom = docViewTop + $window.height(),
elemTop = $elem.offset().top,
elemBottom = elemTop + $elem.height();
if ((docViewTop elemBottom))
StackExchange.using('gps', function() StackExchange.gps.track('embedded_signup_form.view', location: 'question_page' ); );
$window.unbind('scroll', onScroll);
;
$window.on('scroll', onScroll);
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
var $window = $(window),
onScroll = function(e)
var $elem = $('.new-login-left'),
docViewTop = $window.scrollTop(),
docViewBottom = docViewTop + $window.height(),
elemTop = $elem.offset().top,
elemBottom = elemTop + $elem.height();
if ((docViewTop elemBottom))
StackExchange.using('gps', function() StackExchange.gps.track('embedded_signup_form.view', location: 'question_page' ); );
$window.unbind('scroll', onScroll);
;
$window.on('scroll', onScroll);
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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