3
\$\begingroup\$

As a lone developer I don't really have anyone to peer review my work in-house so I was hoping someone could take a look at this class and tell me in what ways I could improve it.

Essentially what this class does is receive a string from a socket that could possibly have multiple messages in it and isolates these messages based on the message terminator. In the isolate message method, it acquires the number of terminators with split and then loops through and adds the substring to the list of messages. Afterwards it removes that substring from the data string.

Off the top of my head the first opts I spot are using an array based off the terminator count instead of a list and making the terminator string array global so it's not making a new array each time.

internal class MessageHandler
{
    const string m_MessageTerminator = "<|>";
    int m_TerminatorLength = m_MessageTerminator.ToArray().Length;

    internal void ProcessStream(SocketState state)
    {
        ///Extract individual messages from socket message queue.
        ProcessMessages(IsolateMessages(state.MessageQueue.ToString()));

        //Find the last index of the message terminator and remove the substring from the string builder.
        //We do not want to clear the string builder because it could have partial messages inside.
        int lastTerminatorIndex = state.MessageQueue.ToString().LastIndexOf(m_MessageTerminator);
        state.MessageQueue.Remove
            (0, lastTerminatorIndex + m_TerminatorLength);
    }

    internal string[] IsolateMessages(string data)
    { 
        List<string> messages = new List<string>();
        string[] terminators = new string[] { m_MessageTerminator };
        int terminatorCount = data.Split(terminators, StringSplitOptions.None).Length -1;

        for(int i = 0; i < terminatorCount; i++)
        {
            //Find The First Instance Of Message Termination
            int terminatorIndex = data.IndexOf(m_MessageTerminator);

            //Extract The Message Based On Terminator's Index
            //Then Remove The Terminator From The Message.
            messages.Add(data.Substring
                (0, terminatorIndex + m_TerminatorLength)
                    .Remove(terminatorIndex, m_TerminatorLength));

            //Remove The Message We Just Extracted From Overall Data Set
            data = data.Remove(0, terminatorIndex + m_TerminatorLength);
        }

        if (messages.Count > 0)
            return messages.ToArray();
        else
            return null;
    }

    internal void ProcessMessages(string[] messages)
    {
        if (messages == null)
            return;

        for(int i = 0; i < messages.Length; i++)
        {
            Console.WriteLine(messages[i]);
        }
    }
}
\$\endgroup\$
10
  • \$\begingroup\$ Could you please give a brief explanation of what this code is supposed to do, and what environment it is supposed to run in? You don't have any comments so it's time-consuming to figure out the logic. \$\endgroup\$
    – Snowbody
    Dec 20, 2017 at 21:57
  • \$\begingroup\$ Should I comment the code or explain it in paragraph form? My apologies. @Snowbody \$\endgroup\$
    – En'gai
    Dec 20, 2017 at 22:04
  • \$\begingroup\$ What's the definition of SocketState and MessageQueue? Your MessageQueue seems to be quite different than the .NET Framework's System.Messaging.MessageQueue, right? \$\endgroup\$
    – Snowbody
    Dec 20, 2017 at 22:10
  • \$\begingroup\$ Code comments always help. But you really are supposed to explain the purpose of the code and briefly the logic in prose. \$\endgroup\$
    – Snowbody
    Dec 20, 2017 at 22:11
  • 2
    \$\begingroup\$ Could you please show the form in which the messages arrive. Is each message terminated like "first<|>second<|> third<|>forth <|> last <|>". What should happen if the arrived data look like "first<|>second<|> third<|>forth <|><|>" or "first" \$\endgroup\$
    – Heslacher
    Dec 21, 2017 at 5:46

2 Answers 2

5
\$\begingroup\$

IsolateMessages()

You are creating a lot of strings here. Each time you call Substring(), or .Remove() on a string you are creating a new string. In addition the readability of

messages.Add(data.Substring
    (0, terminatorIndex + m_TerminatorLength)
        .Remove(terminatorIndex, m_TerminatorLength));

is very very bad.

  • By returning an IEnumerable<string> instead of string[] the execution of that method can be deffered by using yield.
  • By using the overloaded IndexOf(int, int) you could omit the terminatorCount and replace the for loop by a while loop.

Applying these two points would lead to this beautyful code

internal IEnumerable<string> IsolateMessages(string data)
{
    if (string.IsNullOrWhiteSpace(data)) { yield break; }

    var currentIndex = 0;
    var terminatorIndex = -1;

    while ((terminatorIndex = data.IndexOf(m_MessageTerminator, currentIndex)) > 0)
    {
        var messageLength = terminatorIndex - currentIndex;

        yield return data.Substring(currentIndex, messageLength);

        currentIndex = terminatorIndex + m_TerminatorLength;
    }
}  

the only difference to your code is that if there is no message or the passed string data is empty my message isn't returning null so you could change the ProcessMessages() like

internal void ProcessMessages(IEnumerable<string> messages)
{
    foreach(var message in messages)
    {
        Console.WriteLine(message);
    }
}
\$\endgroup\$
0
2
\$\begingroup\$

Comments inline

  • const string m_MessageTerminator = "<|>";
    int m_TerminatorLength = m_MessageTerminator.ToArray().Length;
    
    1. string instances actually have a .Length property already -- you don't have to cast it to an array first.
    2. Why is the terminator member declared as const, but the terminator length member is not? What's your intent? At least make it readonly.
  • ///Extract individual messages from socket message queue.
    

    You have the syntax for documentation comments wrong. Please see this tutorial.

  • ProcessMessages(IsolateMessages(state.MessageQueue.ToString()));
    
    1. Is MessageQueue a property or a field (member variable)? It's not readonly, right? So...
    2. Why convert the StringBuilder MessageQueue to a string here? You are writing IsolateMessages(), so just pass it as-is without copying.
    3. Since ProcessMessages() is just going to foreach over the collection of messages, the collection (both the input parameter to ProcessMessages() and the return type ofIsolateMessages()) should be of typeIEnumerableand thenIsolateMessages()will useyield return`. Please see this tutorial. That way the whole list doesn't need to remain in memory for the duration of things. This is called "lazh evaluation"; execution gets suspended until the next chunk is actually needed. Often resulting in a memory savings, which may result in increased performance due to better caching. Have to test it though.
  • int lastTerminatorIndex = state.MessageQueue.ToString().LastIndexOf(m_MessageTerminator);
    state.MessageQueue.Remove(0, lastTerminatorIndex + m_TerminatorLength);
    

    This seems like work that IsolateMessages() could do.

  • internal string[] IsolateMessages(string data)
    

    I reiterate my suggestion that this return an IEnumerable<string> and use yield return rather than building a List<string> and then converting it to a string[].

  • string[] terminators = new string[] { m_MessageTerminator };
    

    This should be defined (and marked readonly) at the same place as m_MessageTerminator. It isn't going to change.

  • int terminatorCount = data.Split(terminators, StringSplitOptions.None).Length -1;
    

    Arrgh! What a waste! What a duplication of effort! The rest of the function after this point does the exact same thing as string.Split(), whose return value you just threw away. Check out the documentation.

  •   messages.Add(data.Substring(0, terminatorIndex + m_TerminatorLength).Remove(terminatorIndex, m_TerminatorLength));
    

    Quite aside from the previous bullet point, this line makes no sense. The code deliberately adds on m_TerminatorLength characters, then removes them again.

  •    data = data.Remove(0, terminatorIndex + m_TerminatorLength);
    

    If you're going to be doing a lot of operations on text, you should do it in a StringBuilder, because each time you do so you create and destroy temporary strings, wasting speed and memory. But you really don't have to. Just keep track of the index and do just one remove at the end. (Heslacher showed you this in his answer)

  •        if (messages.Count > 0)
            return messages.ToArray();
        else
            return null;
    

    Why a null return here? Wouldn't a 0-element array be better? Then you wouldn't need a special case in ProcessMessages.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ PascalCase is also used for (public) properties and fields, not just for methods and classes. Regarding yield, I think it's important to note that this makes a method 'lazy': it only does work when the result is actually iterated. This may cause problems in some cases if one is not aware of the difference (work not being done, or being done multiple times). \$\endgroup\$ Dec 21, 2017 at 8:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.