Text based game “Hunt the Wumpus” Version 2

Question

This is a follow-up of: Text based game "Hunt the Wumpus"

A follow up of this review was posted here: Text based game “Hunt the Wumpus” Version 3

I tried to incorporate the suggestions made in the first question into the code. However I still feel that there is maybe space for improvement.

I completely changed how the dungeon is made. Now I just build the dodecahedron and fill it with random room numbers (like the original game).

Please let me know how to make the code cleaner / better structured.

In the raw string of the text the page highlights the code wrong. this seems to be a bug of the codereview page.

wumpus.h

#pragma once

#include <array> 
#include <iostream>
#include <random>
#include <string>
#include <sstream>
#include <vector>

namespace wumpus {

    using Room_number = int;

    struct Room {
        std::array <Room*, 3> neighbors{ nullptr }; //pointer to 3 rooms next to this room
        Room_number room_number{ 0 };
        bool has_wumpus{ false };
        bool has_pit{ false };
        bool has_bat{ false };
        bool has_player{ false };
    };

    class Dungeon {
    public:
        Dungeon();
        void indicate_hazards();
        bool shoot_arrow(std::vector<int> tar_rooms);
        bool Dungeon::move_wumpus();
        bool move_player(Room_number target_room);
        void debug();                   //shows the status of the cave for debug purpose
        Room_number get_player_room_number() const { return player_room_number; }

        std::vector<int> get_neighbour_rooms() const
        {
            return std::vector<int>{
                rooms[player_room_number - 1].neighbors[0]->room_number,
                rooms[player_room_number - 1].neighbors[1]->room_number,
                rooms[player_room_number - 1].neighbors[2]->room_number
            };
        }

    private:
        static constexpr int count_of_pits = 3;
        static constexpr int count_of_bats = 3;

        int arrows = 5;

        std::array<Room, 20> rooms;

        Room_number wumpus_room_number;
        Room_number player_room_number;

        void connect_rooms_to_dodecahedron();
    };

    int get_random(int min, int max);
    void hunt_the_wumpus();
    void instructions();
    int select_room_to_move(Dungeon& d1);
    std::vector<int> select_rooms_to_shoot();

}

wumpus.cpp

#include "wumpus.h"

namespace wumpus {

    Dungeon::Dungeon()
    {
        // construct the Dungeon
        connect_rooms_to_dodecahedron();

        // create room numbers
        std::vector<Room_number> random_room_numbers;

        for (size_t i = 0; i < rooms.size(); ++i) {
            random_room_numbers.push_back(i + 1);
        }

        //generate random numbers t0 use to put room numbers random
        std::random_shuffle(random_room_numbers.begin(), random_room_numbers.end());

        // add room numbers randomly
        for (size_t i = 0; i < rooms.size(), i < random_room_numbers.size(); ++i) {
            rooms[i].room_number = random_room_numbers[i];
        }

        //add the wumpus
        rooms[get_random(1, rooms.size())-1].has_wumpus = true;

        //add pit
        for (int i = 0; i < count_of_pits; ++i) {
            int index = get_random(1, rooms.size()) - 1;

            while (rooms[index].has_wumpus || rooms[index].has_pit) {
                index = get_random(1, rooms.size());
            }
            rooms[index].has_pit = true;
        }

        //add bat
        for (int i = 0; i < count_of_bats; ++i) {
            int index = get_random(1, rooms.size())-1;

            while (rooms[index].has_wumpus || rooms[index].has_pit || rooms[index].has_bat) {
                index = get_random(1, rooms.size());
            }
            rooms[index].has_bat = true;
        }

        {
            //add player
            player_room_number = get_random(1, rooms.size());
            int index = player_room_number - 1;

            while (rooms[index].has_wumpus || rooms[index].has_pit || rooms[index].has_bat) {
                player_room_number = get_random(1, rooms.size());
            }
            rooms[index].has_player = true;
        }
    }

    void Dungeon::indicate_hazards()
    {
        bool is_first_bat = true;
        bool is_first_pit = true;

        int index = player_room_number - 1;

        for (auto& x : rooms[index].neighbors) {
            if (x->has_wumpus) { 
                std::cout << "I smell the wumpus\n";
            }
            if (is_first_pit && x->has_pit) {
                is_first_pit = false;
                std::cout << "I feel a breeze\n";
            }
            if (is_first_bat && x->has_bat) {
                is_first_bat = false;
                std::cout << "I hear a bat\n";
            }
        }

        std::cout   << "You are in room " << rooms[index].room_number << "\n"
                    << "You have "<<arrows<< " arrow(s) left\n"
                    << "Tunnels lead to rooms " 
                    << rooms[index].neighbors[0]->room_number << ", "
                    << rooms[index].neighbors[1]->room_number << " and "
                    << rooms[index].neighbors[2]->room_number << "\n"
                    << "what do you want to do? (M)ove or (S)hoot?\n";
    }

    bool Dungeon::shoot_arrow(std::vector<int> target_rooms)
        //trys to shoot in the supplied tar rooms an arrow
        //if the wumpus is hit returns true to indicate victory
        //moves the wumpus on fail
    {
        --arrows;
        int index = player_room_number - 1;

        for (const auto& target : target_rooms){

            bool room_reached = false;

            for (const auto& neigbour : rooms[index].neighbors) {       

                if (neigbour->room_number == target) {
                    room_reached = true;

                    index = neigbour->room_number - 1;

                    if (rooms[index].has_wumpus) {      
                        std::cout << "!!!!!!YOU WON!!!!!!: You killed the Wumpus in room " << rooms[index].room_number << "\n";
                        return true;
                    }
                    break;
                }
            }
            if (!room_reached) {    
                std::cout << "Room " << target << " could not be reached from arrow\n";
                return false;
            }           
        }
        if (arrows == 0) {
            std::cout << "You lost: You ran out of arrows";
            return true;
        }
    }

    bool Dungeon::move_wumpus() 
        //moves the wumpus with a chance of 75% to a new room
        //if player room is entered true is returned for game over
    {
        if (get_random(1, 4) == 4) {    // no movement on 4
            return false;
        }
        else {

            rooms[wumpus_room_number - 1].has_wumpus = false;

            wumpus_room_number = rooms[wumpus_room_number - 1].neighbors[get_random(0, 2)]->room_number;

            rooms[wumpus_room_number - 1].has_wumpus = true;

            if (rooms[wumpus_room_number - 1].has_player) {
                std::cout << "You lost: Wumpus enters youre room and eats you\n";
                return true;
            }
        }
        return false;
    }

    bool Dungeon::move_player(Room_number target_room)
        //trys to move player to the selected room
        //if deadly hazard like pit or wumpus is found return game over = true;
        //if bat is found choose new random room free from hazards to put the player
    {

        for (auto& x : rooms[player_room_number - 1].neighbors) {

            if (x->room_number == target) {
                if (x->has_wumpus) {
                    std::cout << "You lost: You got eaten by the Wumpus\n";
                    return true;
                }
                else if (x->has_pit) {
                    std::cout << "You lost: You fell in a bottomless pit\n";
                    return true;
                }
                else if (x->has_bat) {
                    std::cout << "Gigantic bat appeared!!!\n";
                    std::cout << "You got dragged to a new room\n";

                    int index = get_random(1, rooms.size()) - 1;

                    //Only put player in empty room
                    while (rooms[index].has_wumpus || rooms[index].has_pit || rooms[index].has_bat || rooms[index].has_player) {
                        index = get_random(1, rooms.size()) - 1;
                    }
                    rooms[player_room_number - 1].has_player = false;

                    player_room_number = index + 1;

                    rooms[index].has_player = true;
                    return false;
                }
                else {
                    rooms[player_room_number - 1].has_player = false;
                    player_room_number = target;
                    rooms[player_room_number - 1].has_player = true;
                    return false;
                }
            }
        }
        std::cerr << "Dungeon::move_player: Unknown target room entered";
        return false;
    }

    void Dungeon::debug()
    {
        for (const auto&x : rooms) {
            std::cout << "Room " << x.room_number << " connects to: ";

            for (const auto&y : x.neighbors) {
                if (y != nullptr) 
                    std::cout << y->room_number << " ";
                else std::cout << "np" << " ";
            }

            std::cout << " ";
            if (x.has_wumpus) {
                std::cout << "wumpus:" << x.has_wumpus << " ";
            }
            if (x.has_pit) {
                std::cout << "pit:" << x.has_pit << " ";
            }
            if (x.has_bat) {
                std::cout << "bat:" << x.has_bat << " ";
            }
            if (x.has_player) {
                std::cout << "player:" << x.has_player << " ";
            }
            std::cout << "\n";
        }
    }

    //-------------------------------------------------------------
    //Private functions
    //-------------------------------------------------------------


    void Dungeon::connect_rooms_to_dodecahedron()
    {
        rooms[0].neighbors = { &rooms[1] ,&rooms[4], &rooms[19] };
        rooms[1].neighbors = { &rooms[0] ,&rooms[2], &rooms[17] };
        rooms[2].neighbors = { &rooms[1] ,&rooms[3], &rooms[15] };
        rooms[3].neighbors = { &rooms[2] ,&rooms[4], &rooms[13] };
        rooms[4].neighbors = { &rooms[0] ,&rooms[3], &rooms[5] };
        rooms[5].neighbors = { &rooms[4] ,&rooms[6], &rooms[12] };
        rooms[6].neighbors = { &rooms[5] ,&rooms[7], &rooms[19] };
        rooms[7].neighbors = { &rooms[6] ,&rooms[8], &rooms[11] };
        rooms[8].neighbors = { &rooms[7] ,&rooms[9], &rooms[18] };
        rooms[9].neighbors = { &rooms[8] ,&rooms[10], &rooms[16] };
        rooms[10].neighbors = { &rooms[9] ,&rooms[11], &rooms[14] };
        rooms[11].neighbors = { &rooms[7] ,&rooms[10], &rooms[12] };
        rooms[12].neighbors = { &rooms[5] ,&rooms[11], &rooms[13] };
        rooms[13].neighbors = { &rooms[3] ,&rooms[12], &rooms[14] };
        rooms[14].neighbors = { &rooms[10] ,&rooms[13], &rooms[15] };
        rooms[15].neighbors = { &rooms[2] ,&rooms[14], &rooms[16] };
        rooms[16].neighbors = { &rooms[9] ,&rooms[15], &rooms[17] };
        rooms[17].neighbors = { &rooms[1] ,&rooms[16], &rooms[18] };
        rooms[18].neighbors = { &rooms[8] ,&rooms[17], &rooms[19] };
        rooms[19].neighbors = { &rooms[0] ,&rooms[6], &rooms[18] };
    }

    //-------------------------------------------------------------
    //Helper functions
    //-------------------------------------------------------------

    int get_random(int min, int max)
    {
        static std::random_device rd;
        static std::mt19937 mt(rd());
        std::uniform_int_distribution<int> distribution(min, max);
        return distribution(mt);
    }

    void hunt_the_wumpus()
    {
        instructions();

        for (;;)        // restart game
        {
            Dungeon d1;
            for (;;) {      // current room handle

                d1.indicate_hazards();

                std::string in;
                std::cin >> in;
                if (std::cin.fail()) {
                    std::cin.clear();
                    std::cin.ignore(999, '\n');
                    continue;
                }

                bool game_over = false;

                if (in == "m" || in == "M" || in == "Move" || in == "move") {
                    game_over = d1.move_player(select_room_to_move(d1));
                }
                else if (in == "s" || in == "S" || in == "Shoot" || in == "shoot") {

                    game_over = d1.shoot_arrow(select_rooms_to_shoot());

                    if (game_over == true) { 
                        break; 
                    }
                    game_over = d1.move_wumpus();
                }
                if (game_over == true) {
                    break;
                }
            }

            std::cout << "Press any key to start a new game or (q)uit to end game\n";
            std::string in;
            std::cin >> in;

            if (in == "q" || in == "Q" || in == "Quit" || in == "quit")
                break;
        }
    }

    void instructions()
    {
        std::cout <<R"(Welcome to "Hunt the Wumpus"!
The wumpus lives in a cave of rooms.Each room has 3 tunnels leading to
other rooms. (Look at a dodecahedron to see how this works - if you don't know
what a dodecahedron is, ask someone).

Hazards
Bottomless pits - two rooms have bottomless pits in them.If you go there, you
fall into the pit(and lose!)
Super bats - two other rooms have super bats.If you go there, a bat grabs you
and takes you to some other room at random. (Which may be troublesome).

Wumpus
The wumpus is not bothered by hazards(he has sucker feet and is too big for a
bat to lift).Usually he is asleep.Two things wake him up : you shooting an
arrow or you entering his room.\n"

If the wumpus wakes he moves(p = .75) one room or stays still(p = .25).After
that, if he is where you are, he eats you up and you lose!\n"

Each turn you may move or shoot a crooked arrow.
Moving: you can move one room(thru one tunnel).
Arrows : you have 5 arrows.You lose when you run out.Each arrow can go from 1
to 3 rooms.You aim by telling the computer the rooms you want the arrow to go
to.If the arrow can\'t go that way (if no tunnel) it moves at random to the
next room.If the arrow hits the wumpus, you win.If the arrow hits you, you lose.

Warnings
When you are one room away from a wumpus or hazard, the computer says :

Wumpus: "I smell the wumpus"
Bat : "I hear a bat"
Pit : "I feel a breeze"


"Press any key to start")";

        char c;
        std::cin.get(c);
    }

    int select_room_to_move(Dungeon& d1)
    {
        for(;;) {

            std::cout << "To where??\n";

            int target = 0;
            std::cin >> target;

            if (std::cin.fail()) {
                std::cin.clear();
                std::cin.ignore(999, '\n');
                continue;
            }

            std::vector<int> neighbor = d1.get_neighbour_rooms();

            if (target == neighbor[0] || target == neighbor[1] || target == neighbor[2])
                return target;
        }
    }


    std::vector<int> select_rooms_to_shoot()
    {
        for(;;){
            std::cout << "Enter the rooms you want to shoot the arrow (e.g. 2-3-12, e.g. 4-5, e.g. 2)\n";

            std::string input;
            std::cin >> input;

            std::istringstream ist{ input };

            std::vector<int> target_rooms;

            bool bad_input = false;

            while (!ist.eof()) {

                int room_number;
                ist >> room_number;

                if (ist.fail()) {
                    bad_input = true;
                    break;
                }

                target_rooms.push_back(room_number);

                if (target_rooms.size() == 3 || ist.eof())
                    break;

                char seperator;
                ist >> seperator;

                if (ist.fail()) {
                    bad_input = true;
                    break;
                }
                if ((seperator != '-')   || (target_rooms.size() > 3)) {
                    bad_input = true; 
                    break; 
                }
            }

            if (bad_input) {
                continue;
            }
            else {
                return target_rooms;
            }
        }
    }
}

main.cpp

#include <iostream>
#include "wumpus.h"


int main()
try {
    wumpus::hunt_the_wumpus();
}
catch (std::runtime_error& e) {
    std::cerr << e.what() << "\n";
    std::cin.get();
}
catch (...) {
    std::cerr << "unknown error\n";
    std::cin.get();
}

Answer 1

I see some things that may help you improve your code.

Fix the bug (#1)

The Dungeon::move_player takes target_room as a parameter, but then refers to target within the code for the function, so it doesn't compile at the moment. I assume you meant target_room in both places.

Fix the bug (#2)

The code to add the wumpus currently looks like this:

//add the wumpus
rooms[get_random(1, rooms.size())-1].has_wumpus = true;

No problem except that it fails to initialize wumpus_room_number, so the first time move_wumpus is called, it's referring to an uninitialized value. Not good! Instead, initialize the wumpus like this:

//add the wumpus
wumpus_room_number = get_random(1, rooms.size())-1;
rooms[wumpus_room_number].has_wumpus = true;

Fix the bug (#3)

The reporting of room numbers is inconsistent. For example, here's part of the play of the game:

You are in room 2
You have 3 arrow(s) left
Tunnels lead to rooms 5, 16 and 6
what do you want to do? (M)ove or (S)hoot?
M
To where??
5
You are in room 17
You have 3 arrow(s) left
Tunnels lead to rooms 8, 14 and 5

So I moved to room 5 and arrived in room 17? Also, I was in room 2 and now there's no path back to that room? That's a bug!

Fix the bug (#4)

You have a number of classic off-by-one errors in the code. An example is this:

//add bat
for (int i = 0; i < count_of_bats; ++i) {
    int index = get_random(1, rooms.size())-1;
    while (rooms[index].has_wumpus || rooms[index].has_pit || rooms[index].has_bat) {
        index = get_random(1, rooms.size());
    }
    rooms[index].has_bat = true;
}

The first instance of the call to get_random correctly subtracts 1 but subsequent calls do not. I'd rewrite it like this:

//add bat
for (auto i{count_of_bats}; i;  ) {
    int index = get_random(0, rooms.size()-1);
    if (!rooms[index].has_wumpus && !rooms[index].has_pit && !rooms[index].has_bat) {
        rooms[index].has_bat = true;
        --i;
    }
}

Omit the class name from member function declarations

It was probably just a typo or cut-and-paste error, but within the Dungeon class in wumpus.h we have this:

bool Dungeon::move_wumpus();

As you know (because this is the only place it's done otherwise) the extra qualification is not necessary here. Instead, that should be:

bool move_wumpus();

Use all required #includes

The wumpus.cpp file contains calls to std::random_shuffle so it should #include <algorithm>. Further, this should be placed in wumpus.cpp and not wumpus.h as per the next suggestion.

Separate interface from implementation

The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. That's mostly done in this code but remember that the #include are actually part of the interface. Thus, only the #includes that are required to understand the interface should be in the wumpus.h file and all #includes that are specific to the implementation should be isolated in the wumpus.cpp file. In this case, that means only <array> and <vector> should be in the .h file. Everything else should go into the .cpp file.

Always return an appropriate value

Your shoot_array() routine has control paths that cause it to end without returning any bool value. This is an error and should be fixed, probably most easily by simply adding return false; to the end.

Prefer std::array to std::vector when fixed size is known at compile time

The way this game is constructed, there will always be 20 room, each with 3 neighbors. For that reason, the get_neighbor_room should probably return a std::array<Room_number, 3> instead of a std::vector<int>.

Move construction from runtime to compile time where practical

There is no need for the dodecahedron to be reconstructed at runtime each game. It's a fixed structure with fixed connections. All that the game really needs is to randomize the assigned room_number for each game. So for that reason, I'd statically construct the rooms array like this:

    std::array<Room, 20> rooms{{
        { &rooms[1] ,&rooms[4], &rooms[19] },
        { &rooms[0] ,&rooms[2], &rooms[17] },
        { &rooms[1] ,&rooms[3], &rooms[15] },
        // and all the other rooms
    }};

Use standard library algorithms

The room number creation and constructor randomization could be considerably improved. Currently the code looks like this:

// create room numbers
std::vector<Room_number> random_room_numbers;

for (size_t i = 0; i < rooms.size(); ++i) {
    random_room_numbers.push_back(i + 1);
}

//generate random numbers t0 use to put room numbers random
std::random_shuffle(random_room_numbers.begin(), random_room_numbers.end());

// add room numbers randomly
for (size_t i = 0; i < rooms.size(), i < random_room_numbers.size(); ++i) {
    rooms[i].room_number = random_room_numbers[i];
}

Then the wumpus, pits, bats and player are placed in the constructor which is more than 50 lines. There's a better way to do this. First shuffle the rooms collection, then add each feature sequentially since the rooms are already randomized. Then shuffle again. This makes things really simple:

Dungeon::Dungeon()
{
    std::random_device rd;
    std::mt19937 g(rd());
    std::shuffle(rooms.begin(), rooms.end(), g);
    std::size_t i{0};
    rooms[i++].has_player = true;
    rooms[i++].has_wumpus = true;
    for (auto pits{count_of_pits}; pits; --pits) {
        rooms[i++].has_pit = true;
    }
    for (auto bats{count_of_bats}; bats; --bats) {
        rooms[i++].has_bat = true;
    }
    std::shuffle(rooms.begin(), rooms.end(), g);
}

If you adopt this method, there's no longer any need to have a separate room_number field, since you could just use the index. That would eliminate the category of bugs such as caused bug #3 above. It also eliminates the wumpus_room_number member variable. If we need to find the wumpus (e.g. in the move_wumpus member function) we can just do it using std::find_if like this:

bool Dungeon::move_wumpus() {
    auto direction = get_random(0, 3);
    if (direction == 3) {  // 25% chance that wumpus won't move
        return false;
    }
    // find the wumpus
    auto wumpus_room{std::find_if(rooms.begin(), rooms.end(), [](const Room &r){ return r.has_wumpus; })};
    // move him
    wumpus_room->has_wumpus = false;
    auto new_room = wumpus_room->neighbors[direction];
    new_room->has_wumpus = true;
    if (new_room->has_player) {
        std::cout << "You lost: Wumpus enters your room and eats you\n";
        return true;
    }
    return false;
}

Look out for member functions

There is a function in the current code with this prototype:

int select_room_to_move(Dungeon& d1);

Since it's being passed a non-const reference to a Dungeon anyway, it seems likely that this should instead be a member function.

Fix the text string

It looks like you may have started out with a plain string and then changed it to a literal string. Unfortunately, now that it's literal, things like \n get printed just as they appear, so you might want to clean things up.