3
\$\begingroup\$

I have this very simple code which is used to parse values:

public class Connections
{
    public List<Integer> getConnections() throws IOException
    {
        byte[] fileBytes;
        File myFile = new File("/proc/net/sockstat");
        if (myFile.exists())
        {
            try
            {
                fileBytes = Files.readAllBytes(myFile.toPath());
            }
            catch (java.nio.file.AccessDeniedException e)
            {
                return null;
            }

            if (fileBytes.length > 0)
            {
                String doc = new String(fileBytes);

                Pattern p = Pattern.compile("-?\\d+");
                Matcher m = p.matcher(doc);

                List<Integer> list = new ArrayList<>();
                while (m.find())
                {
                    list.add(Integer.valueOf(m.group()));
                }


                return list;
            }
        }
        return null;
    }

}

This code works well but it's using too many slow operations to get the values. Is there a way to improve the code? I would like to use Java 8.

\$\endgroup\$
  • 2
    \$\begingroup\$ The title of your post should be the function/purpose of your code. \$\endgroup\$ – SirPython Feb 9 '15 at 21:46
6
\$\begingroup\$

This code works well but it's using too many slow operations to get the values.

Not sure what really bugs you:

  • You read the entire file with a single Files.readAllBytes. Doesn't get faster than that. If the file is huge, I imagine this could take time, but if you want the entire file, there's just no way around it
  • You convert the byte[] to a String so that you can use a Pattern + Matcher. The alternative I see would be parsing the digits by yourself, but that wouldn't be as optimized as the built-in regex engine, so you won't be better off
  • I imagine the number parsing and Integer object creation might cost you a couple of extra cycles. But I don't see an alternative to make that significantly faster

My point is, if you want to read and parse the entire file, and store the result in a List, then I don't think it can get much faster than this.

Is there a way to improve the code?

When a method is expected to return a List or other collection type, it's good to avoid returning null values, as that forces the caller to add null checks. A more ergonomic solution is to return an empty list, for example Collections.emptyList().

Code is usually easier to read when not so deeply nested. The majority of the method is nested within a if (myFile.exists()) { ... } block. If you flip this condition and return early, you can reduce the nesting level.

It's better to declare variables in the most limited scope where they are used. In this example the byte[] fileBytes is only used if the file exists, so you could declare it later.

Putting these tips together, the method body becomes:

List<Integer> list = new ArrayList<>();
File myFile = new File("/proc/net/sockstat");
if (!myFile.exists()) {
    return list;
}

byte[] fileBytes;
try {
    fileBytes = Files.readAllBytes(myFile.toPath());
} catch (java.nio.file.AccessDeniedException e) {
    return list;
}

if (fileBytes.length <= 0) {
    return list;
}

String doc = new String(fileBytes);

Pattern p = Pattern.compile("-?\\d+");
Matcher m = p.matcher(doc);

while (m.find()) {
    list.add(Integer.valueOf(m.group()));
}

return list;

Also, the convention is to put opening braces on the same line as their statement, not on the next line as you did (C# style).

Finally, the variable names are kind of fishy. Single letter names like p and m are considered a bad practice, and doc doesn't seem to give a clue about what this variable is about.

Alternative approach

Although as I wrote above, if you want to read and parse a large file it's bound to be slow, but there's a different approach that might still help.

Instead of reading the entire file at once (time-consuming), you could read part of the file and return results as you go. You could do this using the iterator pattern:

  • Create a class that implements the Iterator interface
  • Use an internal buffer to store the next chunk of the file
  • When hasNext is called, check if there is still data left in the buffer, or read in the next chunk
  • When next is called, return the next integer

This approach has the following advantages over the original:

  • Instead of waiting for the entire file to be processed, users of the iterator can start processing sooner. That is, when you read the entire file at once, it may seem to the user that the program freezes for a couple of seconds. If you process chunk by chunk, this doesn't happen
  • If the user of the iterator stops in the middle, the rest of the file won't be read and parsed unnecessarily
  • Since the integers don't need to be stored in a list anymore, you can reduce the memory use dramatically

Potential disadvantages:

  • More work to implement. (Or, you could play with a Scanner)
  • If you always need to read in the entire file, then it will be more efficient to do it once and be done with it, like the original code
\$\endgroup\$

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.