-2
\$\begingroup\$

I have a piece code which retries 3 more times to open a file (see below). Are there potential problems or side effects with the way it is written? How can it be improved? Also, why would one use Thread.currentThread().sleep(x) instead of simply Thread.sleep(x)? Any other code smell exists here?

import java.io.*;
public class MyTest{
    public static void main(String[] args) {
        int retryCount = -1;
        String line = null;
        do {
            try {
                BufferedReader br = new BufferedReader(
                    // try to open a file or stream (may not exist)
                    new InputStreamReader(new FileInputStream("input.txt")));
                    line = br.readLine();
                    br.close();
                }
            catch (IOException e) {
                try{
                    // wait a few moment to see if the file or stream will be available soon
                    System.out.println("IOException occurred.");
                    Thread.currentThread().sleep(300);
                }catch(InterruptedException ie){
                    ie.printStackTrace();
                }
                if (retryCount == -1) {
                    retryCount = 3;
                } else {
                    retryCount--;
                }
            }
        } while (retryCount > 0);
       }
}
\$\endgroup\$
1

1 Answer 1

7
\$\begingroup\$

First, don't ever call Thread.currentThread().sleep(...). sleep(...) is a static method on Thread and it's only because of some syntactic peculiarities that you can invoke static methods from an instance to begin with. Thread.sleep(...) is idiomatic.

Second, I wouldn't set retryCount to -1, set it to 3, and then count down again to 0. Why not just start at 0 and count up to 3 when necessary? As a bonus, then your variable name will actually describe what it's doing.

Third, your BufferedReader won't close if an exception is thrown. Either enclose the close in a finally block, or if you're using Java 7 or above, use a try-with-resources block.

As a general case there's probably a better solution than just waiting to see if a file is magically going to show up for you to read, but it's hard to say what it is without some more specific details.

Formatting: Be consistent in your indentation and spacing. Both are all over the place for both your try/catch blocks and neither match the (more common) style you use for the if/else blocks.

\$\endgroup\$
1
  • \$\begingroup\$ @MannyMeng, yeah. More credit for user3761894 and also to you for editing. :) \$\endgroup\$
    – Qrious
    Commented Mar 6, 2015 at 21:44

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