3

I have a Server class that processes QJsonObject data and handles it according to a key set in the data.

At the moment, I use a big if-then-else statement to decide what to do like this:

const QString action = jsonObject.value(KEY_ACTION).toString();
if (action == SOME_ACTION) {
    // do something
} else if (action == SOME_OTHER_ACTION) {
    // do something else
}

and so on. Now, meanwhile, I have quite a lot of actions, and for each one, my server has to check all cases until it finds the correct one. I thus wondered if there was a nicer way to do this.

I thought about having the data processing in different functions and having a QHash with the respective function pointer to the respective function for each action like this:

In the constructor:

const QHash<QString, void(Server::*)(const QJsonObject &)> processFunctionsMap {
    { SOME_ACTION, &Server::processSomeAction },
    { SOME_OTHER_ACTION, &Server::processSomeOtherAction }
}

And the respective functions:

void Server::processSomeAction(const QJsonObject &data)
{
    ...
}

and then to invoke the matching function:

if (! processFunctionsMap.contains(action)) {
    // Catch this case
}
(this->*processFunctionsMap.value(action))(jsonObject);

This seems to work, but I'm not a C++ pro, so my question is if this is the correct way to do it.

Tobias Leupold
  • 1,512
  • 1
  • 16
  • 41

1 Answers1

2

Your approach is reasonable, but you've changed the no-matches scenario from executing an else block (potentially doing nothing at all) to instant undefined behavior.

You need to separate the hash lookup from the call so you can insert a check for successful lookup in between. With C++ standard collections (std::map which is a red-black tree, std::unordered_map which is a hashtable), that'd be a call to find(key) which returns an iterator... you compare it to map.end() and make very sure not to dereference if they are equal. QHash, or any other non-standard hashtable, will surely provide something similar.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • Thanks for confirming doing so is okay :-) Sorry for the incomplete example, I'll edit my code to avoid undefined behavior. – Tobias Leupold Aug 12 '19 at 18:41