3
\$\begingroup\$

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;
    }
}
\$\endgroup\$

5 Answers 5

6
\$\begingroup\$

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.)

\$\endgroup\$
3
\$\begingroup\$

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();
    }
\$\endgroup\$
3
\$\begingroup\$

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)));
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Computing the format string "| %-" + contentWidth + "s |", a constant, each time through the loop is inefficient. FramedString has all final instance members, so it can never change. Therefore, the result of toString() will always be the same, so it could be computed once and stored during construction, in which case storing lines and contentWidth is just wasted space. However, this turns the class into a glorified String constant, with no public methods other than toString(), begging the question “why maintain a FramedString instance?” static methods sufficed here. \$\endgroup\$
    – AJNeufeld
    Mar 23, 2022 at 5:34
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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();
}
\$\endgroup\$

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.