7
\$\begingroup\$

My experience is more general architecture and software design not code snippets like this. See any improvement to my code?

var message = @"This is a test message 
012345678901234567890123";

var output = Split(message, 20);

output

"This is a test",
"message",
"01234567890123456789",
"0123"

Max length of a line is 20 chars and it does not split words if not needed

code:

private IEnumerable<string> Split(string text, int maxLength)
{
    var n = '\n';
    var whiteSpaces = new HashSet<char> { ' ', '\t', n };
    text = text.Replace(Environment.NewLine, n.ToString());
    if (!text.EndsWith(n)) text += n;

    var whiteSpaceIndices = text
        .Select((c, i) => (i, c))
        .Where(t => whiteSpaces.Contains(t.c))
        .ToList();

    var index = 0;
    var line = string.Empty;

    char? last = null;

    foreach (var white in whiteSpaceIndices)
    {

        do
        {
            var wordLength = white.i - index;
            var wordTrimmed = text.Substring(index, Math.Min(maxLength, wordLength));
            var wordWasTrimmed = wordLength > maxLength;
            var trimmedTotalLength = wordTrimmed.Length + (wordWasTrimmed ? 0 : 1);

            if (line.Length + trimmedTotalLength > maxLength || last == n)
            {
                if (line != string.Empty)
                    yield return line;

                last = null;
                line = string.Empty;
            }

            line += last;
            line += wordTrimmed;
            index += trimmedTotalLength;

        } while (index < white.i);

        last = white.c;
    }

    if (line != string.Empty)
        yield return line;
}

https://dotnetfiddle.net/gpT5UY

Target framework is Core 3.1

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Before I comment, what improvements are you hoping for, or rather what is you criticism of this logic. There are slightly simpler ways to achieve the same outcome but apart from some code comments this is not too bad. Also what environments are you expecting to execute this code and with what sort of frequency? \$\endgroup\$ – Chris Schaller Sep 27 '20 at 11:58
  • 1
    \$\begingroup\$ I was looking for a more elegant way of solving the same thing basicly. It will be executed in azure service fabric, core 3.1 performance isn't super critical a few thousand calls to the method and the texts are not longer than 50 to 200 chars. It's a backend process \$\endgroup\$ – Anders Sep 27 '20 at 13:16
  • \$\begingroup\$ Could you please add the expected environment to the question? \$\endgroup\$ – pacmaninbw Sep 27 '20 at 16:55
  • \$\begingroup\$ Done target framework is Core 3.1 \$\endgroup\$ – Anders Sep 27 '20 at 17:19
  • \$\begingroup\$ And you want to allow lines that have more than maxLength characters if there are no whitespace characters to split on? (so an individual word with more than maxLength characters) or should these words be split and an optional hyphen or continuation character/string be injected? \$\endgroup\$ – Chris Schaller Sep 28 '20 at 3:02
3
\$\begingroup\$

I'm not sure if i can review this in details. But I suggest to consider my own implementation of the same.

private static IEnumerable<string> Split(string text, int maxLength)
{
    char[] whiteSpaces = new[] { ' ', '\t' };
    string[] lines = text.Replace(Environment.NewLine, "\n").Split('\n');

    foreach (string line in lines)
    {
        int i = 0;
        string s = line;
        while (true)
        {
            s = s.Substring(i);
            string t = s.Trim();
            if (t.Length > maxLength)
            {
                int diff = s.Length - t.Length;
                i = t.LastIndexOfAny(whiteSpaces, maxLength);
                if (i < 0)
                    i = maxLength;
                yield return t.Remove(i);
                i += diff;
            }
            else
            {
                yield return t;
                break;
            }
        }
    }
}

Probably this code isn't absolutely accurate but it's ~5 times faster than original one.
I suggest you to test behavior and compare the performance.

The main slow points in your code are string concatenation operations.


Edit: Ok, the encrypted version for var lovers.

private static IEnumerable<string> Split(string text, int maxLength)
{
    var whiteSpaces = new[] { ' ', '\t' };
    var lines = text.Replace(Environment.NewLine, "\n").Split('\n');

    foreach (var line in lines)
    {
        var i = 0;
        var s = line;
        while (true)
        {
            s = s.Substring(i);
            var t = s.Trim();
            if (t.Length > maxLength)
            {
                var diff = s.Length - t.Length;
                i = t.LastIndexOfAny(whiteSpaces, maxLength);
                if (i < 0)
                    i = maxLength;
                yield return t.Remove(i);
                i += diff;
            }
            else
            {
                yield return t;
                break;
            }
        }
    }
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ A good solution, I disagree on the var statements though. In cases where the type is immediately obvious to the reviewing developer, I will usually accept var in a code review, and for logic processing will usually prefer it. In that way the code is transferrable and resilient to other contexts and type changes. So if the variable is created only to make the code more readable (instead of inlining) then IMO var is superior \$\endgroup\$ – Chris Schaller Sep 28 '20 at 23:25
  • \$\begingroup\$ @ChrisSchaller ok, removed that from the answer. \$\endgroup\$ – aepot Sep 29 '20 at 8:38
  • 1
    \$\begingroup\$ Looks good, full score if using var ;) \$\endgroup\$ – Anders Sep 29 '20 at 11:02
  • 1
    \$\begingroup\$ @Anders fixed. :) \$\endgroup\$ – aepot Sep 29 '20 at 12:51
3
\$\begingroup\$

Overall this is not a bad technique, there is a code smell though in the number of string concatenations, I'm not going to profile it, but I prefer solutions in string processing that write to string variables as little as possible.

What are you missing:

  1. There is no support or checking for null values
  2. There is no support or checking for a maxLength that is zero or less.

This achieves the same outcome, but assigns less string variables along the way.

private IEnumerable<string> Split(string text, int maxLength)
{
    if (String.IsNullOrWhiteSpace(text) || maxLength <= 0)
        yield return text;
    else
    {
        var whiteSpaces = new char[] { ' ', '\t', '\n' };
        text = text.Replace(Environment.NewLine, "\n");

        int index = 0;
        int totalLength = text.Length;
        string max = "";
        do
        {
            // skip any whitespaces, handles case or multiple consecutive whitespaces
            for (; index < totalLength; index++)
            {
                if (!whiteSpaces.Contains(text[index]))
                    break;
            }

            if (index + maxLength < totalLength)
                max = text.Substring(index, maxLength);
            else
                max = text.Substring(index);

            int maxBreak = max.LastIndexOfAny(whiteSpaces);
            if (maxBreak > 0)
            {
                yield return text.Substring(index, maxBreak);
                index += maxBreak;
            }
            else
            {
                yield return max;
                index += maxLength;
            }

        } while (index < totalLength);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for taking time to answer. It does not work exactly the same though. For example it forgets the original new lines :P \$\endgroup\$ – Anders Sep 29 '20 at 10:47
  • \$\begingroup\$ @Anders fair enough, I was not aware that was a requirement, in fact you can see I deliberately remove the extra lines \$\endgroup\$ – Chris Schaller Sep 30 '20 at 0:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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