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.