Reading columns of a delimited text file

Question

This class reads a delimited text file into String arrays on demand. Delimiter can be any regex.

Features include:

1. Read a single column

2. Read a range of columns

3. Read arbitrary columns by providing an array of indices, e.g. for columns 1, 3, 5, use new int[]{1, 3, 5}

import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;
import java.util.function.Supplier;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
 * Created by IDEA on 17/11/14.
 */
public class Txt {
    final private String sep;
    final private Pattern pattern;
    final private String filename;
    private Supplier<Stream<String>> fileStream;
    Txt(String _filename, String _sep) throws IOException {
        sep = _sep;
        filename = _filename;
        pattern = Pattern.compile(sep);
        fileStream = () -> {
            try {
                return Files.lines(Paths.get(filename), Charset.defaultCharset());
            } catch (IOException e) {
                e.printStackTrace();
                return null;
            }
        };
    }
    Txt(String _filename) throws IOException {
        this(_filename, "\\s+");
    }


    public String[] nthCol(int n) throws IOException {
        if(n < 1) n = 1;
        final int finalN = n;
        List<String> colList =  fileStream.get()
                .map(line -> pattern.split(line, finalN + 1)[finalN - 1])
                .collect(Collectors.toList());
        return colList.toArray(new String[colList.size()]);
    }

    public String[][] cols() throws IOException {
        List<String[]> colsList =  fileStream.get()
                .map(line -> pattern.split(line))
                .collect(Collectors.toList());
        return colsList.toArray(new String[colsList.size()][]);
    }

    public String[][] cols(final int start, final int end) {
        if(start > end || start < 1) throw new IllegalArgumentException("Starting index must <= ending index AND >= 1");
        List<String[]> colsList =  fileStream.get()
                .map(line -> {
                    String[] fields = pattern.split(line, end + 1);
                    // indices of col 2-4          2 3 4
                    // fields after splitting    0 1 2 3 4
                    // range of array              1     4
                    // java ending index is              exclusive
                    String[] demandedFields = Arrays.copyOfRange(fields, start-1, end);
                    return demandedFields;
                })
                .collect(Collectors.toList());
        return colsList.toArray(new String[colsList.size()][]);
    }

    public String[][] cols(int[] index) {
        Arrays.sort(index);
        final int[] finalIndex = index;
        final int start = finalIndex[0];
        final int end = finalIndex[finalIndex.length - 1];
        if(start > end || start < 1) throw new IllegalArgumentException("Starting index must <= ending index AND >= 1");
        List<String[]> colsList =  fileStream.get()
                .map(line -> {
                    String[] fields = pattern.split(line, end + 1);
                    String[] demandedFields = new String[finalIndex.length];
                    for(int i=0; i<finalIndex.length; i++) {
                        demandedFields[i] = fields[finalIndex[i]-1];
                    }
                    return demandedFields;
                })
                .collect(Collectors.toList());

        return colsList.toArray(new String[colsList.size()][]);
    }
}

Answer 1

General Remarks

The code checks incoming parameters, and throws an IllegalArgumentException when they are out of bounds (except nthCol; see later). This is a good thing. It would be even better if you could include the faulty indices in the error message so that I don't have to pop out the debugger. ;-)

The class seems to work with 1-based indices rather than 0-based, the latter being more common in Java^[1]. As you commented about this within your code (but not in your public documentation), you've felt there's a mismatch; it may be better to use 0-based indices for consistency with the rest of the JDK.

You're going through hoops to work with arrays in the public interface while using streams and collections behind the scenes. Consider returning lists instead of arrays, or maybe even streams if you're so inclined.

Constructors

Character encodings are important, too important to force the default encoding on your users, for any definition of 'default encoding'. Many of the classes and methods deprecated in java.io were so because they assumed about encodings, which I've seen lead to subtle but no less spectacular breakage.^[2] Allow/force your users to specify the encoding. If they make it be this fangled default encoding, then on their heads be it. (A simular argument could be made for Paths.get.)

For your class, it doesn't matter how it gets its stream of strings, as long as it gets it. Make this clear in your constructor:

Txt(Supplier<? extends Stream<String>> stream, Pattern delimiter) {
    this.stream = stream;
    this.delimiter = delimiter;
}

No fuss about encodings, file paths, IOExceptions, invalid patterns... Just the bare needs to function. Any other constructors or static factory methods you desire are then for convenience, and should delegate to this one.

Specifics

nthCol(int) breaks with the other methods in that it tries to fix an erroneous input parameter. This leads to the oddness of nthCol(0) succeeding but cols(0, 0) failing. Consistency is king in library usage: nthCol should fail, or cols should succeed.^[3]

[!] cols(int[]) changes the contents of the incoming array. You don't really need to sort this array, anyway: I'd consider it a feature if the columns returned are in the same order that I requested them.

Txt is not a very descriptive name. ;-) Consider names such as LineSplitter or TextTable.

---

^{[1] Oh, how sweetly I loathe you, java.sql, for your 1-based indexing.}

^{[2] Fixed record length + multi-byte encoding + JNI. It was glorious.}

^{[3] Personally, I lean towards "fail early, fail hard," (i.e. throw) but this depends on your intended audience.}

Answer 2

A number of smaller issues stand out:

• you don't actually use final private String sep; anywhere. this variable can be removed. (you use _sep only inside the constructor)

• There doesn't seem to be a clear need to use a lambda expression to set the filestream variable. You actually introduce an extra type (Supplier<...>) to be able to use lambda.

• You don't actually handle the exceptional situation inside the lambda:

What if fileStream ends up being null ?

fileStream = () -> {
        try {
            return Files.lines(Paths.get(filename), Charset.defaultCharset());
        } catch (IOException e) {
            e.printStackTrace();
            return null;
        }
    };

Then you use it later on:

List<String[]> colsList =  fileStream.get()

I can see a use for lambda expressions in general if they are an argument to a method (so a method can more easily be reused with different actions, like iteration or other circumstances.)

However the benefit in using them in this occasion evades me:

fileStream = () -> {
        try {
            return Files.lines(Paths.get(filename), Charset.defaultCharset());
        } catch (IOException e) {
            e.printStackTrace();
            return null;
        }
    };

versus

try {
    fileStream = Files.lines(Paths.get(filename), Charset.defaultCharset());
} catch (IOException e) {
    e.printStackTrace();
}

Perhaps you or another reviewer can point out to me what the benefit is of using lambda in this situation. I just see more room for errors.

The additional problem is in my opinion: what if you want to throw a checked exception when you catch an IOException? As far as I know you cannot throw a checked exception from inside a lambda block that doesn't throw it. (see this question)

So what if somewhere down the line you want to throw this exception or a similar one? You would either have to rewrite it to not use lambda expressions, or throw an unchecked exception, catch and rethrow as checked.

To me this seems really wrong. Lamdba is very good for specific purposes but not for convenience methods. Try to keep them simple and to the point and make the chance that anyone will want to throw an exception to a minimum. Or handle the actual exception

When you write this the first time, none of this will be an issue but during the maintenance lifecycle this will eventually cause someone to swear. anyways this is 2 months after I decided against raging against lambda's :D they're not bad but I don't recommend using them for the sake of using lambda's.

• Agree with @JvR, don't force encodings onto users. It's better to have them individually chosen. Having said that, why anyone would use non-utf-8 is beyond me.

anyways no further time I may come back and put more in. but since you asked I added my anti-lambda bit in there, including the exception part as best as I remembered it at the time