3
\$\begingroup\$

I am a hobbyist programmer trying to learn modern C++. My son had an assignment in his C++ class to fill in some functions that populate draw the cave in the "hunt for the wumpus" game. When I viewed the instructors skeleton code I found it to be very C-like rather than C++-like. So, I decided to rewrite it so I could show it to my son. Since I don't want to show my son bad code, I thought I would bring it here first. So, the big question is did I do a good job of producing creditable C++ code?

Secondarily, when I checked the code against the core guidelines (Visual C++ 2019 NuGet Microsoft.GSL v0.1.2.1) I received a number of lifetime.3 warnings (shown by gsl::suppress). I cannot figure out how to rewrite the code not to get these warnings, and any help in getting a conceptual understanding of these warnings would be appreciated.

wompus.cpp

#include <iostream>
#include <map>
#include <vector>
#include <array>
#include <set>
#include <string>
#include <string_view>
#include "dhth.hpp"

using std::vector;
using std::array;
using std::set;
using std::map;
using std::string;
using std::cout;
using std::endl;

using grid_point = array<int, 2>;

grid_point Get_A_Random_Grid_Point(int rows, int cols) {
    dhth::Rand rand;
    return grid_point{ rand(1, rows), rand(1, cols) };
}

enum class Cave_Item : int8_t {
    breeze,
    stench,
    pit,
    ladder,
    player,
    wumpus,
    gold
};

string citos(Cave_Item item) noexcept {  // citos - Cave_Item to std::string
    try {
        switch (item) {
        case Cave_Item::breeze: return string{ "breeze" };
        case Cave_Item::stench: return string{ "stench" };
        case Cave_Item::pit: return string{ "pit" };
        case Cave_Item::ladder: return string{ "ladder" };
        case Cave_Item::player: return string{ "player" };
        case Cave_Item::wumpus: return string{ "wumpus" };
        case Cave_Item::gold: return string{ "gold" };
        default:
            std::abort();
        }
        return string{ "error" };
    }
    catch (...)
    {
        try { 
            cout << "Failed to make string\n"; 
            return string{ "error" };
        }
        catch (...) {
            std::abort();
        }
    }
}

class Cave
{
public:
    Cave() noexcept;
    Cave(int rows, int cols) noexcept;
    void Add_Item(grid_point loc, Cave_Item cave_item);
    void Print_Square(grid_point loc);
    void Print_Cave_Diagram();
    bool Remove_Item(Cave_Item cave_item);
    bool Remove_Item(grid_point loc, Cave_Item cave_item);

private:
    void Fill_Adjacent(grid_point loc, Cave_Item sensation);
    void Get_Cave_Diagram(vector<string>& cave_diagram);
    void init();
    bool Is_Square_Valid(grid_point indx) noexcept;
    void Remove_Adjacent(grid_point loc, Cave_Item sensation);

    int rows_ = 4;
    int cols_ = 4;
    map<grid_point, set<Cave_Item>> cave;
};

Cave::Cave() noexcept
{
    try {
        init();
    }
    catch (...) {
        try {
            cout << "Failed to initialize\n";
        }
        catch (...) {
            std::abort();
        }
    }
}

Cave::Cave(int rows, int cols) noexcept : rows_{ rows }, cols_{ cols } 
{
    try {
        init();
    }
    catch (...) {
        try {
            cout << "Failed to initialize\n";
        }
        catch (...) {
            std::abort();
        }
    }
}

void Cave::Add_Item(grid_point loc, Cave_Item cave_item)
{
    [[gsl::suppress(26486)]]       // Don't pass a pointer that may be invalid 
    cave[loc].insert(cave_item);   // Parameter 0'@(*this).cave
    if (cave_item == Cave_Item::wumpus) Fill_Adjacent(loc, Cave_Item::stench);
    if (cave_item == Cave_Item::pit) Fill_Adjacent(loc, Cave_Item::breeze);
}

void Cave::Print_Square(grid_point loc)
[[gsl::suppress(26486)]]       // Don't pass a pointer that may be invalid 
{                              // Parameter 0'@(*this).cave
    cout << "This part of the cave (" << loc.at(0) << ", " << loc.at(1) <<
        ") contains:\n";
    if (cave[loc].empty()) {
        cout << "    nothing\n\n";
        return;
    }
    for (auto item : cave[loc]) cout << "    " << citos(item) << "\n";
    cout << "\n";
}

void Cave::Print_Cave_Diagram() {
    vector<string> cave_diagram;
    cave_diagram.reserve(5 * rows_ + 1);

    Get_Cave_Diagram(cave_diagram);
    for (auto s : cave_diagram)
        cout << s << endl;
}

bool Cave::Remove_Item(Cave_Item cave_item)
{
    bool ret = false;
    for (auto& items : cave) {
        auto itr = items.second.find(cave_item);
        if (itr != items.second.end()) {
            ret = true;
            items.second.erase(itr);
            if (cave_item == Cave_Item::wumpus)
                Remove_Adjacent(items.first, Cave_Item::stench);
            if (cave_item == Cave_Item::pit)
                Remove_Adjacent(items.first, Cave_Item::breeze);
        }
    }
    return ret;
}

[[gsl::suppress(26486)]]       // Don't pass a pointer that may be invalid 
                               // Parameter 0'@(*this).cave
bool Cave::Remove_Item(grid_point loc, Cave_Item cave_item)
{
    const auto num = cave[loc].erase(cave_item);
    if (num == 0) return false;
    if (cave_item == Cave_Item::wumpus)
        Remove_Adjacent(loc, Cave_Item::stench);
    if (cave_item == Cave_Item::pit)
        Remove_Adjacent(loc, Cave_Item::breeze);
    return true;
}

[[gsl::suppress(26486)]]       // Don't pass a pointer that may be invalid 
                               // Parameter 0'@(*this).cave
void Cave::Fill_Adjacent(grid_point loc, Cave_Item sensation)
{
    for (auto& coord : loc) {
        coord--;
        if (Is_Square_Valid(loc)) cave[loc].insert(sensation);
        coord += 2;
        if (Is_Square_Valid(loc)) cave[loc].insert(sensation);
        coord--;
    }
}

void Cave::Get_Cave_Diagram(vector<string>& cave_diagram)
{
    constexpr int cell_rows = 5;
    constexpr int cell_columns = 11;
    const int total_rows = cell_rows * rows_ + 1;
    const int total_columns = cell_columns * cols_ + 1;

    // fill in with vertical cell divisions
    for (int r = 0; r < total_rows; r++) {
        string row(total_columns, ' ');
        for (int c = 0; c < total_columns; c += cell_columns) {
            row.at(c) = '|';
        }
        cave_diagram.push_back(row);
    }

    // udpate horizontal rows with '-'
    for (int i = 0; i < total_rows; i += cell_rows) {
        cave_diagram.at(i) = string(total_columns, '-');
    }
    // update cell corners with '+'
    for (int r = 0; r < total_rows; r += cell_rows) {
        for (int c = 0; c < total_columns; c += cell_columns) {
            cave_diagram.at(r).at(c) = '+';
        }
    }

    // replace the part of the string with the cell contents
    for (auto items : cave) 
[[gsl::suppress(26486)]]       // Don't pass a pointer that may be invalid 
    {                          // Parameter 0'@(*this).cave
        grid_point loc{ items.first };
        const int& row = loc.at(0);
        const int& col = loc.at(1);
        if (cave[loc].empty()) continue;
        int r = (row - 1) * cell_rows + 2;
        const int c = (col - 1) * cell_columns + 3;
        if (cave[loc].size() == 4) --r;
        for (auto item : cave[loc]) {
            cave_diagram.at(r).replace(c, citos(item).size(), citos(item));
            ++r;
        }
    }
}

void Cave::init()
[[gsl::suppress(26486)]]   // Don't pass a pointer that may be invalid 
{                          // Parameter 0'@(*this).cave
    grid_point loc{ 1, 1 }; // row column
    cave[loc].insert(Cave_Item::ladder);
    cave[loc].insert(Cave_Item::player);
    while (cave[loc].find(Cave_Item::ladder) != cave[loc].cend())
        loc = Get_A_Random_Grid_Point(rows_, cols_);
    Add_Item(loc, Cave_Item::wumpus);
    for (auto i = 0; i < 3; ++i) {
        while (!(cave[loc]).empty()) loc = Get_A_Random_Grid_Point(rows_, cols_);
        Add_Item(loc, Cave_Item::pit);
    }
    while (cave[loc].find(Cave_Item::ladder) != cave[loc].cend() ||
        cave[loc].find(Cave_Item::pit) != cave[loc].cend()) 
        loc = Get_A_Random_Grid_Point(rows_, cols_);
    cave[loc].insert(Cave_Item::gold);
}

bool Cave::Is_Square_Valid(const grid_point loc) noexcept {
    const int& row = loc.at(0);
    const int& col = loc.at(1);
    if (row < 1 || row > rows_) return false;
    if (col < 1 || col > cols_) return false;
    return true;
}

void Cave::Remove_Adjacent(grid_point loc, Cave_Item sensation)
{
    for (auto& coord : loc) {
        coord--;
        if (Is_Square_Valid(loc)) {
            const bool first = Remove_Item(loc, sensation);
            if (!first) {
                cout << "Disaster first remove adjacent\n";
                std::abort();
            }
        }
        coord += 2;
        if (Is_Square_Valid(loc)) {
            const bool first = Remove_Item(loc, sensation);
            if (!first) {
                cout << "Disaster second remove adjacent\n";
                std::abort();
            }
        }
        coord--;
    }
}


int main()
{
    constexpr int rows = 4;
    constexpr int cols = 4;
    Cave cave(rows, cols);

    for (int r = 1; r <= rows; r++) {
        for (int c = 1; c <= cols; c++) {
            const grid_point loc{ r,c };
            cave.Print_Square(loc);
        }
    }

    cave.Print_Cave_Diagram();

    cave.Remove_Item(Cave_Item::wumpus);
    cout << "\n";
    cave.Print_Cave_Diagram();
    cave.Add_Item({ 1,1 }, Cave_Item::wumpus);
    cout << "\n";
    cave.Print_Cave_Diagram();

    return 0;
}

Additionally, I was already working on trying to encapsulate random number generation, so I wrote my first impl/pimpl implementation of a Rand class. During this, the compiler complained that I needed to include the other constructors (again seen as gsl::suppress), and when I did the program fail to work. Interestingly, one of the gsl suggestions to make a variable const since it was used only once proved to be fatal when I used the compiler explorer http://godbolt.org I discovered that my changes created errors in gcc. So, how can I improve this code?

dhth.hpp

#ifndef DHTH_HPP
#define DHTH_HPP
#pragma once
#include <random>
#include <memory>
#include <iostream>

namespace dhth {
    class Rand {
    public:
        Rand() noexcept;

[[gsl::suppress(26432)]]
        ~Rand();
//      Rand(const Rand& rand) = default;
//      Rand(Rand&& rand) = default;
        double operator()(void);
        int operator()(int limit);
        int operator()(int lower, int upper);
        double operator()(double lower, double upper);
    private:
        class impl;
        std::unique_ptr<impl> upimpl;
    };
}
#endif

dhth.cpp

#include "dhth.hpp"

namespace dhth {
    class Rand::impl {
    public:
        impl() noexcept {
            try { e1.seed(rd()); }
            catch (const std::exception e)
            {
                try {
                    std::cout << "Failed to seed\n";
                }
                catch (...) {
                    std::abort();
                }
            }
        }

[[gsl::suppress(26432)]]
        ~impl() {}
//      impl(const impl& imp) = default;
//      impl(impl&& imp) = default;
        int func(int lower, int upper);
        double func(double lower, double upper);
    private:
        std::random_device rd;
        std::default_random_engine e1;
    };

#pragma warning(push)
#pragma warning(disable:26447)   // noexcept violation
    Rand::Rand() noexcept : upimpl{ std::make_unique<impl>() } {}
#pragma warning(pop)
    Rand::~Rand() = default;
    double Rand::operator()(void) { 
        return upimpl->func(0.0, 1.0); 
    }
    int Rand::operator()(int limit)
    {
        return upimpl->func(0,limit-1);
    }
    int Rand::operator()(int lower, int upper)
    {
        return upimpl->func(lower, upper);
    }
    double Rand::operator()(double lower, double upper)
    {
        return upimpl->func(lower, upper);
    }

[[gsl::suppress(26496)]]  // dist used once make const
    int Rand::impl::func(int lower, int upper)
    {
        std::uniform_int_distribution<int> dist(lower, upper);
        return dist(e1);
    }

[[gsl::suppress(26496)]]  // dist used once make const
    double Rand::impl::func(double lower, double upper)
    {
        std::uniform_real_distribution<double> dist(lower, upper);
        return dist(e1);
    }
}

This is the output of the entire program. I make remove the wumpus and then add it back to location 1,1.

This part of the cave (1, 1) contains:
    ladder
    player

This part of the cave (1, 2) contains:
    nothing

This part of the cave (1, 3) contains:
    breeze

This part of the cave (1, 4) contains:
    pit

This part of the cave (2, 1) contains:
    stench
    gold

This part of the cave (2, 2) contains:
    breeze

This part of the cave (2, 3) contains:
    pit

This part of the cave (2, 4) contains:
    breeze

This part of the cave (3, 1) contains:
    wumpus

This part of the cave (3, 2) contains:
    breeze
    stench

This part of the cave (3, 3) contains:
    breeze

This part of the cave (3, 4) contains:
    nothing

This part of the cave (4, 1) contains:
    breeze
    stench

This part of the cave (4, 2) contains:
    pit

This part of the cave (4, 3) contains:
    breeze

This part of the cave (4, 4) contains:
    nothing

+----------+----------+----------+----------+
|          |          |          |          |
|  ladder  |          |  breeze  |  pit     |
|  player  |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|  stench  |  breeze  |  pit     |  breeze  |
|  gold    |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|  wumpus  |  breeze  |  breeze  |          |
|          |  stench  |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|  breeze  |  pit     |  breeze  |          |
|  stench  |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+

+----------+----------+----------+----------+
|          |          |          |          |
|  ladder  |          |  breeze  |  pit     |
|  player  |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|  gold    |  breeze  |  pit     |  breeze  |
|          |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|          |  breeze  |  breeze  |          |
|          |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|  breeze  |  pit     |  breeze  |          |
|          |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+

+----------+----------+----------+----------+
|          |          |          |          |
|  ladder  |  stench  |  breeze  |  pit     |
|  player  |          |          |          |
|  wumpus  |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|  stench  |  breeze  |  pit     |  breeze  |
|  gold    |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|          |  breeze  |  breeze  |          |
|          |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
|          |          |          |          |
|  breeze  |  pit     |  breeze  |          |
|          |          |          |          |
|          |          |          |          |
+----------+----------+----------+----------+
\$\endgroup\$
  • 2
    \$\begingroup\$ "I cannot figure out how to rewrite the code not to get these errors" As you should know already: SE Code Review is only for reviewing already working code, and not about fixing errors. \$\endgroup\$ – πάντα ῥεῖ Nov 24 '19 at 16:47
  • 1
    \$\begingroup\$ The code as written works! (as demonstrated by the output) Probably should have said "warnings." When I tried to add the commented code it didn't work. I'm trying to understand the impl/pimpl idiom. \$\endgroup\$ – davidbear Nov 24 '19 at 17:11
  • 2
    \$\begingroup\$ Questions asking what the code does are also off-topic, so we can't help with the impl/pimpl idiom. \$\endgroup\$ – pacmaninbw Nov 24 '19 at 17:20
  • 1
    \$\begingroup\$ I've edited to reword "error" -> "warning", but I'll leave it up to the community to vote to keep it open or have it closed, since it's unclear to me whether the last part is about code that produces compiler errors... which would be off-topic. \$\endgroup\$ – Mathieu Guindon Nov 25 '19 at 22:04
  • 1
    \$\begingroup\$ Honestly this looks totally on topic. The code runs, gives the expected output. I suspect you got downvoted/VTCd because there was the word "error" in your title. This looks on-topic to me. \$\endgroup\$ – IEatBagels Nov 26 '19 at 2:03
6
\$\begingroup\$

Include Order

#include <iostream>
#include <map>
#include <vector>
#include <array>
#include <set>
#include <string>
#include <string_view>
#include "dhth.hpp"

I always put the most significant at the top and then work down to the least significant. The reason for this is that if I miss a required header file in the most significant include then I will spot it during compilation.

Here you put the most significant class last "dhth.hpp". This is the most important header to "dhth.cpp" so it should go first. At the moment if this header file requires something like vector (lets just imagine) to work correctly that fact is hidden from you because you have included this in the source file first. If you look at from another developer that is building "TheLandOfWump.cpp" and he #includes your header file "dhth.hpp" but finds it does not compile because you forgot to include vector he is going to be pissed that you have not included the required headers in your file (why does he need to include vector in his source file if he is not using it.

So load headers from most significant to least.

 #include "HeaderForThisSource.h"

 #include "OtherHeadersForThisProject.h"

 #include <Headers For libraries you use>

 #include <Standard C++ Libraries>

 #include <C Libraries>

Within each group some thinks it is worth sorting them (alphabetically). Personally I don't but I do group them with each block. For the C++ headers I will list all the stream stuff together all the iterator stuff together and all the algorithm stuff together.

Though I don't use alphabetical sorting I do trying and making the grouping logical.


Not quite is bad as using namespace std; but close.

using std::vector;
using std::array;
using std::set;
using std::map;
using std::string;
using std::cout;
using std::endl;

Is it really that hard to prefix standard types with std::?


Two points:

using grid_point = array<int, 2>;

Not sure an array is the best type?

std::pair?  std::tuple? struct Point {int x,y};

Random. You are doing it wrong.

grid_point Get_A_Random_Grid_Point(int rows, int cols) {
    dhth::Rand rand;
    return grid_point{ rand(1, rows), rand(1, cols) };
}

The random number generator is not supposed to be created everytime you need a random number. You create it once then use it multiple times so you get a sequence of numbers that are random from the same generator.

In this context you can do this by marking it a static member of the function. But I am sure you want to use it other contexts so making it a global object or accessed from a static function on Rand may be a better choice.

grid_point Get_A_Random_Grid_Point(int rows, int cols) {
    static dhth::Rand rand;
    return grid_point{ rand(1, rows), rand(1, cols) };
}

Not sure that catching the exception here is a good thing.

string citos(Cave_Item item) noexcept {  // citos - Cave_Item to std::string
    try {
        switch (item) {
        case Cave_Item::breeze: return string{ "breeze" };
        case Cave_Item::stench: return string{ "stench" };
        case Cave_Item::pit: return string{ "pit" };
        case Cave_Item::ladder: return string{ "ladder" };
        case Cave_Item::player: return string{ "player" };
        case Cave_Item::wumpus: return string{ "wumpus" };
        case Cave_Item::gold: return string{ "gold" };
        default:
            std::abort();
        }
        return string{ "error" };
    }
    catch (...)
    {
        try { 
            cout << "Failed to make string\n"; 
            return string{ "error" };
        }
        catch (...) {
            std::abort();
        }
    }
}

Just let the exception propogate all the way out and close the application. If this function fails then something has gone terribly wrong with the application and trying to continue is a mistake.

In main you can catch all exceptions and report on them.

int main()
{
     try {
         runGame();
     }
     // Catch print error message then re-throw the exception.
     // This way the external systems will know the application
     // failed abnormally.
     //
     // Also use std::cerr for error messages.
     catch(std::exception const& e) {
         std::cerr << "Exception: " << e.what() << "\n";
         throw;
     }
     catch(...) {
         std::cerr << "Exception: Unknown\n";
         throw;
     }
}  

You should only mark a function noexcept if it cant throw an exception.

Cave::Cave() noexcept

Again I don't like your use of std::abort() here. Simply allow the exception to propogate up to main and force the application to exit. That way you make sure all the resources are correctly released via the destructors.

{
    try {
        init();
    }
    catch (...) {
        try {
            cout << "Failed to initialize\n";
        }
        catch (...) {
            std::abort();
        }
    }
}

So 1: remove noexcept 2: Remove the try/catch 3: Catch the exception in main.


Always put braces around sub statements:

    if (cave_item == Cave_Item::wumpus) Fill_Adjacent(loc, Cave_Item::stench);

It looks innocuous enough. But there are situations where this will fail. So it is good habit to always put braces around the statements of an if, even if there looks to only be one.

    if (cave_item == Cave_Item::wumpus) {
        Fill_Adjacent(loc, Cave_Item::stench);
    }

This protects you from some enthusiastic amateur breaking the code by changing Fill_Adjacent() from a method to inline macro. I have seen that happen.

Also using two lines is always nice for when you are debugging. Its hard to tell with the interactive debugger has decided to activate the if statement if it is all on one line. If it is on two lines simply stepping through this immediately shows that the if was activated.


Not sure it is worth using the at() method.

    cout << "This part of the cave (" << loc.at(0) << ", " << loc.at(1) <<
        ") contains:\n";

We know a location is 2 places. So 0 and 1 are always valid. So why are we forcing a check to see if they are valid? I would use loc[0] and loc[1] unverified check because I know the array always has two elements (actually I would probably use a Point class so there is no need to worry and x and y cords have named members that are checked by the compiler at compile time).


Again I would always use braces around the sub expression.

    for (auto item : cave[loc]) cout << "    " << citos(item) << "\n";

And put it on two lines so it is easy to read:

    for (auto item : cave[loc]) {
        cout << "    " << citos(item) << "\n";
    }
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you for your review, I learned a lot. The choice of array was driven in part on the by the desire to use a loop in Fill_Adjacent routine. \$\endgroup\$ – davidbear Nov 24 '19 at 21:33
  • \$\begingroup\$ 1. Why differentiate between standard headers? Also, mention sorting each list? 2. There is something to be said for thread_local for random number generators. 3. Always forcing blocks is a call to flamewar. Anyway, good one. \$\endgroup\$ – Deduplicator Nov 25 '19 at 23:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for?Browse other questions tagged or ask your own question.