Table printing code using a fluent interface

Let's say you want to display something like:

One Two Three Four 
1   2   3     4

And since you hate the idea of predefining the widths of the columns you want to print it with something like this:

public static void main(String[] args) {
    new Columns()
       .addLine("One", "Two", "Three", "Four")
       .addLine("1", "2", "3", "4")
       .print()
       ;
}

Either listing below can do this. Which is easier to read? How would you improve either of them? Do either paint us into a corner? Any multithreaded concerns? How could it be made extensible? Would the getThis() trick help here or is it a bad idea?

Listing A

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

public class Columns {

    List> lines = new ArrayList<>();
    List maxLengths = new ArrayList<>();
    int numColumns = -1;

    public Columns addLine(String... line) {

        if (numColumns == -1){
            numColumns = line.length;
            for(int i = 0; i  line : lines) {
            for(int i = 0; i < numColumns; i++) {
                result += pad( line.get(i), maxLengths.get(i) + 1 );                
            }
            result += System.lineSeparator();
        }
        return result;
    }

    private String pad(String word, int newLength){
        while (word.length() < newLength) {
            word += " ";            
        }       
        return word;
    }
}

Listing B

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

public class Columns {

List> lines = new ArrayList<>();
List maxLengths = new ArrayList<>();
int numColumns = -1;
String[] line;

public Columns addLine(String... line) {

this.line = line;
learnNumOfColumnsOnce();
ensureConsistantNumOfColumns();
widenColumnsThatNeedIt();
rememberLine();
return this;
}

private void learnNumOfColumnsOnce() {
if (numColumns == -1){

Which is easier to read? How would you improve either of them? Do either paint us into a corner?

I love A. It is simple, clear, concise and meaningful. B is the same as A but with one major difference: the method addLine was cut into several small pieces.

In my opinion, the issue with B is that it extracts from one meaningful method (adding a line) a lot of small awkward methods, and in the process completely obscuring the real task at play: none of those methods are really reusable and probably none of those methods would make sense in a context other than addLine (think removeLine, withColor, withFont...).

Think back and focus on what you want the code to do. We have addLine and we expect it to add a line to our columns. Clearly, there are several code paths to deal with: notably, the first case is to determine the length of each line, etc. But a method can have 30 lines of code without it doing too much work, because it wouldn't make sense to cut that work in pieces: they are all part of the same unit of work. All of the pieces are needed to make the lot work, and they need to be called and assembled in a specific way that is unique to adding a line. In that sense, having multiple methods may be fragile because they are coupled to each-other.

Of course, if that work would require lots of lines of code, you can move part of in a dedicated method. Additionally, work that requires few lines of code could also be extracted in multiple utility methods.

The point is to find a balance between small, easy to read methods and logical unit of work. Part of it can also rely on opinions — both approaches are correct and none of them are fundamentally bad.


Any multithreaded concerns?

Both approaches won't work in a multi-threaded environment because Columns isn't thread-safe: it has state, for example with List> lines and List maxLengths. One possible way to make it thread-safe would be to return a new Columns instance each time addLine is called (for that, we can add a private constructor taking lines, maxLengths and numColumns), and making Columns immutable in the process (do not update the fields of this but use the new values to construct the new instance and return it).


How could it be made extensible?

It is already extensible enough. The data structures for Columns are kept hidden; I can see a removeLine, withColor (setting a color for each column), withFont (setting a font for each column) added without refactoring the current code.


Would the getThis() trick help here or is it a bad idea?

Do you really intend to have classes inhering from Columns? Keep it simple, you most likely aren't going to need it.

Other comments on the code, applicable to both examples:

-
You are throwing an IllegalArgumentException in case a line with a different length is added:

throw new IllegalArgumentException();

That's great, but consider adding a message so that it is clearer for the user what went wrong. IllegalArgumentException with no message is not really helpful.

-
Both in toString and in pad, you are concatenating String with the += operator. This can lean to serious performance issues. When used inside a loop, you should always use a StringBuilder (or a StringJoiner).

StackExchange Code Review Q#136353, answer score: 3