Frame Strings that contain newlines

Question

I have an application where I need to highlight some information that is printed to the console, so I wrote some static library methods which do that. I tested for bugs. Is my code clean and satisfying, or are there aspects to criticise?

It has to work for multiple lines - given as a string that contains newlines and for arrays and lists.

• Compile with: javac *.java
• Run with: Main.java

Example Output

+---------------------------+
| one                       |
| two three                 |
| four                      |
| let your dreams come true |
| nothing is impossible     |
+---------------------------+
+--------+
| Single |
+--------+

Main.java

import java.util.List;
import java.util.ArrayList;
import java.util.Arrays;

public class Main {
    public static void main(String[] args) {
        String string = "one\ntwo three\nfour\nlet your dreams come true\n";
        string += "nothing is impossible";
        System.out.println(makeBorderString(string));
        
        System.out.println(makeBorderString("Single"));
    }
    
    // frames a string that contains newlines
    // does not work for strings that contain symbols that need more than one
    // space unit, for example tabs
    public static String makeBorderString(String value) {
        String[] lines = value.split("\\r?\\n");
        return makeBorderStringFromArray(lines);
    }
    
    // does not work for strings that contain symbols that need more than one
    // space unit, for example tabs
    public static String makeBorderStringFromArray(String stringArray[]) {
        List<String> stringList = Arrays.asList(stringArray);
        return makeBorderStringFromList(stringList);
    }
    
    // does not work for strings that contain symbols that need more than one
    // space unit, for example tabs
    public static String makeBorderStringFromList(List<String> stringList) {
        // find most long line
        int mostLong = 0;
        for (int i = 0; i < stringList.size(); i++) {
            if (stringList.get(i).length() > mostLong) {
                mostLong = stringList.get(i).length();
            }
        }
        
        StringBuilder sb = new StringBuilder();
        
        // make upper Border border
        sb.append("+" + repeatSymbol('-', mostLong + 2) + "+\n");
        
        // make body
        for (int i = 0; i < stringList.size(); i++) {
            String current = stringList.get(i);
            int whiteSpaceAmount = mostLong - current.length();
            if (whiteSpaceAmount > 0) {
                String whitespace = repeatSymbol(' ', + whiteSpaceAmount);
                sb.append("| " + current + whitespace + " |\n");
            } else {
                sb.append("| " + current + " |\n");
            }
            
        }
        
        // make lower Border border
        sb.append("+" + repeatSymbol('-', mostLong + 2) + "+");
        
        return sb.toString();
    }
    
    public static String repeatSymbol(char symbol, int times) {
        if (times > 0) {            
            StringBuilder sb = new StringBuilder();
            for (int i = 0; i < times; i++) {
                sb.append(symbol);
            }
            return sb.toString();
        }
        return null;
    }
}

Answer 1

First of all, the solution is sound and rather well-structured. So, take my comments as hints towards production-quality coding.

Class structure

The algorithm for framing is in a class named Main, which also contains the main() method.

Place it into a class with a better name, e.g. StringFramer, and omit the main() method there. This way, it's much easier to re-use it. (But maybe, you just pasted together a small class, just for this question - then everything is fine, probably.)

If you want to test the functionality of your algorithm (and that's a good idea!), create test cases. You may want to read about JUnit, being a better alternative to main() methods for testing purposes.

Documentation / comments

// frames a string that contains newlines
// does not work for strings that contain symbols that need more than one
// space unit, for example tabs
public static String makeBorderString(String value) {
    String[] lines = value.split("\\r?\\n");
    return makeBorderStringFromArray(lines);
}

In your comment, the first line is important, as it documents the task of this method. This should become the summary in a Javadoc-formatted documentation comment (your IDE can create a template "with a single click"). The rest states a limitation. It should become part of the Javadoc as well (until you find a solution).

If, on your roadmap, you plan to improve the method to support e.g. tab characters as well, a convention is to write TODO comments:

// TODO find a solution for tab characters

Your IDE understands this as some future task.

If you feel tempted to write a comment in the middle of a method, take that as a hint that the following part might benefit from becoming a well-named method of its own, e.g.

    // make upper Border border
    sb.append("+" + repeatSymbol('-', mostLong + 2) + "+\n");

could then become

    addTopBorder(sb, mostLong);

Implementation hints

The code for the body can be simplified:

    // make body
    for (int i = 0; i < stringList.size(); i++) {
        String current = stringList.get(i);
        int whiteSpaceAmount = mostLong - current.length();
        String whitespace = repeatSymbol(' ', whiteSpaceAmount);
        sb.append("| " + current + whitespace + " |\n");
    }

The only reason why this won't work with your current code out of the box is that the repeatSymbol() method returns null in the zero-count case instead of an empty String "". You should change that, as it also makes repeatSymbol() simpler, no longer needing the conditional:

public static String repeatSymbol(char symbol, int times) {
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < times; i++) {
        sb.append(symbol);
    }
    return sb.toString();
}

A loop like

    for (int i = 0; i < stringList.size(); i++) {
        String current = stringList.get(i);
        // ...
    }

can be rewritten as:

    for (String current : stringList) {
        // ...
    }

Visibility

You made all methods public. For the three makeBorderStringXYZ() methods, this can be a valid decision, if you want to offer the framing algorithm for a variety of multi-line text representations. But then, the array and list version should get a documentation stating that the elements must represent single lines and not contain line break characters.

I don't see a reason to make the repeatSymbol() method public. It's just meant to be an aid for the framing algorithm. If you instead want it to become a user-available string-manipulation method, place it into a StringUtils class (or similar).

By default, make everything private, unless you really want foreign code to use it.

Array declaration style

In

public static String makeBorderStringFromArray(String stringArray[]) {

you use the C-style array declaration (which is frowned-upon in the Java world). Java developers prefer

public static String makeBorderStringFromArray(String[] stringArray) {

which more consistently declares that stringArray is a parameter with type String[]. (In the C language, there is some reason to write it the other way round, but that doesn't apply to Java - but alas, the original Java language designers allowed for that style as well.)

Answer 2

A couple of points beyond Ralf's excellent suggestions.

---

JDK 15 has introduced multiline strings. Instead of

        String string = "one\ntwo three\nfour\nlet your dreams come true\n";
        string += "nothing is impossible";

You can write:

        String string = """
                        one
                        two three
                        four
                        let your dreams come true
                        nothing is impossible""";

---

You do not need to name your methods with the argument types they expect. If the methods perform the same function, name them the same thing despite different arguments:

    public static String makeBorder(String value) {
        String[] lines = value.split("\\r?\\n");
        return makeBorder(lines);
    }

    public static String makeBorder(String[] stringArray) {
        List<String> stringList = Arrays.asList(stringArray);
        return makeBorder(stringList);
    }

    public static String makeBorder(List<String> stringList) {
        ...
    }

---

You can preallocate your StringBuilder for efficiency. You know how long each line is, and how many lines you have.

        StringBuilder sb = new StringBuilder((mostLong + 5) * (stringList.size() + 2));

---

You can avoid creating extra StringBuilder objects by passing your existing sb to the repeatSymbol method.

    public static String repeatSymbol(StringBuilder sb, char symbol, int times) {
        for (int i = 0; i < times; i++) {
            sb.append(symbol);
        }
    }

This does mean you'd have to break up some append operations into multiple lines. For example

        sb.append("+" + repeatSymbol('-', mostLong + 2) + "+\n");

would become:

        sb.append('+');
        repeatSymbol(sb, '-', mostLong + 2);
        sb.append("+\n");

but would again be more efficient, as it would not need to create and discard as many temporary objects.

---

There isn't much difference between these two lines:

+---------------------------+
| nothing is impossible     |

The first can be thought to have a prefix of "+-", a suffix of "-+\n", no content, and a "fill" character of '-', where as the second could have a prefix of "| ", a suffix of " |\n", the content "nothing is impossible", and a fill character ' '.

With this in mind we can write a frame line function:

    private static void frameLine(StringBuilder sb, String prefix, String suffix, String content,
                                  char fill, int width) {
        int repeat = width - content.length();

        sb.append(prefix).append(content);
        for (int i=0; i<repeat; i++) {
            sb.append(fill);
        }
        sb.append(suffix);
    }

Your main makeBorder method could then become:

    public static String makeBorder(List<String> lines) {

        int max_line = lines.stream().mapToInt(String::length).max().orElse(0);
        StringBuilder sb = new StringBuilder((max_line + 5) * (lines.size() + 2));

        frameLine(sb, "+-", "-+", "", '-', max_line);
        for(String line: lines) {
            frameLine(sb, "| ", " |\n", line, ' ', max_line);
        }
        frameLine(sb, "+-", "-+", "", '-', max_line);

        return sb.toString();
    }

Answer 3

Try to think of the types of things that your program deals with, and define those things as their own class of information. That way, you aren't just defining a bunch of static methods that happen to live in the same file, but you're actually defining something with meaning to the user. Instead of a StringFramer, consider a FramedString.

The data captured would be the lines and we'd only need to ever calculate the width once. Just like a String it would be immutable - if you need to frame a different String, just create a new FramedString.

import java.util.Arrays;
import java.util.List;

public class FramedString {
    private final String[] lines;
    private final int contentWidth;

    /**
     * Creates a new instance from a String which may be broken with newlines ("\n")
     * and is not expected to contain tabs.
     */
    public FramedString(String string) {
        this.lines = string.split("\n");
        this.contentWidth = widthOfLongest();
    }

    /**
     * Creates a new instance from a String[] which is not expected to contain
     * newlines ("\n") or tabs.
     */
    public FramedString(String[] lines) {
        this.lines = lines;
        this.contentWidth = widthOfLongest();
    }

    /**
     * Creates a new instance from a List<String> which is not expected to contain
     * newlines ("\n") or tabs.
     */
    public FramedString(List<String> lines) {
        this.lines = (String[]) lines.toArray();
        this.contentWidth = widthOfLongest();
    }

    private int widthOfLongest() {
        int width = 0;
        for (String line : lines) {
            int length = line.length();
            if (length > width) {
                width = length;
            }
        }
        return width;
    }

    /**
     * Returns the content of this object pretty printed with a frame.
     * 
     * <pre>
     * +-----------------------+
     * | one                   |
     * | two three             |
     * | four                  |
     * | your dreams come true |
     * | nothing is impossible |
     * +-----------------------+
     * </pre>
     */
    public String toString() {
        StringBuilder out = new StringBuilder((lines.length + 2) * (contentWidth + 5));

        out.append("+-");
        for (int i = 0; i < contentWidth; i++) {
            out.append("-");
        }
        out.append("-+");

        for (String line : lines) {
            out.append("\n");
            out.append(String.format("| %-" + contentWidth + "s |", line));
        }

        out.append("\n");
        out.append("+-");
        for (int i = 0; i < contentWidth; i++) {
            out.append("-");
        }
        out.append("-+");

        return out.toString();
    }

    public static void main(String[] args) {
        final String[] example = {
                "one",
                "two three",
                "four",
                "your dreams come true",
                "nothing is impossible" };
        System.out.println(new FramedString(String.join("\n", example)));
        System.out.println(new FramedString(example));
        System.out.println(new FramedString(Arrays.asList(example)));
    }
}

Answer 4

Just some additional tips, not an answer as such.

1. Regex for line break:

    value.split("\\r?\\n");
   

   can be done more abstract & more complete.

    value.split("\\R");
   

2. You might want to try graphic lines:

    static final char TOP_LEFT = '\u250C';
    static final char HORIZONTAL = '\u2500';
    static final char TOP_RIGHT = '\u2510';
    static final char VERTICAL = '\u2502';
    static final char BOTTOM_LEFT = '\u2514';
    static final char BOTTOM_RIGHT = '\u2518';
   

   You could also use the standard Unicode name of the char, something like:

    static final char TOP_LEFT =
        (char)Character.codePointOf("BOX DRAWINGS LIGHT HORIZONTAL"):
   

   But this seems a bit overdone. You would not even want to use the Unicode standard names as java constant name: BOX_DRAWINGS_LIGHT_HORIZONTAL - too long.

3. Varargs would add another usage.

    public FramedString(String[] lines) {
   

   could be

    public FramedString(String... lines) {
   

   Also allowing:

    new FramedString("one cat has one tail more than no cat",
                     "no cat has nine tails",
                     "so one cat has ten tails");
   

4. Special text

   The tab char \t is a special case.

   Then accented characters like ñ (Spanish mañana) and ç (French façade) can be written as 1 char (the accented char) or two chars (basic latin plus zero-width mark). Java has a Unicode text normalizer:

    current = Normalizer.normalize(current, Normalizer.Form.NFKC);
   

   Which converts any string to its composed form.

Answer 5

Adding to existing answers that I mostly agree with...

Don't use C-style array declarations. (Others have all rewritten your arrays in the standard Java way but they didn't explicitly point this out.)

public FramedString(String[] lines)

Use the Stream APIs to simplify, name variables what they represent, not their type:

    int mostLong = 0;
    for (int i = 0; i < stringList.size(); i++) {
        if (stringList.get(i).length() > mostLong) {
            mostLong = stringList.get(i).length();
        }
    }

becomes:

    // find the length of the longest line
    int longest = lines.stream().mapToInt(String::length).max().getAsInt();

Java 11 has added a repeat(int count) method to String.

Never use string concatenation with the argument to StringBuilder.append. That's unnecessary nested concatenation. This:

sb.append("+" + repeatSymbol('-', mostLong + 2) + "+\n");

Is roughly equivalent to:

sb.append(new StringBuilder().append("+").append(repeatSymbol('-', mostLong + 2)).append("+\n"));

When appending a single character, pass it as a char instead of a String.

The lower border is the same as the upper border. You could reuse it, though I'm not sure if it is worth it.

Use for-each loops. Values that aren't reused don't need to be assigned to variables, except perhaps while debugging or if it significantly improves readability.

Combining the above hints we get:

public static String makeBorderString(List<String> lines) {
    // find the length of the longest line
    int longest = lines.stream().mapToInt(String::length).max().getAsInt();
    
    StringBuilder sb = new StringBuilder((longest+5)*(lines.size()+2));
    
    // make upper border
    sb.append("+-").append("-".repeat(longest)).append("-+\n");
    
    // make body
    for (String line : lines) {
        sb.append("| ").append(line)
                .append(" ".repeat(longest - line.length()))
                .append(" |\n");
    }
    
    // make lower border
    sb.append(sb.subSequence(0, longest+5));
    
    return sb.toString();
}