• defualt_title anddefualt_options are misspelled.

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

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

Jamal hit it good. This should be a class withprivate 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{;}    };

• 2
  \$\begingroup\$The only difference between them is the curly brace placement. One distinction you could make (if it also fits your preferences) is removing eachelse{;} and adding a space after eachif and before each{.\$\endgroup\$

  Jamal

  – Jamal

  2015-09-05 16:43:17 +00:00

  CommentedSep 5, 2015 at 16:43
• \$\begingroup\$The use ofoptions.at() is reasonable, but that function does range checking and willthrow if the index is out of range. For that reason, I think it would only make sense if the current range checking was eliminated and the calling code would be expected to handle the exception.\$\endgroup\$

  Edward

  – Edward

  2015-09-05 19:01:34 +00:00

  CommentedSep 5, 2015 at 19:01

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

Themenu 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 initializedbeforetitle because members are always initialized indeclaration order andoptions is declared beforetitle 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

Yourhide_option andunhide_options code could be much simpler if you choose a different data structure. In particular, you could declare a parallel vector ofbool 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 aMenuOption class. Then all you would need would be astd::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 themenu 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){}

Preferclass tostruct

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

Return something where practical

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

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

Omitreturn 0

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

• c++
• game
• c++11
• role-playing-game