I see some things that may help you improve your program.
Separate the interface and implementation
Perhaps you posted it as just one file for the sake of simplicity and compactness, but if you were to actually use this, I think it's more likely that it would be useful as separate header and implementation files. This has the additional benefit of making the interface part of the code (the part that a user would have to see) a little shorter and easier to comprehend.
Allow the user to specify streams
Right now, the code is tied to std::cin and std::cout, but what if I wanted to use the menu, say, remotely over a socket connection? This could very easily be allowed by adding input and output stream references to the class. One way to do it would be to have std::istream and std::ostream arguments with defaults to std::cin and std::cout.
Prefer const references
The printChoices routine is currently this:
bool ConsoleMenu::printChoices() {
bool successful = false;
for (auto commandByChoice : commandsByChoice_) {
successful = static_cast<bool>(
out << '[' << commandByChoice.first << ']' << '\n');
}
return successful;
}
The for loop should instead use const auto & rather than auto to assure no copies are made of the strings. Further, the successful flag isn't really necessary. Instead we could write it like this:
bool ConsoleMenu::printChoices() const {
for (const auto &commandByChoice : commandsByChoice_) {
out << '[' << commandByChoice.first << ']' << '\n';
}
return !!out;
}
The !!out construct assures that out is interpreted as a bool but you could use a cast if you think it's more readable. (Perhaps I've been writing too much Javascript lately!)
Prefer unordered containers for performance
The std::map container has logarithmic search performance, but the std::unordered_map container has constant time performance. For that reason, it makes sense to prefer the unordered container unless there's actually a need for a sorted one. There isn't in this case.
Use const where practical
Right now, it's not possible to declare and use a const ConsoleMenu. There's no reason that shouldn't be possible. To do that, all of the functions except the constructor would have to be const. Mostly that means simply adding const to the declaration of the function, but we need to do a little more to adapt use. The reason is the last line:
commandsByChoice_[userChoice]();
There are a few problems with this simple line. First, the operator[] has a side effect you probably don't want. Specifically, if the key is not found, it is inserted into the map. That's definitely not what's wanted here whether we are trying for const or not. Two alternatives are using at (which can throw an exception) or find. A way to do that would be this:
const auto result{commandsByChoice_.find(userChoice)};
if (result != commandsByChoice_.end()) {
result->second();
}
Since you're already calling find in the helper function, it would make sense to only call it once and save (and use) the result. I'll show that later on.
Rename use
The use method is a bit awkwardly named in my view. I'd suggest instead to use operator()(). This would allow us to rewrite the sample main like this:
int main() {
bool running{true};
const ConsoleMenu menu{"What should this program do?\n",
"Invalid operation; ", "> ",
{
{"run", [&running] { running = true; }},
{"stop", [&running] { running = false; }}
}};
while(running) {
menu();
}
}
Note that I consistently use the C++11 uniform initialization syntax to reduce ambiguity.
Avoid re-doing work
Because there is no way within this code to insert or delete menu choices once the ConsoleMenu is created, we can create the entire menu as a static string once (perhaps within the constructor) and simply use that rather than iterating through the menu choices and making multiple calls to operator<< every time printChoices() is called. Further, one could create a single string that includes message_, all choices and prompt_.
Results
Using all of the suggestions above, we can simplify considerably and remove the two private helper functions. Here's the header file:
menu.h
#ifndef MENU_H
#define MENU_H
#include <functional>
#include <iostream>
#include <unordered_map>
#include <string>
class ConsoleMenu {
public:
ConsoleMenu(const std::string& message,
const std::string& invalidChoiceMessage,
const std::string& prompt,
const std::unordered_map<std::string, std::function<void()>>& commandsByChoice,
std::istream &in = std::cin,
std::ostream &out = std::cout);
void operator()() const;
private:
std::string invalidChoiceMessage_;
// the prompt_ string contains the message, all choices and prompt
std::string prompt_;
std::unordered_map<std::string, std::function<void()>> commandsByChoice_;
std::istream ∈
std::ostream &out;
};
#endif // MENU_H
And this is the implementation.
menu.cpp
#include "menu.h"
#include <sstream>
ConsoleMenu::ConsoleMenu(const std::string& message,
const std::string& invalidChoiceMessage,
const std::string& prompt,
const std::unordered_map<std::string, std::function<void()>>& commandsByChoice,
std::istream &in, std::ostream& out) :
invalidChoiceMessage_{invalidChoiceMessage},
commandsByChoice_{commandsByChoice},
in{in},
out{out}
{
std::stringstream ss;
ss << message;
for (const auto &commandByChoice : commandsByChoice_) {
ss << '[' << commandByChoice.first << ']' << '\n';
}
ss << prompt;
prompt_ = ss.str();
}
void ConsoleMenu::operator()() const {
std::string userChoice;
const auto bad{commandsByChoice_.cend()};
auto result{bad};
out << '\n';
while ((out << prompt_) && (!(std::getline(in, userChoice)) ||
((result = commandsByChoice_.find(userChoice)) == bad))) {
out << '\n' << invalidChoiceMessage_;
}
result->second();
}
Additional features
There are some additional things that might be nice. One thing that comes immediately to mind is that it would be useful to be able to create a constexpr ConsoleMenu. One way to do that might be to use C++17's std::string_view instead of std::string and to use an array instead of a map.
Another feature that might be nice would be the ability to optionally use wide strings. This is pretty easy to accommodate by turning the class into a templated class.
