4
\$\begingroup\$

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:
enter image description here
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.

\$\endgroup\$
6
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You've lost information here. A non-existent file, and an empty file both will result in an empty Stream<>, which would result in an empty List<>. The first should be an error, where as the second case may be valid; the caller has no way of distinguishing the two. \$\endgroup\$ – AJNeufeld Apr 5 at 0:22
  • \$\begingroup\$ I wrote them in caps to signal that they're constants. And how else should I seperate words if not with "_" then? Also I didn't use them class-wide, because this class should read graph-files from multiple sources, not only this format. \$\endgroup\$ – Morinator Apr 5 at 0:52
  • \$\begingroup\$ @Morinator, for the constants, generally we use the static and final keywords combined. In this example, the variable will be recreated each time you call the method; in my opinion, this is not a constant. For the word separation, you can separate words with camelCase by using the upper case as the word separator. LINE_DATA_FORMAT = lineDataFormat :) \$\endgroup\$ – Doi9t Apr 5 at 1:21
  • \$\begingroup\$ Ok, did that now. But the runtime in this part of the program is pretty irrelevant anyways. What my program does outside this reader is NP-hard, so this won't be a bottleneck :D \$\endgroup\$ – Morinator Apr 5 at 1:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.