[Possibly OT] How to handle text parsing functions?

[Jim Langston]

In my program I am accepting messages over the network and parsing them. I
find that the function that does this has gotten quite big, and so want to
break the if else code into functions. I started thinking of how to do
this, but came up with a number of ways and don't know what would be best,
and fit into the C++ idoism.

This is what I have now:

if ( ThisPlayer.Character.GMLevel == 100 && ( StrMessage == "/debugserver"
|| StrMessage == "/DEBUGSERVER" ) )
DebugMode = ! DebugMode;

else if ( StrMessage == "/list" || StrMessage == "/LIST" )
{
// lots of cod here
}

else if ( ThisPlayer.Character.GMLevel >= 50 && ( StrMessage.compare( 0, 8,
"/summon " ) == 0 || StrMessage.compare( 0, 8, "/SUMMON " ) == 0 ) )
{
// lots of code here
}

else if ( ThisPlayer.Character.GMLevel >= 50 && ( StrMessage.compare( 0, 8,
"/jumpto " ) == 0 || StrMessage.compare( 0, 8, "/JUMPTO " ) == 0 ) )
{
// lots of code here
}

etc.. There are actually 30 different else if's. As you can imagine, the
code is quite huge. The simplest thing would to take each // lots of code
here and make them functions and call them. But then I was thinking I could
just pass the GMLevel and StrMessage to the functions themselves. Then I
started thinking about maybe having some type of custom map type container
with the criteria (keyword and GMLevel) and a pointer to the function,
etc...

Over time this is just going to grow. This is the main text processing for
my online game and commands will only get added, not removed over time.

I can think of having a structure/class with Minimim GMLevel, keyword,
pointer to function call. There shouldn't be too much of a problem with
having to pass parameters to the class because of the game system constrains
I have a global structure that I can access what I need anyway.

What would be your suggestion?

 

[Jim Langston]

In a possible solution I am looking at this (something I just took 10
minutes to wrote but haven't tested yet). Any comments?

typedef bool (*CmdFuncPointer)( CPlayer& ThisPlayer, const std::string&
Message );

class CChatCommand
{
public:
unsigned int GMLevel; // minimum GMLevel
std::string Command; // Command string that calls (I.E. /kick)
CmdFuncPointer CmdFunc; // Command to process

CChatCommand( const unsigned int GMLevel, const std::string& Command,
const CmdFuncPointer CmdFunc ):
GMLevel( GMLevel ), Command( Command ), CmdFunc( CmdFunc ) {}
};

class CProcessChatMessage
{
public:
void AddCommand( const unsigned int GMLevel, const std::string& Command,
CmdFuncPointer CmdFunc )
{
Commands.push_back( CChatCommand( GMLevel, Command, CmdFunc ) );
}
bool ProcessMessage( CPlayer& ThisPlayer, const std::string& Message )
{
for ( std::vector<CChatCommand>::iterator it = Commands.begin(); it
!= Commands.end(); ++it )
{
if ( Message.compare( 0, (*it).Command.length(), (*it).Command )
== 0 &&
( (*it).Command[(*it).Command.length() - 1 ] != ' ' ||
Message.length() == (*it).Command.length() ) &&
ThisPlayer.Character.GMLevel >= (*it).GMLevel )
{
(*(*it).CmdFunc)( ThisPlayer, Message );
return true;
}
}
return false;
}
private:
std::vector<CChatCommand> Commands;
};

 

[Marcus Kwok]

In my program I am accepting messages over the network and parsing them. I
find that the function that does this has gotten quite big, and so want to
break the if else code into functions. I started thinking of how to do
this, but came up with a number of ways and don't know what would be best,
and fit into the C++ idoism.

This may not work for you since you are receiving the messages over the
network and they appear to be plain strings, but IMO the "idiomatic" way
to do it would be to have a "Command" base class with a virtual
"execute()" method or somesuch, and each specific command would be
derived from the Command base class. I believe there is a design
pattern for this.

 

[Dave Townsend]

This may not work for you since you are receiving the messages over the
network and they appear to be plain strings, but IMO the "idiomatic" way
to do it would be to have a "Command" base class with a virtual
"execute()" method or somesuch, and each specific command would be
derived from the Command base class. I believe there is a design
pattern for this.

You might want to look at the chain of command design pattern.

Each of you possibilities would be modelled as a separate subclass of a
common base class. You would register instances of these
classes to handle the messages. When a message arrives, you would iterate
through each of the instances
calling a virual function such as "execute()" . If the object handles that
message, it would process the message, then
return true, otherwise false, and you'd try the next object in the sequence.
You should check that you need to
continue processing after each "execute()" call .

The benefit of this approach is each object is simple and self contained.
Adding new messages is also very easy,
and the behavior can be dynamic, ie, messages handlers can be added or
removed as the situation requires.

 

[Jerry Coffin]

In my program I am accepting messages over the network and parsing them. I
find that the function that does this has gotten quite big, and so want to
break the if else code into functions. I started thinking of how to do
this, but came up with a number of ways and don't know what would be best,
and fit into the C++ idoism.

[ ... ]

Over time this is just going to grow. This is the main text processing for
my online game and commands will only get added, not removed over time.

I can think of having a structure/class with Minimim GMLevel, keyword,
pointer to function call. There shouldn't be too much of a problem with
having to pass parameters to the class because of the game system constrains
I have a global structure that I can access what I need anyway.

What would be your suggestion?

A couple of minor suggestions: first of all, instead of making the block
of code functions, I'd probably make them class objects. They give you
more flexibility in the long run. You might not need it now, but the
penalty is low (a couple extra lines of code for each) and in the long
run the flexibility is likely to pay off. For simplicity, I'd probably
derive them all from a fairly simple base class.

Second, instead of making the framework directly aware of the GMLevel,
I'd probably have each of those functors include a "check" function or
something like that, that could look at a player object to determine
whether the player can carry out that action. Make it a virtual
function, and have the base class version simply check whether the
player's GMLevel is high enough -- but someday, when you decide an
action requires GMLevel X _and_ the player to be carrying object Y (or
whatever) it'll be easy to handle. Again, the penalty now is minimal,
and the long-term payoff is potentially fairly high.

You also mentioned using a custom map type -- nothing you've said
convinces me that you need anything std::map doesn't provide. The
keyword is the key for your map, and the value type will be a pointer to
the "action" object. From what you've said so far, the base class would
look something like this:

class player;

class action {
int req_GMLevel;
public:
virtual bool check(player const &p) {
return p->GMLevel>= req_GMLevel;
}
virtual void exec() = 0;
}

From there your framework looks something like this:

// Do a case-insensitive comparison to avoid repeating comparisons,
// as well as allowing, e.g., "/List" along with "/list" and "/LIST"
//
struct cmp {
// Q&D case-insensitive comparison: convert a copy of each
// string to upper case, and compare those copies:
//
bool operator()(std::string a, std::string b) {
std::transform(a.begin(), a.end(), a.begin(), std::toupper);
std::transform(b.begin(), b.end(), a.begin(), std::toupper);
std::string::size_type len = b.size();
return a.compare(0, len, b) == 1;
}
};

// We'll be using this a couple of times and don't want to re-type it:
typedef std::map<std::string, action *, cmp> actions_map;

// The actual map from command names to actions:
actions_map actions;

// Create our actual actions. Each must override execute().
// Each can override check() if more complex criteria ever arise
// for qualifying a player to carry out a particular action:
class lister : public action { /* ... */ } list;
class summoner: public action { /* ... */ } summon;
class jumper : public action { /* .. .*/ } jump;
// 27 more action classes here...

// Set up the map from commands to actions:
actions["/list"] = &list;
actions["/summon"] = &summon;
actions["/jumpto"] = &jump;
// 27 more command/action pairs here...

// Execute a command from the user:
//
// look the command up in the map
actions_map::iterator it = actions.find(strMessage);

// if it wasn't found, tell the user:
if (actions.end() == it)
std::cerr<<"Sorry, I don't understand: \""<< strMessage<<"\"\n";
// it was found; check whether the player can do that
else if (it->second.check(player)))
// and if so, do it.
it->second.execute();
else
// otherwise, tell them they can't.
std::cerr << "Sorry, you can't do that\n";