3
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ Ok, I guess I need to remove the file creation part as it makes no sense here. \$\endgroup\$ – svz May 29 '13 at 13:32
  • 1
    \$\begingroup\$ Short note: you could do new BufferedReader(new FileReader()) instead of new BufferedReader(new InputStreamReader(new FileInputStream())) \$\endgroup\$ – fge May 29 '13 at 13:48
  • \$\begingroup\$ You can get a Reader in as the input parameter and remove almost all resource management concerns (including closing the reader) elsewhere right away. What does it matter if the characters come from a file or http or loaded from classpath or telepathied. As long as it implements Reader, you're good. \$\endgroup\$ – abuzittin gillifirca May 30 '13 at 6:23
  • 1
    \$\begingroup\$ See : Joshua Bloch in Effective Java : Item 7: Avoid finalizers - DO NOT change other things, your code is readable, everybody can point out where is a bug at debugging \$\endgroup\$ – cl-r May 30 '13 at 7:10
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Won't there be too many classes in this case, each with like 10 lines of code? I think there is no need to separate points 2-4 in this case. And as for logging, could you please provide a small example of what you mean? \$\endgroup\$ – svz May 29 '13 at 21:45
  • \$\begingroup\$ I've edited the answer to address this. \$\endgroup\$ – bowmore May 30 '13 at 5:12
  • 1
    \$\begingroup\$ @DaveJarvis ConcurrentSkipListSet throws NullPointerException when adding nulls, while my intent is just to filter nulls out. Aside from that : nonNulls() could indeed be implemented by copying into a Collection that ignores adding nulls. \$\endgroup\$ – bowmore Jun 14 '13 at 4:39
  • \$\begingroup\$ I should have read the documentation more carefully, bowmore. I thought it failed silently. \$\endgroup\$ – Dave Jarvis Jun 14 '13 at 4:56
3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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
    }

    // ...
}
\$\endgroup\$
  • \$\begingroup\$ Does moving the if() part out of the try/catch block make much difference? As for the Exception I'm not sure if NumberFormatException is the only one that can be thrown and I must be sure that the app keeps running. \$\endgroup\$ – svz May 29 '13 at 13:40
  • 1
    \$\begingroup\$ As to .parseInt() and .parseLong(), this is guaranteed to be the only exception as per the javadoc. NullPointerException cannot be thrown here. Moving the if out of it just makes the try block thinner, no real difference ;) But I like my try blocks small. \$\endgroup\$ – fge May 29 '13 at 13:43
2
\$\begingroup\$

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.)

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is a method returning an instance of ExchangeHistory. How probable you think that it is not part of a much bigger enterprisey system? As for " easier to read, and easier to maintain in one little method like you have it" I disagree with all three points. And even if you are doing procedural programming, you still need to follow single responsibility principle, except only this time your components are procedures. \$\endgroup\$ – abuzittin gillifirca May 30 '13 at 7:09
  • \$\begingroup\$ +1 I agree : old programmers write clear, readable code easy to debug, and they are able to make it evolve only if needed (I agree also with programmers.stackexchange.com/users/1130/rachel , a very good link) \$\endgroup\$ – cl-r May 30 '13 at 7:26

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.