Optimizing algorithm of analyzing incoming data

Question

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 < 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;

            if (oneMessage == "")
                return;

            if (oneMessage[0] == 'D')
            {
                if (oneMessage[1] == '1')
                {
                    if (oneMessage[2] == '1')
                    {
                        Dispatcher.RunAsync(Windows.UI.Core.CoreDispatcherPriority.Normal, () => {
                            DI1TB.Text = "1";
                        });  
                    }
                    else if (oneMessage[2] == '0')
                    {
                        Dispatcher.RunAsync(Windows.UI.Core.CoreDispatcherPriority.Normal, () => {
                            DI1TB.Text = "0";
                        });
                    }
                }
            }
}

I think that it's not really that important to descibe how this function exactly works. It analyzes data and sets Text property of DI1TB TextBlock.

So what I'm asking for is opinions of yours (and maybe some proposals) of what could be improved, what is wrong. As I said, I'm not an experienced programmer and this is just a side project of mine, which I hope will help me to develop some skills. I also hope you could help me with that, instruct me a little.

//EDIT I just tried running my application on a Windows Phone - it turns out that it doesn't work as good as on PC. On my Page I have a SyncFusion chart, which displays the data that is sent via bluetooth. It turns out, that on a mobile device displaying the graph (with dynamic data) and receiving data in the background is not so smooth. What should I do about it? I tried using a Timer, instead of running Async() in the Task. Even though my Timer has interval of 20ms it's not enough to display incoming data in time - there's is a noticeable delay (external device sends data at the interval of 400ms, so I don't know why 20ms is not enough for the phone to catch up). Do you have any ideas?

//EDIT2 If someone wants to have a look at the whole picture: here's the code of the application: https://1drv.ms/f/s!Atl-jM_xUkz1hf9DOeTqVWi0JKOU0g It's not all of it, I uploaded only 2 files, which are most important

Answer 1

This can be simplified quite a bit:

    if (oneMessage == "")
        return;

    if (oneMessage[0] == 'D')
    {
        if (oneMessage[1] == '1')
        {
            if (oneMessage[2] == '1')
            {
                Dispatcher.RunAsync(Windows.UI.Core.CoreDispatcherPriority.Normal, () => {
                    DI1TB.Text = "1";
                });  
            }
            else if (oneMessage[2] == '0')
            {
                Dispatcher.RunAsync(Windows.UI.Core.CoreDispatcherPriority.Normal, () => {
                    DI1TB.Text = "0";
                });
            }
        }
    }

It can be boiled down to:

if (oneMessage.StartsWith("D11"))
    Dispatcher.RunAsync(...);
else if (oneMessage.StartsWith("D10")
    Dispatcher.RunAsync(...);
else  //This else isn't needed and could be removed too
    return;

The other comment I have is that you are starting a Task with an infinite loop and no way to (gracefully) cancel it. You should read up on cancellation tokens and how to use them.

You also have an extra indentation level in your method, while it doesn't affect the performance or operation of your program, it is nice to get them lined up.

Answer 2

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 < 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;

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

Answer 3

Your Analyze does too much. From what I can see it

1. splits the input stream into individual messages
2. parses each message
3. processes the message

Those are different concerns which should have dedicated abstractions. I'm not sure whether this is a real example, since it looks pretty simplistic, but if it is I would:

1. Create a class to read the data source, write the data to buffer and notify your application when the complete message is read. Currently, you are constantly "analyzing" input, even if no new data is received and there is nothing to actually analyze. This will eat up both a CPU core and phone charge even though no useful work is being done. What you should do is put your thread to sleep and only wake it up, when complete message is received. This is a classic producer-consumer problem, you can look up a C# implementation on the internet. You should also gracefully terminate your analysis task when your application shuts down. Don't just leave it there.

2. Create one class per message and encapsulate the parsing logic there. For example:

   interface IMessage
   {
       bool TryParse(string input);
   }
   
   class HelloWorldMessage : IMessage
   {
       public bool TryParse(string input)
       {
          if (!input.StartsWith("Hello")) return false;
   
          //parse any meaningful information, if any
          return true;
       }
   }
   

3. Create a class that would loop over defined messages and parse the input

   var parsedMessage = definedMessages.FirstOrDefault(m => m.TryParse(oneMessage));
   if (parsedMessage == null)
   {
       //throw exception or something, if message had incorrect format
   }
   //send parsed message to processor(s)
   

4. A class or classes to process parsed messages. Using event aggregator to wire things up might prove useful in this case.

Answer 4

This is exactly your code logic
Not a single one of these test cases return any data

    public MainWindow()
    {
        InitializeComponent();
        string input = "D11*";
        Analyze(ref input);
        input += "D10*";
        Analyze(ref input);
        input += "trashD10*";
        Analyze(ref input);
        input = "AD10*";
        Analyze(ref input);
        input = "DA10*";
        Analyze(ref input);
        input = "AD11*";
        Analyze(ref input);
        input = "DA11*";
        Analyze(ref input);
    }
    public void Analyze(ref string input)
    {
        string oneMessage = "";

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

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

                if (startIndex < 0)
                    return;
                oneMessage = input.Substring(startIndex, indexOfStar + 1);
                input = input.Remove(0, indexOfStar + 1);
                Debug.WriteLine(input);
            }
        }
        else
            return;

        if (oneMessage == "")
            return;

        if (oneMessage[0] == 'D')
        {
            if (oneMessage[1] == '1')
            {
                if (oneMessage[2] == '1')
                {
                    Debug.WriteLine("DI1TB.Text = 1");
                }
                else if (oneMessage[2] == '0')
                {
                    Debug.WriteLine("DI1TB.Text = 1");
                }
            }
        }
    }

On top if it does not work

• ProcessMessage should just call Analyze and pass the message rather than have Analyze process in an endless loop;
• You look anywhere and then Remove from the start
  input = input.Remove(0, indexOfStar + 1);