14
\$\begingroup\$

I made this method that takes any string and transforms it into a multiline text l; each line having the max (character) length specified by the rowLength parameter.

How can I make this better, functionality and code wise?

There is currently one known issue. It sometimes adds an empty line at the end because I have to add the Enviroment.NewLine call. Check it out, please, and give some suggestions.

public static string ConvertToMultiLineText(string value, int rowLength)
{
    var multiLine = new StringBuilder();
    string[] lines = value.Split(new[] { Environment.NewLine }, StringSplitOptions.None);
    var filterLines = lines.Where(line => !line.IsNullEmptyOrWhiteSpace());

    foreach (var line in filterLines)
    {
        if (line.IsNullEmptyOrWhiteSpace())
            continue;

        if (line.Length > rowLength)
        {
            int startIndex = 0;
            var number = (decimal)line.Length / rowLength;
            var rowNumber = Math.Ceiling(number);

            for (int i = 0; i < rowNumber; i++)
            {
                if (line.Contains(@" "))
                {
                    int lastIndex = rowLength;

                    // if it's not the first row set the last character on the current row of the iteration
                    if (i != 0)
                        lastIndex = (rowLength * (i + 1));

                    // if it's the last row set last character as the lenght of the last row
                    if (i == rowNumber - 1)
                        lastIndex = line.Length - 1;

                    int lastSpace = line.LastIndexOf(@" ", lastIndex, StringComparison.Ordinal);

                    //if " " is found inside the string
                    if (lastSpace > -1)
                    {
                        var lastOccurence = (lastSpace <= startIndex) ? startIndex - lastSpace : lastSpace - startIndex;

                        if (i == rowNumber - 1)
                            multiLine.Append(line.Substring(startIndex, lastOccurence) + Environment.NewLine + line.Substring(lastSpace, line.Length - lastSpace));
                        else
                            multiLine.Append(line.Substring(startIndex, lastOccurence) + Environment.NewLine);

                        startIndex = lastSpace;
                    }
                    else
                    {
                        if (i == rowNumber - 1)
                            multiLine.Append(line.Substring(startIndex, (line.Length - rowLength * ((int)rowNumber - 1))));
                        else
                            multiLine.Append(line.Substring(startIndex, rowLength) + Environment.NewLine);
                        startIndex += rowLength;
                    }
                }
                else
                {
                    if (i == rowNumber - 1)
                        multiLine.Append(line.Substring(startIndex, (line.Length - rowLength * ((int)rowNumber - 1))));
                    else
                        multiLine.Append(line.Substring(startIndex, rowLength) + Environment.NewLine);
                    startIndex += rowLength;
                }
            }
        }
        else
            multiLine.Append(line.Substring(0, line.Length) + Environment.NewLine);
    }

    return string.Format(@"{0}", multiLine);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Even though we can help you make this cleaner, Code Review is not the place to ask for how to fix unwanted behavior in your code (the empty line that you are mentioning). I think there's enough in here to be reviewable though, so again: Welcome to Code Review! \$\endgroup\$ – Simon Forsberg Jun 19 '14 at 11:18
  • \$\begingroup\$ Thanks!It's funny because i posted it on "normal" stackexchange and it got down-voted and i was suggested to post it here. :) \$\endgroup\$ – Gabriel Andrei Jun 19 '14 at 11:25
  • \$\begingroup\$ My first impression is that the function is way too complicated and needs breaking up for clarity and to make it more expressive of intent. Start by extracting the foreach loop to a method - you can use yield return from this each time you successfully extract a line. \$\endgroup\$ – Mr Cochese Jun 19 '14 at 14:30
4
\$\begingroup\$

This type of functionality does not require whole document to be loaded in to the memory. It might be easily done as a set of improvements over TextReader which provide better scaling possibilities:

public static class WordWrap
{
    public static string Wrap(this string text, int lineLength)
    {
        using (var reader = new StringReader(text))
            return reader.ReadToEnd(lineLength);
    }

    public static string ReadToEnd(this TextReader reader, int lineLength)
    {
        return string.Join(Environment.NewLine, reader.ReadLines(lineLength));
    }

    public static IEnumerable<string> ReadLines(this TextReader reader, int lineLength)
    {
        var line = new StringBuilder();
        foreach (var word in reader.ReadWords())
            if (line.Length + word.Length < lineLength)
                line.Append($"{word} ");
            else
            {
                yield return line.ToString().Trim();
                line = new StringBuilder($"{word} ");
            }

        if (line.Length > 0)
            yield return line.ToString().Trim();
    }

    public static IEnumerable<string> ReadWords(this TextReader reader)
    {
        while (!reader.IsEof())
        {
            var word = new StringBuilder();
            while (!reader.IsBreak())
            {
                word.Append(reader.Text());
                reader.Read();
            }

            reader.Read();
            if (word.Length > 0)
                yield return word.ToString();
        }
    }

    static bool IsBreak(this TextReader reader) => reader.IsEof() || reader.IsWhiteSpace();
    static bool IsWhiteSpace(this TextReader reader) => string.IsNullOrWhiteSpace(reader.Text());
    static string Text(this TextReader reader) => char.ConvertFromUtf32(reader.Peek());
    static bool IsEof(this TextReader reader) => reader.Peek() == -1;        
}

As you can see we have many ways to use it: String.Wrap(int lineLength), TextReader.ReadWords(), TextReader.ReadLines(int lineLength), TextReader.ReadToEnd(int lineLength).

Example:

class Program
{
    public static void Main()
    {
        var text = "I made this method that takes any string and transforms it into a multiline text l; each line having the max (character) length specified by the rowLength parameter. How can I make this better, functionality and code wise? There is currently one known issue.It sometimes adds an empty line at the end because I have to add the Enviroment.NewLine call. Check it out, please, and give some suggestions.";
        Console.WriteLine(text.Wrap(lineLength:20));
    }
}

ANE checking is missing for brevity. Comments are appreciated.

\$\endgroup\$
  • 2
    \$\begingroup\$ This should be the most upvoted and accepted answer... \$\endgroup\$ – t3chb0t Nov 4 '18 at 7:25
  • \$\begingroup\$ @t3chb0t Yeah, this is better and more manageable than the accepted answer. \$\endgroup\$ – r.mirzojonov May 19 at 16:33
7
\$\begingroup\$

First Impressions
I haven't gone through the functionality in detail but a few things strike me immediately.

Why explicitly include StringSplitOptions.None. They default value is none.

IsNullEmptyOrWhiteSpace() seems to be a (not included) extension method for string that performs the same function as String.IsNullOrWhiteSpace. Does it do something else as well?

The check for an empty 'line' inside the loop

if (line.IsNullEmptyOrWhiteSpace())
        continue;

should be redundant as we have previously filtered the lines

var filterLines = lines.Where(line => !line.IsNullEmptyOrWhiteSpace());

I would need to do up some performance checks to be sure but I reckon that the use of the StringBuilder (presumably for performance reasons) will be negated by the calls to line.SubString().

I can't tell for certain what the requirements are - I would guess

  • Max line Length of n
  • Do not split words
  • Do not end on a space
  • Do not start on a space

but it look like a linear progression through the string, with some back tracking to deal with split words/spaces, might work and appending the individual chars might give you better performance.

The final return should be

return multiLine.ToString();

there is no need to create another string and place the result of multiLine.ToString() inside it and then return the anonymous string.

\$\endgroup\$
7
\$\begingroup\$

Your code is complicated but I think it's can be done easier. I prefer to take text, split it in words and then construct new lines.

public static string SplitLineToMultiline(string input, int rowLength)
{        
    StringBuilder result = new StringBuilder();
    StringBuilder line = new StringBuilder();

    Stack<string> stack = new Stack<string>(input.Split(' '));

    while ( stack.Count > 0 )
    {
        var word = stack.Pop();
        if ( word.Length > rowLength )
        {
            string head = word.Substring(0, rowLength);
            string tail = word.Substring(rowLength);

            word = head;
            stack.Push(tail);
        }

        if ( line.Length + word.Length > rowLength)
        {
            result.AppendLine(line.ToString());
            line.Clear();
        }

        line.Append(word + " ");
    }

    result.Append(line);
    return result.ToString();
}
\$\endgroup\$
  • \$\begingroup\$ Very nice algorithm but in my case, the result of your function return words in the wrong order. For example: "I'm not able to respond" will be output "respond\r\nI'm not able to" but in my case, I want to have as a result "I'm not able to\r\nrespond". Instead of trying some weird things, I use LinkedList like in this post stackoverflow.com/questions/848082/… and I just change a couples of lines to use "First" instead of "pop" and "AddLast" instead of "Push". Note that you also must call "RemoveFirst" after getting "First" result. \$\endgroup\$ – Samuel Sep 4 '15 at 18:40
  • \$\begingroup\$ Wanted to point out that the result is reversed (not just the wrong order), but @nassimlouchani has noticed this too and added a stringreverse function down in the answers. \$\endgroup\$ – Wouter Vanherck Jan 4 at 8:08

protected by Jamal Jan 4 at 9:11

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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