Real time UDP packet decoder

I have basic C++ lectures at a university and I don't really know how I can optimize this line of code for a real-time application (I call this function every 5 ms for UDP packet decoding).

I would like to have some tips about this code:

• How can I improve its efficiency (in time processing rather than memory? (declare variable in the UDPmanager class, use static variable, ... ?)

• I am using openFrameworks for doing this project. Would it be better to use core C++ functions?

```
void UDPmanager::readPacket() {

udpConnection.Receive(udpMessage,100000);
string message=udpMessage;
static int count = 0;

if(message!="")
{
vector strSensor = ofSplitString(message,"[/c]");

vector strQoS = ofSplitString(strSensor[0],"|");

unsigned int receivedNumber = atoi(strQoS[0].c_str());

QoS_estimator(receivedNumber,message.length(),atoi(strQoS[1].c_str()));

if(receivedNumber strData = ofSplitString(strSensor[j],"[/p]");

for(size_t i=0; i value = ofSplitString(strData[i],"|");

if (value[0] == "[/i]") // Duplicate initialization frame
{
break;
}

if (value[0] == "POS")
{
sensors[j].touchpadPosition.x = (float)atoi(value[1].c_str())/10000.0;
sensors[j].touchpadPosition.y = (float)atoi(value[2].c_str())/10000.0;
}

else
{
if(value[0] == "TOUCH")
{
for(unsigned int i=0;i<sensors[j].data.size();i++)
{
sensors[j].data[i].touch = (atoi(value[i+1].c_str()))==1;
}
}

else if(value[0] == "TTHS")
{
for(unsigned int i=0;i<sensors[j].data.size();i++)
{
sensors[j].data[i].tths = atoi(value[i+1].c_str());
}

Welcome to CodeReview. If you are going to permanently keep your debug code around then you
might want to you might want to embed it within ifdefs such as

#ifdef DEBUG
    debug code ...
#endif

Generally, here in CodeReview, we frown on still having debug code within the source code.

Your debug code could be optimized by using "\n" in the first 2 print statements, std::endl
performs a flush operation on the output stream (Context switching is not good in code that needs to perform well).

Prefer std::cout Over using std;

If you are coding professionally you should get out of the habit of using the "using std;"
statement. The code will more clearly define where cout and other functions are coming from
(std::atoi, std::endl). As you start using namespaces in your code it is better to identify where
each function comes from because there may be function name collisions from different namespaces. The function cout you may override within your own classes.

Profile the Code

Use profiling tools to identify where the code is spending the most time.
This StackOverflow question discusses a profiling tool that works on Linux and Unix (Mac OS X). Breaking the code and following the Single Responsibility Principle may help with profiling.

Clean Up the Indentation

I know that the indentation problems I see in the code may be cut and paste errors but
it would make the code easier to read if it was all indented the same way.

An example of indentation problems, most of the following lines should start in the same column.

vector strSensor = ofSplitString(message,"[/c]");

         vector strQoS = ofSplitString(strSensor[0],"|");

         unsigned int receivedNumber = atoi(strQoS[0].c_str());

       QoS_estimator(receivedNumber,message.length(),atoi(strQoS[1].c_str()));

            if(receivedNumber<=previousPacketNumber)
            {
                sleep(1);
                return; // No need to use this packet. (= means it was duplicated in the network, < means it doesn't arrived in order)
            }

        previousPacketNumber=receivedNumber;

Missing Code

Reviewing the code would be easier if more of the UDPmanager Class was included, for instance
the packetLoss variable is not defined within the scope of the code.

The function ofSplitString is obviously a member of the UDPmanager, but we can't see what it does
and the code calls it 4 times (2 times in the loop), that function may be part of the performance
issue. It should definitely be part of the optimization.

First Optimization

If the message is empty why not just return from readPacket() immediately?

if (message=="") {
        return
    }

This would reduce the indentation in the rest of the code by one level.

Second Optimization

Reduce the number of string comparisons, in the lucky case of "POS" the code does only one string
comparison, in the most unlucky case of "DIFF" the code performs 7 string comparisons.

One way to reduce the string comparisons in this code is to have another function that converts
the string to an enum or integer constant. A function is not completely necessary see converting
strings to enums. I suggested a separate
function because the code is already too complex/long for one function, consider the Single Responsibility Principle.

If the code utilized an enum or symbolic constant, then the series of if then else if statements could
be reduced to a switch/case statement:

```
fieldToUpdate = convertStringToEnum(value[0]);
switch (fieldToUpdate) {
case POS:
sensors[j].touchpadPosition.x = (float)atoi(value[1].c_str())/10000.0;
sensors[j].touchpadPosition.y = (float)atoi(value[2].c_str())/10000.0;
break;

case TOUCH:
for(unsigned int i=0;i<sensors[j].data.size();i++)
{
sensors[j].data[i].touch = (atoi(value[i+1].c_str()))==1;
}
break;

case TTHS:
for(unsigned int i=0;i<sensors[j].data.size();i++)
{
sensors[j].data[i].tths = atoi(value[i+1].c_str());
}

case RTHS:
for(unsigned int i=0;i<sensors[j].data.size();i++)
{
sensors[j].data[i].rths = atoi(value[i+1].c_str());
}
break;

case FDAT:
for(unsigned int i=0;i<sensors[j].data.size();i++)
{
sensors[j].data[i].fdat = atoi(value[i+1].c_str());
}
break;

case BVAL:
for(unsigned int i=0;i<sensors[j].data.size();i++)
{
sensors[j].data[i].bval = atoi(value[i+1].c_str());
}
break;

case DIFF:
for(unsigned int i=0;i<sensors[j].data.size();i++)
{
sensors[j].data[i].diff = atoi(value[i+1].c_str());
}
break;

default:
ofLogError() << "Unrecognized key: " <<

StackExchange Code Review Q#159705, answer score: 7