Reader for graph-file with ugly return and questionable use of streams

Question

I wrote a class that contains a static method that reads in files from NetworkRepository and converts them into a list of integer-arrays, each representing an edge. The file format from NetworkRepository is, that each line which is not a comment contains two integer numbers, the IDs of vertices connected by an edge, seperated by whitespace (tabs/spacebar).
Example:

corresponds to:

1 2  
1 3  
2 3  
3 4  

Can my code for this be improved? I really don't like the unreachable return null, but I don't need error-handling except that I know I provided the wrong path.

public static List<Integer[]> edges_from_path(final String filePath) {
    //captures only lines that consist of two positive integer numbers separated by whitespace (each line encodes an edge)
    final String LINE_DATA_FORMAT = "\\d+\\s+\\d+";
    final String SEPARATOR = "\\s+";
    try {
        return Files.lines(Paths.get(filePath))
                .filter(str -> str.matches(LINE_DATA_FORMAT))
                .map(str -> Arrays.stream(str.split(SEPARATOR)).map(Integer::parseInt).toArray(Integer[]::new))
                .collect(toList());
    } catch (IOException e) {
        e.printStackTrace();
        return null;
    }
}

My method doesn't return a graph because I want to be able to create graphs from different sources, and a list of integer-pairs is the interface the constructor for graphs is supposed to use.

Answer 1

Exception Handling

What are you doing in the exceptional case?

Printing an error message (ugly stack trace, actually) and returning null. Now the caller has to handle the problem of the function returning null.

You've said:

I don't need error-handling except that I know I provided the wrong path.

Well, want is the caller doing when it gets the null? Does it say, "I couldn't open that file, please enter a different filename"? That too is error handling, and you've have it in two places: the try ... catch here, and the if (list == null) by the caller.

Better is to ignore the exception here, and let the caller handle it, or the caller's caller.

public static List<Integer[]> edges_from_path(final String filePath) throws IOException {
    //captures only lines that consist of two positive integer numbers separated by whitespace (each line encodes an edge)
    final String LINE_DATA_FORMAT = "\\d+\\s+\\d+";
    final String SEPARATOR = "\\s+";
    return Files.lines(Paths.get(filePath))
                .filter(str -> str.matches(LINE_DATA_FORMAT))
                .map(str -> Arrays.stream(str.split(SEPARATOR)).map(Integer::parseInt).toArray(Integer[]::new))
                .collect(toList());
}

My method doesn't return a graph because I want to be able to create graphs from different sources, and a list of integer-pairs is the interface the constructor for graphs is supposed to use.

This is even more reason not to catch the error and return null; the other sources would need to have the same behaviour. They could fail in different and diverse ways; the exception will provide more details about the failure than null return value will.

List of Integer-Pairs

To say I almost threw-up when I saw List<Integer[]> would be an exaggeration, but only slightly. Mixing collections and raw arrays is cringe worthy. But a list of a raw arrays of a boxed type??? Yikes!

The raison d'être for "boxed types" is so they can be placed into the various Collection<E> containers, which require objects instead of raw types. But here, you're not putting the integers into a Collection<E>; you're putting them into an array ([]), so they don't actually need to be boxed.

To see how wrong this is, each element of the List<> is an object Integer[], which holds two references to two Integer objects. That is 3 objects allocated in the heap (ignoring interning) for every edge in the list.

If instead you used List<int[]>, then element of the List<> would be an int[] object, which directly contains the two integers, instead of references. No extra objects need to be allocated. Less memory is required, and the data is faster to access because an extra level of memory indirection has been removed.

Still, we eschew lists of raw arrays. They are inconsistent; you need different access methods to get to the data at different levels, such as list.get(idx1)[idx2]. The array of integers doesn't convey any requirement on the number of values; you could be given a lists of lengths other than two. It would be better to wrap the edge information into a POD (plain old data) object:

class Edge {
    public final int from;
    public final int to;
    Edge(int _from, int _to) {
        from = _from;
        to = _to;
    }
}

And then you could return a List<Edge>. Change your graph constructor from taking a list of arrays of boxed integers of arbitrary lengths to taking a list of edges. That is a much clearer interface contract.

Answer 2

Since you are handling the file in the same method of the extraction, you have to deal with the exception.

I suggest that you separate the logic of the file reading and the logic of the parsing in two separate methods, so the method that handles the file will handle the exception.

public static Stream<String> readFileAsLines(final String filePath) {
   try {
      return Files.lines(Paths.get(filePath));
   } catch (IOException e) {
      return Stream.empty();
   }
}

public static List<Integer[]> edges_from_path(Stream<String> lines) {
   //captures only lines that consist of two positive integer numbers separated by whitespace (each line encodes an edge)
   final String LINE_DATA_FORMAT = "\\d+\\s+\\d+";
   final String SEPARATOR = "\\s+";

   return lines.filter(str -> str.matches(LINE_DATA_FORMAT))
          .map(str -> Arrays.stream(str.split(SEPARATOR)).map(Integer::parseInt).toArray(Integer[]::new))
          .collect(toList());
}

Bonus

The LINE_DATA_FORMAT & SEPARATOR names are wrong, they should be in camelCase, not snake_case; But they should be class constants, in my opinion.