Inside of a test class there is a static method to return a set of all files inside a particular directory.

Because the method is calling itself to loop through files, I instantiated a set listOfFiles outside the method to add files I find. This works great the first time I call the method, but the second time I call it, listOfFiles gets added up and I can't clear it because of the return statement. Any ideas?

import java.io.File;
import java.util.HashSet;

class TestClass {
    static HashSet<File> listOfFiles = new HashSet<File>();
    
    //returns all files in the directory as a set
    static HashSet<File> getAllFiles(File file) {
        if(file.isDirectory()) {
            for(File subFiles : file.listFiles()) {
                if(subFiles.isDirectory()) {
                    getAllFiles(subFiles);
                } else {
                    listOfFiles.add(subFiles);              
                    }
            }
        } else {
            listOfFiles.add(file);              
        }

//      listOfFiles.clear();
        return listOfFiles;
    }
}

public class Test {
    public static void main(String[] args) {
        TestClass.getAllFiles(new File("C:\\Users\\user\\Documents")).forEach(System.out::println);
        TestClass.getAllFiles(new File("C:\\Users\\user\\Desktop")).forEach(System.out::println);
        
    }
}

I could probably solve this if I write bunch more methods or manually clear the set every time I call the method, but none of this seems elegant and clean.

CodePudding user response：

Don't use a static field for that.

Instead create a HashSet on the first invocation and pass it into all others.

The simplest way to do this is to have a separate helper method like this:

public static Set<File> getAllFiles(File file) {
    Set<File> result = new HashSet<>();
    getAllFilesImpl(file, result);
    return result;
}

private static void getAllFilesImpl(File file, Set<File> result) {
    if(file.isDirectory()) {
        for(File subFile : file.listFiles()) {
            if(subFile.isDirectory()) {
                getAllFilesImpl(subFile, result);
            } else {
                result.add(subFile);              
            }
        }
    } else {
        result.add(file);              
    }
}

Note that I also changed the return type of getAllFiles() to just Set<File> because the concrete implementation class used (HashSet in this case) shouldn't matter to the caller. That's called "coding to interfaces". That change isn't necessary for this to work, though.

CodePudding user response：

I think there's a bit of confusion here. You're using mixed approaches for a recursive method.

You should either rely on an implementation which only uses a static field with no return statement (not recommended) or an implementation which returns a local variable filled with the files encountered.

In the first scenario you don't need a return statement because every method invocation is always working on the same HashSet instance. The static keyword is used to "turn" a member from an instance member to a class member. Specifically, when a field is declared as static, a single instance of that field is created regardless of how many instances of said class are created. There will always be a single instance of a static field whose value will be shared among every static and non-static contexts. Therefore, when you're updating the content of listOfFiles in your first main invocation, the second invocation still sees the results of the first invocation because both of them are working on the same HashSet instance.

In this implementation, you should always remember to clear the HashSet before every getAllFiles invocation which is definitely not the cleanest nor best approach. The getAllFiles method should be independent and return something that only belongs to its computation. The static solution should look like something like this:

public class TestClass {
    static HashSet<File> listOfFiles = new HashSet<File>();

    static void getAllFiles(File file) {
        if (file.isDirectory()) {
            for (File subFiles : file.listFiles()) {
                if (subFiles.isDirectory()) {
                    getAllFiles(subFiles);
                } else {
                    listOfFiles.add(subFiles);
                }
            }
        } else {
            listOfFiles.add(file);
        }
    }

    public static void main(String[] args) {
        //The first call does not need to clear the HashSet since it is empty at the beginning
        getAllFiles(new File("C:\\Users\\user\\Documents"));
        TestClass.listOfFiles.forEach(System.out::println);
        System.out.println();

        //The second call needs to clear the results from the previous invocation
        listOfFiles.clear();
        getAllFiles(new File("C:\\Users\\user\\Desktop"));
        TestClass.listOfFiles.forEach(System.out::println);
    }
}

The second approach, which I recommend, consists in using a local HashSet instance which is returned and added to the caller's HashSet.

public class TestClass {
    static HashSet<File> listOfFiles = new HashSet<File>();

    static HashSet<File> getAllFiles(File file) {
        HashSet<File> localListOfFiles = new HashSet<File>();
        if (file.isDirectory()) {
            for (File subFiles : file.listFiles()) {
                if (subFiles.isDirectory()) {
                    //Adding to the local HashSet the HashSet returned from my inner call
                    localListOfFiles.addAll(getAllFiles(subFiles));
                } else {
                    localListOfFiles.add(subFiles);
                }
            }
        } else {
            localListOfFiles.add(file);
        }
        return localListOfFiles;
    }

    public static void main(String[] args) {
        TestClass.getAllFiles(new File("C:\\Users\\user\\Documents")).forEach(System.out::println);
        TestClass.getAllFiles(new File("C:\\Users\\user\\Desktop")).forEach(System.out::println);
    }
}

CodePudding user response：

static HashSet<File> getAllFiles(File file) {
    HashSet<File> set = new HashSet<>();
    File[] files = file.listFiles();
    if (files != null) {
        for (File f : files) {
            if (f.isDirectory()) {
                set.addAll(getAllFiles(f));
            } else {
                set.add(f);
            }
        }
    }
    return set;
}