Menu class/struct for RPG

Question

I plan to use this menu struct in an RPG played in the terminal. I want to make a RPG similar to D&D. I would like advice on my methods for the menu struct, especially the "hide" and "unhide" option methods.

main.cpp

#include <string>
#include <vector>
#include <cstdlib>
#include <iostream>
#include <initializer_list>

const int invalid_index = -2;
const int default_index = -1;
const std::string defualt_title = "";
const std::initializer_list<std::string> defualt_options = {""};

struct menu{
    menu(const std::string& title, const std::initializer_list<std::string>& options)
        : title(title), options(options), current_index(default_index){;}; 
    menu(void)
        : title(defualt_title), options(defualt_options), current_index(default_index){;};

    std::vector<std::string> hidden_options;
    std::vector<std::string> options;
    std::string title;
    int current_index;

    void insert_option(int index, std::string option){
        if(index >= 0 && index < options.size()){       
            options.insert(options.begin() + index, option);
        }else{;}
    };

    void delete_option(int index){
        if(index >= 0 && index < options.size()){       
            options.erase(options.begin() + index);
        }else{;}
    };

    void obtain_option(int index){
        if(index >= 0 && index < options.size()){
            current_index = index;  
        }else{current_index = invalid_index;}
    };

    void hide_option(int index){
        if(index >= 0 && index < options.size()){
            hidden_options.resize(options.size());
            hidden_options[index] = options[index];
            options.erase(options.begin() + index);
        }else{;}
    };

    void unhide_option(int index){
        if(hidden_options[index] != ""){
            if(index >= 0 && index < options.size()){       
                options.insert(options.begin() + index, hidden_options[index]);
            }else{;}
        }else{;}
    };

    friend std::ostream& operator<<(std::ostream& ostream, const menu& r){
        ostream << r.title << '\n';
        for(int i = 0; i < r.options.size(); i++){
            ostream << "[" << i << "] " << r.options[i] << '\n';
        }ostream << "Enter index: ";
        return ostream; 
    };  
};

int main(){
    menu title_screen("Anyone Know a good RPG?", {"Play","Help","Quit"});
    return 0;   
}

Answer 1

• defualt_title and defualt_options are misspelled.

• Those empty else{;} statements are unnecessary. If there is eventually a need for actual else statements, then they can be added later.

• With those "stray" data members, this should really be a class so that they can be private. Data members should not be directly exposed to the interface. Any functions that shouldn't be called by the user should also be private, otherwise they should be public.

Answer 2

Jamal hit it good. This should be a class with private fields for those stray data members.

Also, instead of:

options.begin() + index

try:

options.at(index)

And, this is personal preference, but:

void unhide_option(int index)
{
        if(hidden_options[index] != "")
        {
            if(index >= 0 && index < options.size())
            {       
                options.insert(options.begin() + index, hidden_options[index]);
            }
            else{;}
        }
        else{;}
};

is much easier to read than:

void unhide_option(int index){
        if(hidden_options[index] != ""){
            if(index >= 0 && index < options.size()){       
                options.insert(options.begin() + index, hidden_options[index]);
            }else{;}
        }else{;}
    };

Answer 3

Here are some things that may help you improve your program.

Use only required #includes

The code includes <cstdlib> but does not appear to use it anywhere. Only include files that are actually needed.

Don't Repeat Yourself (DRY)

The range checking code appears in almost all of the functions. Instead of repeating code, it's generally better to make common code into a function.

Eliminate spurious semicolons

The code has a number of spurious semicolons immediately after closing braces and within otherwise empty braces. They don't bother the compiler but they will bother any other programmer who looks at your code.

Write member initializers in declaration order

The menu class has this constructor

menu(const std::string& title, const std::initializer_list<std::string>& options)
    : title(title), options(options), current_index(default_index){;};

That looks fine, but in fact, options will be initialized before title because members are always initialized in declaration order and options is declared before title in this class. To avoid misleading another programmer, you should swap the order of those such that it says instead:

menu(const std::string& title, const std::initializer_list<std::string>& options)
    : options(options), title(title), current_index(default_index)
{} 

This way the initialization actually proceeds from left to right as one might expect at first glance. It also eliminates spurious semicolons as mentioned in the previous suggestion.

Reconsider your data structures

Your hide_option and unhide_options code could be much simpler if you choose a different data structure. In particular, you could declare a parallel vector of bool to simply hold a flag saying whether an option was hidden or not. Better still, see the next suggestion:

Think carefully about objects

This code could be much simpler and easier to read if it used a MenuOption class. Then all you would need would be a std::vector of such objects. The class might be implemented like this:

class MenuOption 
{
public:
    MenuOption(const std::string &opt) 
        : text{opt}, hidden{false}
    {}
    operator bool() const { return hidden; }
    void hide() { hidden = true; }
    void unhide() { hidden = false; }
    friend std::ostream& operator<<(std::ostream &out, const MenuOption &opt) {
        return out << opt.text;
    }
private:
    std::string text;
    bool hidden;
};

Eliminate global variables where practical

The code declares and uses 4 global variables. Global variables obfuscate the actual dependencies within code and make maintenance and understanding of the code that much more difficult. It also makes the code harder to reuse. For all of these reasons, it's generally far preferable to eliminate global variables. In this case, it would be easy to reimplement all of them as members of the menu class instead or as unnamed default values within the constructor. The latter would look like this:

menu(const std::string& title = "", 
    const std::initializer_list<std::string>& options = {})
: options(options), title(title), current_index(-1)
{} 

Prefer class to struct

The menu class should be a class and have private data members rather than public ones.

Return something where practical

The current_index member variable is quite suspect. Instead of using that, I'd recommend instead having obtain_option return a value instead. That function could be written like this:

int obtain_option(int index) const {
    return isInRange(index) ? index : invalid_index;
}

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.