Java: dangerous self-returning-recursive function by IOException?

Question

I had very errorsome Exception handling with if-clauses. An exception occurs if not found path. An exception returns the function again. The function is recursive. Safe?

$ javac SubDirs.java 
$ java SubDirs
Give me an path.
.
Dir1
Dir2
Give me an path.
IT WON'T FIND ME, SO IT RETURNS ITSELF due to Exception caught
Give me an path.

The code of Subdirs.java looks like this:

import java.io.*;
import java.util.*;

public class SubDirs {
    private List<File> getSubdirs(File file) throws IOException {
        List<File> subdirs = Arrays.asList(file.listFiles(new FileFilter() {
                    public boolean accept(File f) {
                    return f.isDirectory();
                    }
                    }));
        subdirs = new ArrayList<File>(subdirs);

        List<File> deepSubdirs = new ArrayList<File>();
        for(File subdir : subdirs) {
            deepSubdirs.addAll(getSubdirs(subdir)); 
        }
        subdirs.addAll(deepSubdirs);
        return subdirs;
    }

    public static void search() {
        try{
            BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
            String s;
            System.out.println("Give me an path.");
            while ((s = in.readLine()) != null && s.length() != 0){
                SubDirs dirs = new SubDirs();
                List<File> subDirs = dirs.getSubdirs(new File(s));
                for ( File f : subDirs ) {
                    System.out.println(f.getName());
                }
                System.out.println("Give me an path.");
            }
        }catch(Exception e){
                    // Simple but is it safe?
            search();
        }
    }

    public static void main(String[] args) throws IOException {
        search();
    }
}

Answer 1

Yes, it is safe until you enter wrong path several thousand times.

Answer 2

Debatable. I would think this leaves a mess of useless stack frames - maybe not a concern unless you try to reuse this code on a mobile device. Regardless, I wouldn't let it pass a code review without more convincing that there's no better way.

For better understandability and maintainability, get in the habit of coding what you mean: if you intend the code to loop to retry on error, then code a loop. Recursion feels slicker, but, since its not a common practice in exception handlers, it increases the surprise factor. if I were a future maintainer of this code, I'd wonder if the recursion is intentional or a bug - please leave me a comment to explain the recursion is intentional.

I would gladly exchange a few more lines of mundane code now for better maintainability and easier troubleshooting later.

public static void search() {

  boolean succeeded = false;
  int attemptsLeft = MAX_ATTEMPTS;
  while (! succeeded && (--attemptsLeft > 0)) {

    try{
        BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
        String s;
        System.out.println("Give me an path.");
        while ((s = in.readLine()) != null && s.length() != 0){
            SubDirs dirs = new SubDirs();
            List<File> subDirs = dirs.getSubdirs(new File(s));
            for ( File f : subDirs ) {
                System.out.println(f.getName());
            }
            System.out.println("Give me an path.");
        }

        succeeded = true;

    }catch(Exception e){
        // ALWAYS LEAVE YOURSELF A CLUE TO WHAT HAPPENED
        // e.g. the exception caught may not be the exception you expected
        e.printStackTrace();
    }

  } // while not succeeded yet

  if (! succeeded) {
    log("Hmm ... bailing search() without succeeding");
  }
}

Answer 3

Each time search() fails, you create another instance of search() on your call stack. As in this case it prompts the user for an input, I suppose the user is likely to give up long before this overflows your stack, so I suppose here the danger is minimal. But if you had an app that re-tried on failures or moved on to the next item in a potentially-long list, you could overflow the stack with this approach.

I'd suggest a better approach would be to put the try/catch block inside your while loop, and go through the loop again on failure instead of making a recursive call. Something like -- WARNING -- untested suggested code:

public static void search() { 
    BufferedReader in = new BufferedReader(new InputStreamReader(System.in)); 
    String s; 
    System.out.println("Give me an path."); 
    while ((s = in.readLine()) != null && s.length() != 0){ 
        try
        { 
            SubDirs dirs = new SubDirs(); 
            List<File> subDirs = dirs.getSubdirs(new File(s)); 
            for ( File f : subDirs ) { 
                System.out.println(f.getName()); 
            }
        }
        catch (Exception e) {
            System.out.println("Something went wrong! Everybody scream and run in circles!");
        }
    System.out.println("Give me an path."); 
    } 
}