mjhjmtu

I have created a simple version of the classic game Hunt the Wumpus in C++.
The rules for the game can be found on Code Golf SE.

The rules of the game are (from PPCG):

The player starts in a random room on an icosahedral map (thus there are 20 rooms in total, connected to each other like the faces of an icosahedron, and every room has exactly three exits).

The wumpus starts in a randomly selected different room. The wumpus stinks, and its odor can be detected in any of the three rooms adjacent to its location, though the direction of the odor is impossible for the player to determine. The game reports only "you smell a wumpus."

The player carries a bow and an infinite number of arrows, which he may shoot at any time into the room in front of him. If the wumpus is in that room, it dies and the player wins. If the wumpus was not in that room, it is startled and moves randomly into any of the three rooms connected to its current location.

One, randomly selected room (guaranteed not to be the room in which the player starts) contains a bottomless pit. If the player is in any room adjacent to the pit, he feels a breeze, but gets no clue as to which door the breeze came from. If he walks into the room with the pit, he dies and wumpus wins. The wumpus is unaffected by the pit.

If the player walks into the wumpus's room, or if the wumpus walks into the player's room, the wumpus wins.

This is the first non-trivial program I have created and would like to know what I could have done better.

Does my code follow the best practices? Is there anything I can do to clean it up? Can there be any improvements?

#include<iostream>
#include<vector>
#include<algorithm>
#include<limits>

constexpr int ROOMS = 20;
constexpr int BATS = 3;
constexpr int PITS = 3;

constexpr int END_GAME = -1;

struct Room

 std::vector<int>adjRoomsstd::vector<int>(3);
 bool playerfalse;
 bool batfalse;
 bool wumpfalse;
 bool pitfalse;
;

class Player

 std::vector<int> adjRoomsstd::vector<int>(3);
 int currRoom;
 void setAdjRooms();
public:
 void setCurrRoom(int r)currRoom = r; setAdjRooms();

 int room() const return currRoom;
 int getAdj(int i) const return adjRooms[i];
;

void Player::setAdjRooms()

 int t = 2+2*(currRoom&1);
 adjRooms[0] = ROOMS-1-currRoom;
 adjRooms[1] = (currRoom+t)%ROOMS;
 adjRooms[2] = (currRoom-t+20)%ROOMS;


class Map

 std::vector<Room> cavestd::vector<Room>(20);
 std::vector<int> vacant; //vector to keep track of empty rooms

 Player p;

 void addWump();
 void addBats();
 void addPits();
 void addPlayer();

 void reportState();
 int input();

 int movePlayer(int);
 int shoot(int target);
 void batEncounter();
 int moveWump();
public:
 void init();
 void play();
 void printState(); //Only for debugging. Not part of the game.
;

void Map::addPlayer()
//spawn player

 int r = rand()%vacant.size();
 cave[vacant[r]].player = true;
 p.setCurrRoom(vacant[r]);

 //std::cout<<"Player in room "<<vacant[r]<<std::endl;
 vacant.erase(vacant.begin()+r);

 //no enemies should spawn adjacent to player
 for(int i = 0; i < 3; ++i)
 vacant.erase(std::find(vacant.begin(),vacant.end(),p.getAdj(i)));


void Map::addWump()
//spawns the wumpus in a random room

 int r = rand()%vacant.size();
 cave[vacant[r]].wump = true;
 //std::cout<<"Wumpus in room "<<vacant[r]<<std::endl;
 vacant.erase(vacant.begin()+r); //remove vacancy


void Map::addBats()
//spawns bats

 for(int i = 0; i < BATS; ++i)
 int r = rand()%vacant.size();
 cave[vacant[r]].bat = true;
 //std::cout<<"Bat in room "<<vacant[r]<<std::endl;
 vacant.erase(vacant.begin()+r);
 


void Map::addPits()
//place pits

 for(int i = 0; i < PITS; ++i)
 int r = rand()%vacant.size();
 cave[vacant[r]].pit = true;
 //std::cout<<"Pit in room "<<vacant[r]<<std::endl;
 vacant.erase(vacant.begin()+r);
 


void Map::printState()
//for debugging

 for(int i = 0; i < ROOMS; ++i)
 std::cout << "Room #" << i << ":" << std::endl;
 std::cout << "tWumpus -> " << ((cave[i].wump)?"yes":"no") << std::endl;
 std::cout << "tBat -> " << ((cave[i].bat)?"yes":"no") << std::endl;
 std::cout << "tPit -> " << ((cave[i].pit)?"yes":"no") << std::endl;
 std::cout << "tPlayer -> " << ((cave[i].player)?"yes":"no") << std::endl;
 std::cout << "tAdjacent Rooms -> " <<(cave[i].adjRooms[0])<<", "
 <<(cave[i].adjRooms[1])<<", "<<cave[i].adjRooms[2]<<std::endl;
 std::cout << std::endl;
 


void Map::reportState()


int Map::movePlayer(int pos)

 if(pos != p.getAdj(0) && pos != p.getAdj(1) && pos != p.getAdj(2))
 std::cout << "Invalid choice. Please move to an ADJACENT room." << std::endl;
 return 0;
 
 cave[p.room()].player = false;
 cave[pos].player = true;
 vacant.push_back(p.room());
 p.setCurrRoom(pos);

 if(cave[p.room()].wump)
 std::cout << "The Wumpus got you! YOU LOSE." << std::endl;
 return END_GAME;
 
 if(cave[p.room()].pit)
 std::cout << "You fell into a bottomless pit! YOU LOSE." << std::endl;
 return END_GAME;
 
 if(cave[p.room()].bat)
 std::cout << "A giant bat takes you to another room!" << std::endl;
 batEncounter();
 return 0;
 


int Map::moveWump()
//move wumpus to a random adjacent room
 (cave[pos].wump && !(cave[pos].pit)))
 vacant.push_back(pos);
 cave[cave[pos].adjRooms[r]].wump = true;
 if(cave[cave[pos].adjRooms[r]].player)
 std::cout << "The Wumpus got you! YOU LOSE." << std::endl;
 return END_GAME;
 
 return 0;


int Map::shoot(int target)
 cave[p.getAdj(2)].wump)
 return moveWump();


void Map::batEncounter()

 int r = rand()%vacant.size();
 cave[p.room()].player = false;
 vacant.push_back(p.room());
 cave[vacant[r]].player = true;
 p.setCurrRoom(vacant[r]);
 vacant.erase(vacant.begin()+r);


void Map::init()
//set up map
//place player, bats, pits and the wumpus

 //generate the dodecahedral cave
 for(int i = 0; i < ROOMS; ++i)
 int t = 2+2*(i&1);
 cave[i].adjRooms[0] = ROOMS-1-i;
 cave[i].adjRooms[1] = (i+t)%ROOMS;
 cave[i].adjRooms[2] = (i-t+20)%ROOMS;
 vacant.push_back(i);
 

 //add entities
 addPlayer();
 addWump();
 addBats();
 addPits();

 //restore vacant rooms adjacent to player
 for(int i = 0; i < 3; ++i)
 vacant.push_back(p.getAdj(i));


void Map::play()

 reportState();
 while(input() != END_GAME)
 reportState();
 


int Map::input()

 char c = 0;
 int r = -1;
 std::cout << "Type mXX(sXX) to move(shoot) to(at) room XX." << std::endl;
 while(1)
 return (c == 'm') ? movePlayer(r) : shoot(r);


int main()

 srand(unsigned(time(0)));
 Map game;
 game.init();
 game.play();
 //game.printState();

• #include <ctime> (for time()).




• The C++ versions of standard functions are declared in the std namespace, not the global namespace, so we should use std::srand and std::time.



• 
  

  We can use the C++11 random number generation facilities in <random>, rather than srand and rand to generate random numbers. e.g.

  
  
  

  std::mt19937 rng(std::random_device()()); // seed the random number generator (do this once)
  
  ...
  std::uniform_int_distribution<int> dist(0, vacant.size() - 1);
  int r = dist(rng); // generate an int with the given distribution
  

  
  



• int is not an appropriate type to store room indices. We should use the index type of the container (i.e. std::vector<Room>::size_type, which is std::size_t), since that covers the correct range of values.




• Constant variables (ROOMS, BATS, etc.) are better than #defines, but there's no reason these can't be normal member variables (e.g. in the Map class). This would, for example, give us the flexibility to start a new game with different number of rooms or pits.




• The Player stores the index of the current room. Each Room stores the adjacent room indices. There is therefore no reason for the Player class to store adjacent room indices as well. We can get these from the Map instead.




• warning C4715: 'Map::movePlayer': not all control paths return a value, warning C4715: 'Map::shoot': not all control paths return a value - we should fix that!

Map:

• Map is a slightly misleading name, since this class largely handles game logic. Perhaps the game logic could be split into a Game class, or the class itself renamed Game.




• Use the Map constructor to do initialization, removing the need to call a separate init function.




• One would expect the vector of Rooms in Map to be called rooms, not cave.




• Since the bats, wumpus, and pit can all coexist, the Map::add* functions may be slightly wrong - we only need to place a bat in a room with no other bats.




• We should probably check that we don't run out of vacant rooms in which to place things.




• Map::input returns an integer value. However, we're not using it as a number, but to represent game state. C++ has enums for this purpose, e.g. enum class MoveResult END_GAME, CONTINUE , and we should return one of these instead.

bit-flags:

• 
  

  There is some duplication of code when referring to the .bat, .pit, .wump members of Room. It would be nice to remove this, and abstract some more functionality (e.g. checking for an adjacent feature) into a single function. This would be easier if we used bit-flags for the room contents. e.g.:

  
  
  

  enum RoomFeatures
  
   Player = 1 << 0,
   Bat = 1 << 1,
   Pit = 1 << 2,
   Wumpus = 1 << 3,
  ;
  
  struct Room
  
   ...
   RoomFeatures features;
  ;
  
  ...
  
  room.features |= RoomFeatures::Bat; // add bat to room
  if (room.features & RoomFeatures::Pit) ... // test for pit
  room.features &= ~RoomFeatures::Wumpus // remove wumpus from room
  

  
  
  

  While the bitwise operators are admittedly rather awkward, we could wrap this in a neat interface, which lets us do something like this:

  
  
  

  bool Map::isAdjacentTo(int roomIndex, RoomFeatures feature) const
  
   for (auto i : cave[roomIndex].adjRooms)
   if (cave[i].contains(feature)) // `bool Room::contains() const` tests the bit-flag
   return true;
  
   return false;
  
  
  ...
  
  // e.g. in reportState
  if (isAdjacentTo(p.room(), RoomFeatures::Bat))
   std::cout ... ;
  

  
  

• 
  

  It would be nice to separate the input and output from the game logic:

  
  
  

  • At the moment we output messages about the player losing or winning in Map::shoot, Map::moveWump, and Map::movePlayer .

  
  

  • movePlayer() and shoot() are called from the input function, rather than as part of the main loop as one might expect.

  
  

I think your solution is a good start for a first bigger programming task. I'd probably structure the program a little bit differently but I'm not sure if this required or even good. For the size of the problem the layout really is appropriate (meaning exactly right).

I've found some stuff nevertheless. Here you go (simply search for the code parts):

Bugs

• in if((cave[pos].wump && !(cave[pos].bat)) || (cave[pos].wump && !(cave[pos].pit)))
the expression cave[pos].wump is always false, hence the line vacant.push_back(pos); is never run


• restoring the vacant rooms in Map::Init() does not consider non-player entities. If the player spawns by accident next to wumpus, the room will be considered vacant by the game.


• wrong input in Map::input() will lead to shoot being called. The input method should only return if the input is indeed acceptable. Ideally input returns some abstract command structure or is renamed to something like handleInput().

Possible Bugs

• in for(; !(cave[pos].wump); ++pos); add a check for the end of cave
  


• 
  Map::MovePlayer() and Map::shoot() contain code paths that may not always return a value

Style Improvements

• use same name for a member in all locations, e.g. setCurrRoom() and room(). Note that room() offers enough information and is much shorter than currRoom
  


• use ROOMS only for initialization of the cave, stick to cave.size() in all other places


• it makes sense to somehow mark the member variables to make them distinguishable from local variables and globals, prepending m_ to the variable name is common: vacant -> m_vacant.


• add more whitespace: newlines and spaces can be used to show parts of a function that belong logically together


• You can reduce !(cave[pos].wump) to !cave[pos].wump.

General Hints

• you can hide debugging code behind a compiler switch:

#ifdef DEBUG
game.printState();
#endif

If you compile with -DDEBUG you will generate debug output