Parsing a tabular log file into a hashmap

Question

I have this method that reads a file into a hashmap. I feel like there are too many levels in this code with all these tries, ifs and so on. How do you think this can be improved?

public static ExchangeHistory loadHistory(String path){
    Map<Integer, Date> rtcHistory = new HashMap<Integer, Date>();
    Map<Integer, Date> almHistory = new HashMap<Integer, Date>();

    ExchangeHistory exchHist = new ExchangeHistory(rtcHistory, almHistory);

    File file = new File(path);

    BufferedReader reader = null;
    try{
        reader = new BufferedReader(new InputStreamReader(new FileInputStream(file)));

        String line;
        int lineNumber = 1;

        // read file line by line
        while ((line = reader.readLine()) != null){
            String[] tokens = line.split("\\s");
            if (tokens.length != 3){
                log.error("Unable to parse exchange history file at line: " + lineNumber);
                continue;
            }

            // try to parse line. if parsing fails, just skip it
            try {
                int id = Integer.parseInt(tokens[1]);
                long timeInMillis = Long.parseLong(tokens[2]);

                if (tokens[0].equals(System.RTC)){
                    rtcHistory.put(id, new Date(timeInMillis));
                } else if (tokens[0].equals(System.ALM)){
                    almHistory.put(id, new Date(timeInMillis));
                }

            } catch (Exception e){
                log.error("Unable to parse exchange history file at line:" + lineNumber, e);
            }

            lineNumber++;
        }

    } catch (FileNotFoundException e){  // try to create a new file in case it doesn't exist

        log.warn("No exchange history file found at " + path);

        try {
            file.createNewFile();
            log.info("Created new exchange history file at "  + path);
        } catch (IOException ioe){      
            log.error("Unable to create exchange history file at " + path 
                    + ". Exchange session info won't be saved", ioe);
        }
        // return an empty history list cause there are no records in new file
        return exchHist; 
    } catch (IOException e) {
        log.error("Unable to read exchange history file at " + path
                + ". All items will be synchronized anew", e);
        return exchHist;
    } finally{
        if (reader != null){
            try {
                reader.close();
            } catch (IOException e) {
                log.error("Unable to close input stream. Resource leak possible", e);
            }
        }
    }

    return exchHist = new ExchangeHistory(rtcHistory, almHistory);
}

Answer 1

This is indeed way too complex. In fact I think there's enough responsibilities in this one method to be spread over a few classes.

• managing the history file
• parsing history records
• processing history records
• counting lines
• logging

These points may map to simply a helper method or a separate class.

Try organizing this behavior in several levels of abstraction. Try checking the existence of the file prior to tryig to parse its contents. Making a class that models the content of one record will definitely help. It'll make a good home for some of the validation and parsing logic. Don't be afraid to make private helper methods. They can greatly improve readability and lower complexity per method significantly.

The top level version of this method could look like this :

public ExchangeHistory loadHistory(String path) {
    List<HistoryRecord> records = new ArrayList<>();
    for (String line : new LineIterable(getHistoryReader(path))) {
        records.add(HistoryRecord.fromTokenizedString(line));
    }
    return ExchangeHistory.fromHistoryRecords(nonNulls(records));
}

In which

• getHistoryReader(path) will fetch or create the file and wrap it as a reader.
• LineIterable abstracts reading the BufferedReader to an Iterable (enabling the advanced for loop)
• HistoryRecord parses the tokens of a line, and can be queried for the parsed data.
  • fromTokenizedString(line) returns a valid HistoryRecord or null
• nonNulls() filters nulls out of a List
• ExchangeHistory.fromHistoryRecords() creates the ExchangeHistory based on a List/Iterable of HistoryRecords

As for logging, it always clutters code. Your error logging currently seems to be a placeholder for decent exception handling. The other logging appears to be used for debugging. I like AOP to do logging, since it allows you to focus on the code, and the logging can be coded separately.

Answer 2

One thing you could do in a situation like this is to move the meat of the try block into an overloaded function that takes the reader as an argument. This would not only reduce the nesting a bit, but it would also separate the parsing logic from the file/reader handling.

Answer 3

You can reduce the inner try/catch block in your while loop. For instance:

int id;
long timeInMillis;

while (...) {
    // ...

    try {
        id = Integer.parseInt(tokens[1]); 
        timeInMillis = Long.parseLong(tokens[2]);
    } catch (NumberFormatException e) {
        // Note that the specific exception is caught instead of Exception
    }

    // ...
}

Answer 4

I would rewrite it like this:

public static ExchangeHistory loadHistory(String path) {
    File file = new File(path);

    BufferedReader reader = null;
    try {
        reader = new BufferedReader(new InputStreamReader(new FileInputStream(file)));

        return parseHistory(reader);
    } catch (FileNotFoundException e) {
        log.warn("No exchange history file found at " + path);

        createNewFile(file);
        return ExchangeHistory.EMPTY; 
    } catch (IOException e) {
        log.error("Unable to read exchange history file at " + path
                + ". All items will be synchronized anew", e);
        return ExchangeHistory.EMPTY;
    } finally {
        closeOrLog(reader);
    }
}

private static ExchangeHistory parseHistory(Reader reader) {
    Map<Integer, Date> rtcHistory = new HashMap<Integer, Date>();
    Map<Integer, Date> almHistory = new HashMap<Integer, Date>();

    String line;
    int lineNumber = 1;

    while ((line = reader.readLine()) != null){
        String[] tokens = line.split("\\s");
        if (tokens.length != 3){
            log.error("Unable to parse exchange history file at line: " + lineNumber);
            continue;
        }

        try {
            ExchangeHistoryEntry entry = parseEntry(tokens);

            if (entry.isRTC()) {
                rtcHistory.put(entry.getID(), entry.getDate());
            } else if (entry.isALM()) {
                almHistory.put(entry.getID(), entry.getDate());
            }
        } catch (NumberFormatException e) {
            log.error("Unable to parse exchange history file at line:" + lineNumber, e);
        }

        lineNumber++;
    }

    return new ExchangeHistory(rtcHistory, almHistory);
}

private static ExchangeHistoryEntry parseEntry(String[] tokens) throws NumberFormatException {
    int id = Integer.parseInt(tokens[1]);
    long timeInMillis = Long.parseLong(tokens[2]);

    return new ExchangeHistoryEntry(tokens[0], id, new Date(timeInMillis));
}

private static void createNewFile(File file) {
    try {
        file.createNewFile();
        log.info("Created new exchange history file at "  + path);
    } catch (IOException ioe){      
        log.error("Unable to create exchange history file at " + path 
                + ". Exchange session info won't be saved", ioe);
    }
}

private static void closeOrLog(Reader reader) {
    if (reader != null){
        try {
            reader.close();
        } catch (IOException e) {
            log.error("Unable to close input stream. Resource leak possible", e);
        }
    }
}

Basically, I've done the following:

• Removed a bunch of comments because they were completely redundant. We can all read code -- there's no need to describe what it does if it's plain to see.
• Split the method into a few different methods. Now, each method has one responsibility, and all the code in each method works on a similar abstraction level. Now, it's very easy to see what the loadHistory method does without reading a screenful of code. Each individual method can be read and understood at a glance, and it's easy to verify that it does what it claims.
• Reduced the scope of a rtcHistory and almHistory by introducing ExchangeHistory.EMPTY.
• Introduced a helper class (ExchangeHistoryEntry) to reduce complexity and help further break the code down. It should be trivial to implement if you so choose.

(Disclaimer: this is completely untested code.)

Answer 5

I like it. It's the best looking Java code I've seen posted here in a while. I have to disagree with @bowmore. For a hundred lines of code, it's quicker to code, easier to read, and easier to maintain in one little method like you have it. Keep it simple. Make classes only when you exhaust what you can do the way you are coding now.