HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Optimizing algorithm of analyzing incoming data

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
analyzingincomingalgorithmoptimizingdata

Problem

I've been making an app, which gets data from external device (via bluetooth) and displays received data.
I'm still pretty new to programming and I'm struggling to finish this app.

I wrote some code, that works well, but I wanted to ask you if my solution can be considered as a good one.

Firstly, some background. External device sends informations in a format like this: "D1x" - where 'x' can be '1' or '0'. Unfortunately I'm not getting the whole message at once. What I mean by that is the fact, that very often I get messages like: "D1", "1", and so on. So, in order to interprete the data, I need to wait until the whole frame is received. That's why i created a string called appData.readBuffer. Everytime I receive a message, I add it there. I do it like this:

public void OnMessageReceived(object source, string message)
{
    appData.readBuffer += message;
}


So, that's how I receive the messages. Now, the other part -the one where I interprete the data. In order to display the data almost instantly after it's sent, I created a Task, which runs my Analyze function in an infinite loop:

Task AnalyzerTask = Task.Run(() =>
        {
            while (true)
            {
                Analyze();
            }
        });


The above code is called at the end of my Page constructor.

Now here's the Analyze method:

```
public void Analyze()
{
string oneMessage = "";

if ((appData.readBuffer.Contains("A") || appData.readBuffer.Contains("D")) && appData.readBuffer.Contains("*"))
{
int indexOfStar = appData.readBuffer.IndexOf('*');
int indexOfA = appData.readBuffer.IndexOf('A');
int indexOfD = appData.readBuffer.IndexOf('D');

if (indexOfA {
DI1TB.Text = "1";
});
}
else if (oneMessage[2] == '0')
{
Dispatcher.RunAsy

Solution

You can also simplify this part

if ((appData.readBuffer.Contains("A") || appData.readBuffer.Contains("D")) && appData.readBuffer.Contains("*"))
        {
            int indexOfStar = appData.readBuffer.IndexOf('*');
            int indexOfA = appData.readBuffer.IndexOf('A');
            int indexOfD = appData.readBuffer.IndexOf('D');

            if (indexOfA < indexOfStar || indexOfD < indexOfStar)
            {
                int startIndex;
                if (indexOfA < indexOfD)
                    startIndex = indexOfA;
                else
                    startIndex = indexOfD;

                if (startIndex < 0)
                    return;
                oneMessage = appData.readBuffer.Substring(startIndex, indexOfStar + 1);
                appData.readBuffer = appData.readBuffer.Remove(0, indexOfStar + 1);
            }
        }
        else
            return;


Into this

int indexOfStar = appData.readBuffer.IndexOf('*');
        int indexOfA = appData.readBuffer.IndexOf('A');
        int indexOfD = appData.readBuffer.IndexOf('D');
        int startIndex = (indexOfA  -1 && indexOfStar > startIndex)
        {
            oneMessage = appData.readBuffer.Substring(startIndex, indexOfStar + 1);
            appData.readBuffer = appData.readBuffer.Remove(0, indexOfStar + 1);
        }
        else
            return;


1- No point on using .Contains if IndexOf = -1 already means its not present.

2- For calculating the startIndex, instead of If, Else just use the ? operator

3- You also check that indexOfA or indexOfD has to be smaller than indexOfStar. We already know that startIndex contains the smaller value between those two so by checking that indexOfStar is greater than startIndex and making sure that startIndex is greater than -1 we are ready to go, else return

Also, I'd like to add that you check if appData.readBuffer contains "A" OR "D", so even if one of them is not present it still goes inside the condition, then you retrieve their indexes and save the smallest into startIndex, but if one of them was not present it would have index = -1 and startIndex would acquire the -1 value. Afterwards you make sure its greater than -1 or else exit, but whats the point of doing all this if you know for sure that if one of the two characters is not found you will be forced to exit? What I mean is both contains "A" and contains "D" are required in order not to trigger return

Code Snippets

if ((appData.readBuffer.Contains("A") || appData.readBuffer.Contains("D")) && appData.readBuffer.Contains("*"))
        {
            int indexOfStar = appData.readBuffer.IndexOf('*');
            int indexOfA = appData.readBuffer.IndexOf('A');
            int indexOfD = appData.readBuffer.IndexOf('D');

            if (indexOfA < indexOfStar || indexOfD < indexOfStar)
            {
                int startIndex;
                if (indexOfA < indexOfD)
                    startIndex = indexOfA;
                else
                    startIndex = indexOfD;

                if (startIndex < 0)
                    return;
                oneMessage = appData.readBuffer.Substring(startIndex, indexOfStar + 1);
                appData.readBuffer = appData.readBuffer.Remove(0, indexOfStar + 1);
            }
        }
        else
            return;
int indexOfStar = appData.readBuffer.IndexOf('*');
        int indexOfA = appData.readBuffer.IndexOf('A');
        int indexOfD = appData.readBuffer.IndexOf('D');
        int startIndex = (indexOfA < indexOfD) ? indexOfA : indexOfD;

        if (startIndex > -1 && indexOfStar > startIndex)
        {
            oneMessage = appData.readBuffer.Substring(startIndex, indexOfStar + 1);
            appData.readBuffer = appData.readBuffer.Remove(0, indexOfStar + 1);
        }
        else
            return;

Context

StackExchange Code Review Q#147233, answer score: 4

Revisions (0)

No revisions yet.