Parsing command input with simple bimaps

Question

I've been playing with a simple toyrobot exercise to brush up some of my forgotten language skills and was wondering if there was a nicer way to implement my main command parsing loop. I think the use of the two maps and the nested ternary operator is readable but would be interested in seeing other approaches.

#include "toyrobot.hpp"
#include <iostream>
#include <regex>
#include <string>
#include <map>
int main () {
 using namespace std;
 map<string,int> dirtoi = {{"NORTH", 0}, {"EAST", 1}, {"SOUTH", 2}, {"WEST", 3}};
 map<int,string> itodir = {{0, "NORTH"}, {1, "EAST"}, {2, "SOUTH"}, {3, "WEST"}};
 regex place("^PLACE (\\d),(\\d),(NORTH|EAST|SOUTH|WEST)$");
 Robot robot = Robot();
 Option<Table> table = Some(Table(Point(0,0),Point(4,4)));
 smatch m;
 string l;
 while (getline(cin, l))
 {
 l == "MOVE" ? robot.move() :
 l == "LEFT" ? robot.left() :
 l == "RIGHT" ? robot.right() :
 l == "REPORT" ? robot.report(itodir) :
 regex_match (l,m,place) ? robot.place(Point(stoi(m[1]), stoi(m[2]))
 , dirtoi.at(m[3])/2.0, table) :
 (void)0;
 }
}

• Morwenn: The simple solution would be to use Boost.Bimap.

Answer 1

Here are some things I saw and some suggestions for how you might improve your code.

Try not to repeat information

I don't much care for the two parallel maps because they have the same information but in two different places and in two different constructs. What you actually seem to need is a way to go from a std::string to an int and vice-versa. For that, I'd write a simple class that does this:

template <class T>
class Bimap
{
public:
 Bimap(std::initializer_list<T> init) : names{init} {}
 const T& operator[](int i) const { return names[i]; }
 int operator[](const T& n) const { 
 for (unsigned i=0; i < names.size(); ++i) { 
 if (names[i] == n)
 return i;
 }
 return -1;
 }
private:
 std::vector<T> names;
};

Note that is, in fact, a templated class so it could be used with std::string or std::wstring or any other class that implements operator== and can be placed in a std::vector. The simple linear search is not as efficient as a std::map but it's probably just fine for short lists. Using it in main is just this line:

const Bimap<string> dir{{"NORTH", "EAST", "SOUTH", "WEST"}};

Use const where practical

Your regex and std::map variables could and should be const.

Separate parsing from actions

The code is actually attempting to do two different things: it is parsing a line from input and then calling some action based on what it finds. It's probably useful to separate those two a little more clearly. I'd probably use flex and bison if the commands become more complex, but for now you can simply use a more complete regex. Here's one way to do that:

const Bimap<string> simplecmd{{"MOVE", "LEFT", "RIGHT", "REPORT"}};
enum scmd { MOVE, LEFT, RIGHT, REPORT };
const regex command("^(MOVE|LEFT|RIGHT|REPORT)|PLACE (\\d),(\\d),(NORTH|EAST|SOUTH|WEST)$");

By reusing the Bimap class defined above, we can change any command into an integer. By expressing all possible commands within a regex, we can simply ignore any line which doesn't match (or perhaps print an error message).

I'm not happy about having the parallel constructs of simplecmd and the enum scmd but couldn't think of an easy way to avoid it in this case. We can rationalize it by saying that if one wished to have, say, both French and English versions, only the simplecmd would need to changed and not the enum.

Use better variable names

Generally, single letter variable names other than i and j for loop variables, are best avoided in favor of longer names. The variable m is not too terrible, but the variable l is definitely a poor choice, not least because it's easily mistaken for the digit 1 or the letter i. I changed it to line in the rewrite I did.

Use a switch statement where appropriate

Because your parser is selecting some action based on the input, we can make that much more explicit (and easier to read and maintain) by using a switch statement instead of the chained ternary operator. Here's one way to do it, incorporating all of the other suggestions:

smatch m;
string line;
while (getline(cin, line))
{
 if (regex_match(line, m, command)) {
 switch(simplecmd[m[1]]) {
 case MOVE:
 robot.move();
 break;
 case LEFT:
 robot.left();
 break;
 case RIGHT:
 robot.right();
 break;
 case REPORT:
 robot.report();
 break;
 default: // must be PLACE
 robot.place(Point(std::stoi(m[2]), std::stoi(m[3])), dir[m[4]], table);
 }
 }
}

• user71534: Thanks, that is the kind of solid feedback I was after, especially the points about separating the parsing and actions concerns.
• Edward: I should also have mentioned that another possibility is to have the parsing as a member function of Robot. Then most of the messy structures could be hidden within that class and you could simply pass strings to the robot: while (getline(cin, line)) robot.parse(line);

Answer 2

Instead of duplicating the data in dirtoi and itodir, it would be better to derive one from the other, or derive both from a common source, perhaps an enum.

To improve readability, you could start by avoiding single letter variable names. Especially the letter "l" (lowercase L), which is easy to confuse with a pipe or the number one.

That ternary is not terribly hard to read, but it's not that easy either, and it's not the right tool for the job. The right tool here would be a switch.

Another problem with the ternary is that it's using string values instead of something that the compiler can check for you, like values of an enum.