Usage of scanner

Question

I have a legacy method which I would like to clean up:

public void readFile(String filename) {
    int i;
    int j;
    String ins = new String();
    SimpleDateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'");

    try (FileReader input = new FileReader(filename);
            BufferedReader bufInput = new BufferedReader(input);
            Scanner scHeader = new Scanner(bufInput.readLine())) {

        scHeader.next(); // String creationTime
        scHeader.next(); // String idField
        int numRecs = scHeader.nextInt();
        scHeader.next(); // String star
        scHeader.next(); // String comment

        Scanner[] sc = new Scanner[4];

        for (i = 0; i < numRecs; i++) {
            for (j = 0; j < 4; j++) {
                sc[j] = new Scanner(bufInput.readLine());
                if (sc[j] == null) {
                    throw new IOException("No Data Block Record found");
                }
            }
            String keyId = sc[0].next();
            String validityStart = sc[0].next();
            String saa = new String();
            if (sc[0].hasNext()) {
                saa = sc[0].next();
            } else {
                saa = "-999";
            }
            DataSet dataset = new DataSet();
            MetaData meta = dataset.getMeta();
            double[][] ang = new double[3][3];
            ArrayData siam = new Double2d(ang);


            meta.set("instrument", new StringParameter(ins, "Instrument"));
            meta.set("aperture", new StringParameter(keyId, "Aperture identifier"));
            meta.set("validityStart", new DateParameter(new FineTime(df.parse(validityStart)),
                    "Validity start time of instrument calibration"));
            meta.set("SAA", new DoubleParameter(new Double(saa), "Reference solar aspect angle"));

            for (j = 0; j < 3; j++) {
                // Java doesn't seem to like the 'D' as the exponent identifier; replace with an 'E'
                ang[j][0] = Double.parseDouble(sc[j + 1].next().replace('D', 'E'));
                ang[j][1] = Double.parseDouble(sc[j + 1].next().replace('D', 'E'));
                ang[j][2] = Double.parseDouble(sc[j + 1].next().replace('D', 'E'));
            }

            siamDataset.setData(siam);
            getSets().put(keyId, dataset);

        }
    } catch (Exception e) {
        e.printStackTrace();
    }

    return;
}

It is a bit of mess in terms of readability and maintenance so I wanted to clean it up. It seems to me the Scanner[] is a little strange? Basically each block in the file is made up on 4 lines so I thought it could be better to do it that way rather than create these arrays.

I even wondered if there could be a nice concise solution using java 8?

Answer 1

A few things for the current code and some pointers on rewriting this with a Stream-based solution:

Declare variables as close as possible to their usage!

Declaring variables at the top of a function / procedure / ... was necessary in the olden days, when IDEs were rare and far between and most of these IDEs weren't able to jump to the declaration of a variable.

In these days declaring variables at the top is unnecessary or even harmful. I have been debugging code where the first 20 lines (if you were lucky it was just 20) were only variable declarations and initializations.

Debugging that is a nightmare. Additionally it's more or less clutter until you use the variables.

Did I mention it's harder to find unused variables then? Well ins is not used, at least not in this method.

java.io.File is replaced with java.nio.Path

This makes your try-with-resources (kudos on that btw.) a little shorter:

    try (BufferedReader bufInput = Files.newBufferedReader(Paths.get(filename), Charset.forName("UTF-8"));
        Scanner scHeader = new Scanner(bufInput.readLine())) {

Precomputed size is so 2003

Keeping precomputed size with your data to allocate the necessary memory is only useful in: Memory-constrained environments, Time-constrained environments.

For all other intents and purposes you seem to be able to just skip the first line and use a dynamically allocated collection until you're finished reading your file.

This is a good moment to talk about streams...

Java 8 possibilites

Java 8 introduced Files.lines() which gets you a stream of all lines. Remember how you need the first line only to tell you how many records there are?

For now I assume the file only contains that header and some records of the same type. Consider the following:

try (Stream<String> records = Files.lines(Paths.get(filename))) {
    records.skip(1); // skip header

unfortunately this is already where the problems start. As I understood your code, each "record" takes up 4 lines instead of 1. Unfortunately Java does not (yet?) support partitioning a Stream natively. So this is basically where this review ends.

The linked SO question talks about partitioning a Stream by a Function (which isn't necessarily what you need). In general you will have to create your own solution if you want to work with Streams right now.

It's not impossible, but I'd advise against it and stick with an improved version of the current code.

OBTW this makes the "drop the precomputed size" recommendation rather... problematic, since the format of data requires you to rely on that information :/

Answer 2

Comment for method name

Take this piece of code :

// Java doesn't seem to like the 'D' as the exponent identifier; replace with an 'E'
ang[j][0] = Double.parseDouble(sc[j + 1].next().replace('D', 'E'));
ang[j][1] = Double.parseDouble(sc[j + 1].next().replace('D', 'E'));
ang[j][2] = Double.parseDouble(sc[j + 1].next().replace('D', 'E'));

The comment there is a perfect example of what could have a been a good method name dto document and remove duplicate code. You could have a method replaceExponentIdentifier that just do the replace on the String. The comment would disappear and you would have only one place to do the replace.

Unused return

You have a return; at the end of your method after the catch, which is probably not useful since if you reach that point you will exit the method anyway.

Be precise

This is no big deal but being precise with what you throw as exception can help people using your code or app. You have throw new IOException("No Data Block Record found"); which have a clear message but IOException is a bit generic for this. You have precise case here, you know exactly what has gone wrong. I would use a custom exception name something like DataBlockNotFound or something similar.