C++ Hunt the Wumpus

Question

I have been writing Hunt the Wumpus game in C++, and i think it's finished. It's first non-trivial project, and it DOES work properly (at least 20 hours of debugging showed so). How can the code be more clean or simply better ? Rules are in the game.cpp.

Any criticism or suggestions are welcome.

game.cpp

#pragma once
#include "heart.h"
#include <iostream>
#include <ctime>


void rules()
{
    {
        std::cout << "Rules:" << std::endl
            << "1. You are in a cave system(a dodecahedron) such that each cave connects to three other caves.\n"
            << "2. Somewhere in the cave lives a dreaful monster - Wumpus.\nHe has sucker feet to cling to the walls of bottomless pits and is too heavy to be picked up by super - bats.\nHe also has has big teet thand will eat you if you are in the same room with him.\n"
            << "3. In the cave system, two caves are bottomless pits. Step in cave with one = you are scattered into pieces (Game Over).\n"
            << "4. Also, two caves contain super - bats, which will carry you off to a random cave.\nBats can carry you to any room, including Pit room and Wampus room, which will result in death (Game Over).\n"
            << "5. If the adjacent room to your position contains any of three events (wampus, bat, pit), you will be notified.\n"
            << "6. Only one instance of event can spawn at a room, but Wampus can move to both bat and pit rooms and remain there.\n"
            << "7. You have 5 arrows.\nWhen you think you know where the Wumpus is(or even if you don't) shoot an arrow into a room.\nIf you hit the Wumpus, he will die and you win the game (Game WON).\n"
            << "8. If you miss, the Wumpus, who is also a very light sleeper:\n1)Will wakeup and move(75%) to one of his adjucent rooms \n2) Will continue sleeping (25%). \nWampus can move in your room, so you would become a delicious snack (Game Over).\n"
            << "9. The arrows are magical, so you choose three destinations, knowing where they would fly.\n"
            << "10. If all arrows are gone, you are defencelence and therefore die of panic (Game Over).\n" << endl
            << " Notes: " << std::endl 
            << "1. If Wumpus moved to a bat room, then player also moved to this room (containing both bat and wampus already) -\nbat logic activates, so you are saved...probably.\n"
            << "GOOD LUCK, YUMMY HUMAN...\n" << std::endl;
    }
}
int main()
{
    rules();
    while (true)
    {
        //srand(time(0));
        Heart heart;
        heart.play();
        std::cout << "Want to play again? y / n" << std::endl;
        char answer;
        cin >> answer;
        if (answer == 'y') continue;
        else if (answer == 'n') return static_cast<int>(Heart::EXIT);
    }
}

heart.h

#pragma once
#include "Room.h"
#include "Player.h"
class Heart
{
public:
    enum CONFIG { SHUT_DOWN = 1, EXIT, LOST, VICTORY, CONTINUE };
    vector<vector<int> >roomsInitializeArray
    {
        //Room 1   Room 2    Room 3    Room 4    Room 5 
       {2,5,8}, {1,3,10}, {2,4,12}, {3,5,14}, {1,4,6},
       //Room 6   Room 7    Room 8    Room 9    Room 10 
      {5,7,15}, {6,8,17}, {1,7,9}, {8,10,18}, {2,9,11},
      // Room 11    Room 12     Room 13    Room 14    Room 15 
       {10,12,19}, {3,11,13}, {12,14,20}, {4,13,15}, {6,14,16},
       // Room 16    Room 17     Room 18    Room 19    Room 20 
        {15,17,20}, {7,16,18}, {9,17,19}, {11,18,20}, {13,16,19},
    };
    Heart();
    Player p;

    void initializeRooms();
    int validRoom();
    void fillRooms();

    void moveWumpus();
    void batEncounter();

    CONFIG playerMove(int);
    CONFIG moveResult();
    CONFIG playerShoot(int, int, int);
    bool flyCheck(int, int);

    CONFIG input();
    CONFIG inputError();
    void hints();
    void debug();

    CONFIG play();
};

Room.h

#pragma once
#include <vector>
using namespace std;

const int ROOMS = 20;
class Room
{
private:
    static int roomCount;
    int roomNumber;
    bool player;
    bool bat;
    bool pit;
    bool wumpus;
public:
    vector<int>adjRooms = vector<int>(3);
    Room(vector<int>);
    bool hasBats();
    bool hasPit();
    bool hasWumpus();
    bool hasPlayer();
    int getRoomNumber();
    bool empty();

    void setBats(bool);
    void setPit();
    void setWumpus(bool);
    void setPlayer(bool);
};

Player.h

#pragma once
class Player
{
    int arrows = 5;
    int currRoom;
public:
    Player();
    int currPlayer() const;
    void setLocation(int);
    void fired();
    int getArrows();
};

Heart.cpp

#include "heart.h"
#include <random>
#include <iostream>
using namespace std;

vector<Room> labyrinth;

Heart::Heart()
{
    initializeRooms();
    fillRooms();
}
void Heart::initializeRooms()
{
    //copy from hardcoded vector
    for (int i = 0; i < ROOMS; i++)
    {
        // labyrinth[i] = roomsInitializeArray[i]; either way
        Room newRoom = roomsInitializeArray[i];
        labyrinth.push_back(newRoom);
    }
}

int Heart::validRoom()
{
    // checks random room for emptiness; if empty = return index, else recursion
    int room = rand() % ROOMS;
    if (labyrinth[room].empty())
        return room;
    else validRoom();
}

void Heart::fillRooms()
{
    // set up everything (bool true in valid rooms)
    p.setLocation(validRoom());
    labyrinth[p.currPlayer()].setPlayer(true);
    labyrinth[validRoom()].setBats(true);
    labyrinth[validRoom()].setBats(true);
    labyrinth[validRoom()].setPit();
    labyrinth[validRoom()].setPit();
    labyrinth[validRoom()].setWumpus(true);
}

void Heart::moveWumpus()
{
    // 25% chance to move to one of three rooms or remain in the same
    int tmp = rand() % 4;
    int currWumpus;
    for (int i = 0; i < ROOMS; ++i)
    {
        if (labyrinth[i].hasWumpus())
        {
            currWumpus = i;
            break;
        }
    }

    labyrinth[currWumpus].setWumpus(false);

    if (tmp == 0 or tmp == 1 or tmp == 2)
    {
        labyrinth[labyrinth[currWumpus].adjRooms[tmp]].setWumpus(true);
        cout << "Wampus moved to another room..." << endl;

    }
    else if (tmp == 3)
    {
        labyrinth[currWumpus].setWumpus(true);
        cout << "Wampus felt no danger & didn't move to another room" << endl;
    }
}

Heart::CONFIG Heart::inputError()
{
    // Called if input is invalid. Clears buffer and passes continue instruction to play() loop (simply re-calls input()). 
    cout << "Wrong input" << endl;
    cin.clear();
    cin.ignore(32767, '\n');
    return CONTINUE;
};

bool Heart::flyCheck(int target, int start)
{
    // returns true if destination can be chosen
    // may be odd, however used to simplify shootArrow()
    return(target == labyrinth[start].adjRooms[0] || target == labyrinth[start].adjRooms[1] || target == labyrinth[start].adjRooms[2]);
}

Heart::CONFIG Heart::input()
{
    // might be rewrited
    // asks for input, checks if input is valid, finally laucnhes logic according to input ; if wrong - calls inputError()
    cout << "Move(m) or Shoot(s) ?" << endl;
    cout << "To terminate the game enter (T)" << endl;
    char action;
    cin >> action;
    if (action == 'T') return SHUT_DOWN;
    int room;
    int target1;
    int target2;
    int target3;
    if (action == 'm')
    {
        cout << "Where to ?" << endl;
        cin >> room;
        if (cin.fail()) inputError();
        else return playerMove(--room); // f.e. player inputs room 1, but program works with it as room[0], therefore room is decremented by 1 
    }
    else if (action == 's')
    {
        cout << "Where at ?" << endl;

        cin >> target1;

        if (cin.fail()) inputError();
        --target1; // lowering by one just as the 'm' case

        if (flyCheck(target1, p.currPlayer()))
        {
            cout << "Arrow in room # " << target1 + 1
                << ". Enter next destination room: " << labyrinth[target1].adjRooms[0] + 1 << ", "  //+1 is made for proper cout (logic works with [index]
                << labyrinth[target1].adjRooms[1] + 1 << ", "   // cout is index[1] (Room[0] = Room#1)
                << labyrinth[target1].adjRooms[2] + 1 << endl;
        }
        else inputError();

        cin >> target2; // we get there only after we do flyCheck() with previous target

        if (cin.fail()) inputError();
        --target2; // do it for each cin

        if (flyCheck(target2, target1))
        {
            cout << "Arrow in room # " << target2 + 1
                << ". Enter final destination room: " << labyrinth[target2].adjRooms[0] + 1 << ", "
                << labyrinth[target2].adjRooms[1] + 1 << ", "
                << labyrinth[target2].adjRooms[2] + 1 << endl;
        }
        else inputError();

        cin >> target3;
        if (cin.fail()) inputError();
        --target3;
        if (flyCheck(target3, target2)) return playerShoot(target1, target2, target3);  // if all 3 targets are valid, perform a shot.
        else inputError();
    }
    else inputError();
}


void Heart::hints()
{
    // +1 is used for proper cout, Room[0] represented as Room # 1.
    cout << "You are in the room # " << p.currPlayer() + 1 << endl;
    cout << "Adjacent rooms are: " << labyrinth[p.currPlayer()].adjRooms[0] + 1 << " , " << labyrinth[p.currPlayer()].adjRooms[1] + 1 << " , " << labyrinth[p.currPlayer()].adjRooms[2] + 1 << endl;
    cout << "You have " << p.getArrows() << " arrows left" << endl << endl;
    // if more than two same objects, received only one message (f.e. two pit rooms = only one message)
    if (labyrinth[labyrinth[p.currPlayer()].adjRooms[0]].hasBats() || 
        labyrinth[labyrinth[p.currPlayer()].adjRooms[1]].hasBats() || 
        labyrinth[labyrinth[p.currPlayer()].adjRooms[2]].hasBats())
        cout << "You hear flapping wings..." << endl << endl;

    if (labyrinth[labyrinth[p.currPlayer()].adjRooms[0]].hasPit() || 
        labyrinth[labyrinth[p.currPlayer()].adjRooms[1]].hasPit() || 
        labyrinth[labyrinth[p.currPlayer()].adjRooms[2]].hasPit())
        cout << "You feel wind..." << endl << endl;

    if (labyrinth[labyrinth[p.currPlayer()].adjRooms[0]].hasWumpus() ||
        labyrinth[labyrinth[p.currPlayer()].adjRooms[1]].hasWumpus() || 
        labyrinth[labyrinth[p.currPlayer()].adjRooms[2]].hasWumpus())
        cout << "You feel the dread within..." << endl << endl;
}

Heart::CONFIG Heart::playerMove(int newLocation)
{
    //  This ?can? be somehow made into Player member function.
    if (newLocation != labyrinth[p.currPlayer()].adjRooms[0] && newLocation != labyrinth[p.currPlayer()].adjRooms[1] && newLocation != labyrinth[p.currPlayer()].adjRooms[2])
    {
        cout << "Wrong movement!";// << labyrinth[p.currPlayer()].adjRooms[0] << ', ' << labyrinth[p.currPlayer()].adjRooms[1] << ', ' << labyrinth[p.currPlayer()].adjRooms[2] << endl;
        return CONTINUE;
    }
    labyrinth[p.currPlayer()].setPlayer(false);
    labyrinth[newLocation].setPlayer(true);
    p.setLocation(newLocation);
    moveResult();
}


void Heart::batEncounter()
{
    // change player position randomly (no emptyness check)
    int r = rand() % ROOMS;
    labyrinth[p.currPlayer()].setPlayer(false);
    labyrinth[p.currPlayer()].setBats(false);
    labyrinth[r].setPlayer(true);
    p.setLocation(r);
    debug();
    moveResult();
}

Heart::CONFIG Heart::moveResult()
{
    // event reaction
    if (labyrinth[p.currPlayer()].hasBats())
    {
        cout << "You moved to a room with Huge Bat.\nYou are flying! But where...\n" << endl;
        batEncounter();
    }
    if (labyrinth[p.currPlayer()].hasPit())
    {
        cout << "You moved to a room with Pit.\nVZUH!\nYou are now many pieces.\n" << endl;
        return LOST;
    }
    if (labyrinth[p.currPlayer()].hasWumpus())
    {
        cout << "You moved to a room with Wumpus.\nMhm... Yummy Human...\nYOU ARE EATEN\n" << endl;
        return LOST;
    }
        return CONTINUE;
}


Heart::CONFIG Heart::playerShoot(int target1, int target2, int target3)
{
    // shoots an arrow, checks if wampus was hit, WIN if true, else check if Wampus moved to player, LOST if true, else check remaining arrows, LOST if == 0, else continue play() loop
    p.fired();

    if (labyrinth[target1].hasWumpus() || labyrinth[target2].hasWumpus() || labyrinth[target3].hasWumpus())
    {
        cout << "Target practice!\nWampus dead!\nCongratulations!" << endl;
        return VICTORY;
    }
    else
    {
        cout << "A miss... what a pity." << endl;
        moveWumpus();
        if (labyrinth[p.currPlayer()].hasWumpus())
        {
            cout << "YOUR ROOM!\nToo late to run, yummy human" << endl;
            return LOST;
        }
        if (p.getArrows() == 0)
        {
            cout << "No more arrows...\nYou are defenceless!" << endl;
            return LOST;
        }
        return CONTINUE;
    }
}

void Heart::debug()
{
    // only for debug, unused logic
    for (int i = 0; i < ROOMS; ++i)
    {
        cout << "Room# " << i << ":\n";
        cout << "Bats: " << labyrinth[i].hasBats() << endl;
        cout << "Wumpus: " << labyrinth[i].hasWumpus() << endl;
        cout << "Pits: " << labyrinth[i].hasPit() << endl;
        cout << "Player: " << labyrinth[i].hasPlayer() << endl;
        cout << endl << endl;
    }
}

Heart::CONFIG Heart::play()
{
    // check input results due to Heart::CONFIG enum, finishes game in LOST / VICTORY / SHUT_DOWN cases
    while (true)
    {
        hints();
        CONFIG result = input();
        if (result == CONTINUE) continue;
        else if (result == LOST)
        {
            cout << "YOU LOST" << endl << endl;
            break;
        }
        else if (result == VICTORY)
        {
            cout << "YOU WON" << endl << endl;
            break;
        }
        else if (result == SHUT_DOWN) return EXIT;
    }
}

Room.cpp

#include "Room.h"
#include <vector>
using namespace std;

int Room::roomCount = 1;

Room::Room(vector<int> v) // accept vector in case of adjucent changes, however only works with dodecahedrone hardcode;
    :player(false), bat(false), wumpus(false), pit(false), 
        adjRooms{v[0]-1, v[1]-1, v[2]-1}, roomNumber(roomCount++)//Off by one problem sollution //(initializing vector begins with Room#1 but [0])
{}


bool Room::hasBats()
{
    return bat;
}

bool Room::hasPit()
{
    return pit;
}

bool Room::hasWumpus()
{
    return wumpus;
}

bool Room::hasPlayer()
{
    return player;
}

int Room::getRoomNumber()
{
    return roomNumber;
}

bool Room::empty()
{
    return (!hasBats() && !hasPit() && !hasPlayer() && !hasWumpus());
}


void Room::setBats(bool status)
{
    bat = status;
}

void Room::setPit()
{
    pit = true;
}

void Room::setWumpus(bool status)
{
    wumpus = status;
}

void Room::setPlayer(bool status)
{
    player = status;
}

Player.cpp

#include "Player.h"

Player::Player()
    :arrows(5), currRoom(-1) {}

int Player::currPlayer() const
{
    return currRoom;
}

void Player::setLocation(int newLocation)
{
    currRoom = newLocation;
}

void Player::fired()
{
    arrows--;
}

int Player::getArrows()
{
    return arrows;
}

As I see the improvements, they might be:

1. Make labyrinth[x].method() into a function with accepted x as room number and method as method. However [x] is sometimes a function, and method() is often an adjRooms[], what is basically and integer.
2. CONST, references& and pointers*, everywhere. Everything is being copied over and over again, it's a long work, but can be done (if it's better).
3. As far as I know, the Interface should be separate with game logic, and input() function should NOT std::cout, however I have no idea how to do it (if it's needed of course).

BOTH CRITICISM AND RECOMMENDATIONS ARE HIGLY APPRECIATED

Answer 1

General Observations

When you compile make sure to check for all warnings (-Wall compiler switch). There are multiple warnings that indicate possible logic errors:

Heart.cpp(31): warning C4715: 'Heart::validRoom': not all control paths return a value
Heart.cpp(187): warning C4715: 'Heart::playerMove': not all control paths return a value
Heart.cpp(149): warning C4715: 'Heart::input': not all control paths return a value
Heart.cpp(286): warning C4715: 'Heart::play': not all control paths return a value

These warnings indicate that a function that returns a value doesn't always return a value which can cause unknown behavior.

Limit the width of the output to 80 characters, default text screens many times are at most 80 characters wide.

My Visual Studio editor is pretty wide but there are lines in the rules function that are wider than 185 characters. Try to keep lines in a program under 80 characters and definitely under 135 characters.

There is no reason to ever put #pragma once in a C++ source files, C++ source files should never be included by other files.

Be consistent, there is code that uses std::cout in main() but 2 lines lower there is cin without std::. In the rules() function the code mixes \n with std::endl and one endl.

Perfer \n over std::endl except perhaps as the last line of the rules() function. This is for performance reasons, std::endl flushes the output which is a system call and slows down the program. On a time sharing system the program may be swapped out during or after a system call.

The call to srand(time(0)) is currently commented out, which indicates the code isn't quite ready for review. There are also better random number generators to use in C++, srand() and rand() are C library functions and generally shouldn't be used in C++.

The function Heart::input() should be broken up into multiple functions, it does too much.

Avoid using namespace std;

If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.

Do not put using namspace std; in a header file (Room.h)!

Answer 2

I don't think we need a whole function rules(). Consider a simple constant string we can print:

static auto const rules =
    "Rules:\n"
    "1. You are in a cave system (a dodecahedron) such that each cave connects to three other caves.\n"
    "2. Somewhere in the cave lives a dreadful monster - Wumpus.\n"
    "   He has sucker feet to cling to the walls of bottomless pits and is too heavy to be picked up by super-bats.\n"
    "   He also has has big teeth and will eat you if you are in the same room as him.\n"
    "3. In the cave system, two caves are bottomless pits. Step in cave with one = you are scattered into pieces (Game Over).\n"
    "4. Also, two caves contain super-bats, which will carry you off to a random cave.\n"
    "   Bats can carry you to any room, including Pit room and Wumpus room, which will result in death (Game Over).\n"
    "5. If the adjacent room to your position contains any of three events (Wumpus, bat, pit), you will be notified.\n"
    "6. Only one instance of event can spawn at a room, but Wumpus can move to both bat and pit rooms and remain there.\n"
    "7. You have 5 arrows.\n"
    "   When you think you know where the Wumpus is (or even if you don't) shoot an arrow into a room.\n"
    "   If you hit the Wumpus, he will die and you win the game (Game WON).\n"
    "8. If you miss, the Wumpus, who is also a very light sleeper:\n"
    "     1) Will wake up and move (75%) to one of his adjacent rooms, or\n"
    "     2) Will continue sleeping (25%).\n"
    "   Wumpus can move in your room, so you would become a delicious snack (Game Over).\n"
    "9. The arrows are magical, so you choose three destinations, knowing where they would fly.\n"
    "10. If all arrows are gone, you are defenceless and therefore die of panic (Game Over).\n"
    "\n"
    " Notes: \n"
    "1. If Wumpus moved to a bat room, then player also moved to this room (containing both bat and Wumpus already) -\n"
    "   bat logic activates, so you are saved...probably.\n"
    "GOOD LUCK, YUMMY HUMAN...\n"
    "\n";

int main()
{
    std::cout << rules;

(I also fixed the spelling and grammar there)

---

I don't see why we need to return static_cast<int>(Heart::EXIT) when the player doesn't want to play any more. A simple 0, or EXIT_SUCCESS from the standard library would be a better choice, since refusing to play shouldn't be an error condition.

---

I think that labyrinth should be a member of Heart rather than a global variable shared amongst all hearts. We could initialise it directly, rather than needing initializeRooms():

    std::vector<Room> labyrinth{{
            //Rooms 1 to 5
            Room{{2,5,8}}, Room{{1,3,10}}, Room{{2,4,12}}, Room{{3,5,14}}, Room{{1,4,6}},
            //Rooms 6 to 10
            Room{{5,7,15}}, Room{{6,8,17}}, Room{{1,7,9}}, Room{{8,10,18}}, Room{{2,9,11}},
            // Rooms 11 to 15
            Room{{10,12,19}}, Room{{3,11,13}}, Room{{12,14,20}}, Room{{4,13,15}}, Room{{6,14,16}},
            // Rooms 16 to 20
            Room{{15,17,20}}, Room{{7,16,18}}, Room{{9,17,19}}, Room{{11,18,20}}, Room{{13,16,19}},
        }};

There's an inconsistency in Room: we have vector<int> adjRooms, suggesting a room can be connected to any number of other rooms. But everywhere else in the code assumes that adjRooms.size() is exactly 3. Either we should have more general code, or we should replace the vector with an array of size 3.

That also makes the above initialiser simpler, because we could just provide 3 arguments to the Room constructor (e.g. Room{2,5,8}) instead of a vector instance.

---

(partial review - that's as far as I got when Real Life intervened).

Answer 3

I would take Toby's advice but one step further and use RAW string literals.

static auto const rules = R"RULES(
Rules:
1. You are in a cave system (a dodecahedron) such that each cave connects to three other caves.
2. Somewhere in the cave lives a dreadful monster - Wumpus.
   He has sucker feet to cling to the walls of bottomless pits and is too heavy to be picked up by super-bats.
   He also has has big teeth and will eat you if you are in the same room as him.
3. In the cave system, two caves are bottomless pits. Step in cave with one = you are scattered into pieces (Game Over).
4. Also, two caves contain super-bats, which will carry you off to a random cave.
   Bats can carry you to any room, including Pit room and Wumpus room, which will result in death (Game Over).
5. If the adjacent room to your position contains any of three events (Wumpus, bat, pit), you will be notified.
6. Only one instance of event can spawn at a room, but Wumpus can move to both bat and pit rooms and remain there.
7. You have 5 arrows.
   When you think you know where the Wumpus is (or even if you don't) shoot an arrow into a room.
   If you hit the Wumpus, he will die and you win the game (Game WON).
8. If you miss, the Wumpus, who is also a very light sleeper:
     1) Will wake up and move (75%) to one of his adjacent rooms, or
     2) Will continue sleeping (25%).
   Wumpus can move in your room, so you would become a delicious snack (Game Over).
9. The arrows are magical, so you choose three destinations, knowing where they would fly.
10. If all arrows are gone, you are defenceless and therefore die of panic (Game Over).

 Notes: 
1. If Wumpus moved to a bat room, then player also moved to this room (containing both bat and Wumpus already) -
   bat logic activates, so you are saved...probably.
GOOD LUCK, YUMMY HUMAN...

)RULES";

int main()
{
    std::cout << rules;

This way you don't need to worry about new line character (or any escaping) and making sure each line is quoted. You just type exactly what you want the text to look like.

---

This worries me:

    //Room 1   Room 2    Room 3    Room 4    Room 5 
   {2,5,8}, {1,3,10}, {2,4,12}, {3,5,14}, {1,4,6},

In general containers are indexed from zero not one.

I have not read your code to see if you take this into account or not but it seems it would be nice if everything was zero indexed any way. I would simply add the zero room that has no connections.

 // Room 0
 {0, 0, 0},  // hopefully you don't get stuck here!!!!

---

Only include headers you need:

#pragma once
#include "Room.h"    // You don't use Room in this file.
                     // So don't include the header file.
#include "Player.h"
class Heart

---