4
\$\begingroup\$

I wrote a text adventure game a while back and today I looked at it to see how I did the word wrapping (to use in a new project); I'm curious if I went about it the right way. I realize I could use an array instead of a list and remove 2 dependencies from the class but wanted some other feedback as I may have missed something obvious.

    public static string GetBorderedText(string Input, int AbsoluteLength, bool splitLines = true, int Offset = -4)
    {
        string outputString = "";

        if(Input.Length > AbsoluteLength + Offset && splitLines)
        {
            List<string> words = Input.Split(' ').ToList();
            string lineConstruction = "";

            foreach (string s in words)
            {
                if (lineConstruction.Length + s.Length + 1 >= AbsoluteLength + Offset)
                {
                    outputString += "+ " + GetPaddedSubstring(lineConstruction, AbsoluteLength, Offset) + " +" + Environment.NewLine;
                    lineConstruction = "";
                    lineConstruction += s + " ";
                }
                else
                {
                    lineConstruction += s + " ";
                }
            }

            outputString += "+ " + GetPaddedSubstring(lineConstruction, AbsoluteLength, Offset) + " +";

            return outputString;
        }
        else
        {
            outputString = "+ " + GetPaddedSubstring(Input, AbsoluteLength, Offset) + " +";
            return outputString;
        }
    }

where GetPaddedSubstring is used for getting a substring padded to a certain length.

    public static string GetPaddedSubstring(string Input, int AbsoluteLength, int Offset)
    {
        int targetLength = AbsoluteLength + Offset;
        string output = "";

        if(Input.Length > targetLength)
        {
            return Input.Substring(0, targetLength);
        }
        else
        {
            output = Input.PadRight(targetLength);

            return output;
        }
    }

Lastly, for some examples. GetBorderedText can take in something like

"This room looks sturdy, but old. Lots of storage and containers. There is a door on the far side."

And return something like this.

+ This room looks sturdy, but old. Lots of storage and containers. There is a door     +
+ on the far side.                                                                     +

The + on the side are just to create a nice border within the Console window. (Hence the -4 default on Offset)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ In GetPaddedSubstring, remove the else and the output variable, they aren't needed. \$\endgroup\$
    – PmanAce
    Mar 29 '18 at 1:51
7
\$\begingroup\$
GetBorderedText(string Input, int AbsoluteLength, bool splitLines = true, int Offset = -4)

You are very inconsistent with naming parameters. Some of them are in PascalCase while only one has the correct camelCase. This makes your code look strange and unprofessional.


if (Input.Length > AbsoluteLength + Offset && splitLines)

Conditions like this are better stored in a variable that describes what its purpose is and split into two. Consider this:

var wrapRequired = input.Length > absoluteLength + offset;
if (wrapRequired && canWrap)

input.Split(' ').ToList()

You don't need to call ToList, it's already an array and can be enumerated.


foreach (string s in words)

If your collection is called words then name the item word and not s. It's very confusing when one looks at the code and wonders what that s is.


lineConstruction.Length + word.Length + 1 >= absoluteLength + offset

The same here. You should name this condition propertly and especially the magic 1.


Other than this

  • you should use StringBuilder for concatenating strings in loops because it's not only faster but also easier to use.
  • make the border as prefix & suffix parameters instead of hardcoding them (or leftBorder & rightBorder).
  • use variables with stronger names and use more of them so that they document your code.

As an example of how you can apply all these suggestions here's the new code with one new method for adding the border.

public static string CreateBorderedText(string input, int maxLength, bool canWrap = true, string prefix = "+ ", string suffix = " +")
{
    const string wordSeparator = " ";

    var lines = new List<string>();
    var lineBuilder = new StringBuilder();
    var borderLength = prefix.Length + suffix.Length;
    var contentLength = maxLength - borderLength;

    var wrapRequired = input.Length > contentLength;
    if (wrapRequired && canWrap)
    {
        var words = input.Split(' ');
        foreach (var word in words)
        {
            var projectedLength = lineBuilder.Length + word.Length + wordSeparator.Length + borderLength;
            var wrap = projectedLength >= maxLength;
            if (wrap)
            {
                PadRight(lineBuilder, contentLength);
                AddBorder(lineBuilder, prefix, suffix);

                lines.Add(lineBuilder.ToString());
                lineBuilder.Clear();                
            }

            lineBuilder.Append(word).Append(wordSeparator);
        }
    }
    else
    {
        lineBuilder.Append(input);      
    }

    PadRight(lineBuilder, contentLength);
    AddBorder(lineBuilder, prefix, suffix);
    lines.Add(lineBuilder.ToString());

    return string.Join(Environment.NewLine, lines);
}

and the two helpers

private static void AddBorder(StringBuilder lineBuilder, string prefix, string suffix)
{
    lineBuilder
        .Insert(0, prefix)
        .Append(suffix);
}

private static void PadRight(StringBuilder lineBuilder, int targetLength)
{
    lineBuilder.Append(' ', targetLength - lineBuilder.Length);
}
\$\endgroup\$
4
  • \$\begingroup\$ "you should use StringBuilder for concatenating strings in loops because it's not only faster but also easier to use" not really true in some cases where the loop count will be small. \$\endgroup\$
    – PmanAce
    Mar 29 '18 at 13:49
  • \$\begingroup\$ @PmanAce, Why do you say that it is not really true in the cases where the loop count will be small? should we not also plan for the future? \$\endgroup\$
    – Malachi
    Apr 2 '18 at 19:39
  • \$\begingroup\$ @Malachi Using StringBuilder to concatenate 2 or 3 strings is less efficient than using $"" or + for example. \$\endgroup\$
    – PmanAce
    Apr 2 '18 at 20:34
  • \$\begingroup\$ why is it less efficient? when does it become more efficient to use StringBuilder than to use Interpolation or concatenation? all I hear you saying is "no it's not". this code has the possibility of looping many times, so the efficiency may not be good in some circumstances but in others it would be much more efficient in others. please explain your reasoning and give something for us to see as proof, it's not that we don't trust you, it's just that we like information and knowledge and want to know the big question "Why?" \$\endgroup\$
    – Malachi
    Apr 2 '18 at 20:39
3
\$\begingroup\$

first thing that I would do is to move the return statement outside the if else statement, because you are returning the outputString no matter what, this also leads to moving the following line outside of it's immediate if statement but to keep it inside of the foreach

lineConstruction += s + " ";

This DRYs out your code a little bit and looks like this

public static string GetBorderedText(string Input, int AbsoluteLength, bool splitLines = true, int Offset = -4)
{
    string outputString = "";

    if(Input.Length > AbsoluteLength + Offset && splitLines)
    {
        List<string> words = Input.Split(' ').ToList();
        string lineConstruction = "";

        foreach (string s in words)
        {
            if (lineConstruction.Length + s.Length + 1 >= AbsoluteLength + Offset)
            {
                outputString += "+ " + GetPaddedSubstring(lineConstruction, AbsoluteLength, Offset) + " +" + Environment.NewLine;
                lineConstruction = "";

            }
            lineConstruction += s + " ";
        }

        outputString += "+ " + GetPaddedSubstring(lineConstruction, AbsoluteLength, Offset) + " +";
    }
    else
    {
        outputString = "+ " + GetPaddedSubstring(Input, AbsoluteLength, Offset) + " +";
    }
    return outputString;
}

you should get rid of the output string variable from the GetPaddedSubstring() method, because you don't really need it. inside the if block you return the value immediately, but in the Else statement you assign the value to the variable and then return the value of the variable, skip that extra step and just return the value like this

public static string GetPaddedSubstring(string Input, int AbsoluteLength, int Offset)
{
    int targetLength = AbsoluteLength + Offset;
    if(Input.Length > targetLength)
    {
        return Input.Substring(0, targetLength);
    }
    else
    {
        return = Input.PadRight(targetLength);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ I added some additional context. I know the GetPaddedSubstring won't be used if I salvage anything from here but wanted to add it for context. \$\endgroup\$
    – Tyler C
    Mar 26 '18 at 16:14
  • \$\begingroup\$ Trailing elses aren't needed by the way. \$\endgroup\$
    – PmanAce
    Mar 29 '18 at 13:50
-2
\$\begingroup\$

Here is your code cleaned up a bit (even removed 2 dependencies):

namespace Tests
{
    using System;

    class Program
    {
        private static void Main(string[] args)
        {
            Console.WriteLine(GetBorderedText("This room looks sturdy, but old. Lots of storage and containers. There is a door on the far side.", 80));
            Console.ReadKey();
        }

        private static string GetBorderedText(string input, int maxLength, bool splitLines = true, int offset = -4)
        {
            if (input.Length <= maxLength + offset && splitLines)
                return $"+ {GetPaddedSubstring(input, maxLength, offset)} +";

            var outputString = string.Empty;
            var lineConstruction = string.Empty;

            foreach (var s in input.Split(' '))
            {
                if (lineConstruction.Length + s.Length + 1 >= maxLength + offset)
                {
                    outputString += $"+ {GetPaddedSubstring(lineConstruction, maxLength, offset)} +{Environment.NewLine}";

                    lineConstruction = string.Empty;
                }

                lineConstruction += $"{s} ";
            }

            outputString += $"+ {GetPaddedSubstring(lineConstruction, maxLength, offset)} +";

            return outputString;
        }

        private static string GetPaddedSubstring(string input, int maxLength, int offset)
        {
            var targetLength = maxLength + offset;

            if (input.Length > targetLength)
                return input.Substring(0, targetLength);

            return input.PadRight(targetLength);

        }
    }
}

You could put a StringBuilder in there as mentioned earlier but if the number of lines is not many, I wouldn't bother.

\$\endgroup\$
3
  • \$\begingroup\$ What did you do and why? \$\endgroup\$
    – Malachi
    Mar 29 '18 at 13:52
  • \$\begingroup\$ This is what he said: "I'm curious if I went about it the right way. I realize I could use an array instead of a list and remove 2 dependencies from the class but wanted some other feedback", it is exactly what I did, used array and removed 2 dependencies and gave him other feedback like string building and code cleaning. \$\endgroup\$
    – PmanAce
    Mar 29 '18 at 13:53
  • \$\begingroup\$ did he go about it the right way? that is his first question, implicitly asking for the reasoning of the answer. then he says he wants "other feedback" which requires more than dumping the code that he suggests he might want to have instead, I would think he already knows how to write the code, he wants to know why would I do it this way over the other way. \$\endgroup\$
    – Malachi
    Mar 29 '18 at 13:58

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.