6
\$\begingroup\$

Purpose of the code is to delete all virus affected files from your pc.

   private static void SearchAndDeleteFiles(String directoryName, List<File> files) {

        File directory = new File(directoryName);

        File[] fList = directory.listFiles();

        if (fList != null) {

            for (File file : fList) {
                if (file.isFile() && (file.getName().endsWith(".ccc") || file.getName().startsWith("_how_recover_anj")
                        || file.getName().startsWith("_how_recover_jav"))) {
                    files.add(file);
                    System.out.println((new StringBuilder()).append(file).toString());

                    boolean isSuccess = file.delete();

                    System.out.println(
                            (new StringBuilder()).append(file).append(" delete status: ").append(isSuccess).toString());

                } else if (file.isDirectory()) {
                    SearchAndDeleteFiles(file.getAbsolutePath(), files);
                }

            }
        }

    }
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

For what you are trying to achieve, Java has a very nice solution since Java 7: java.nio.Files.

The Files class comes with a lot of useful methods that do what you did more expressively and easier to understand.

Instead of listing all files in a directory and looping over them, you can actually walk the entire file tree, including sub-directories, if you want.

You can do this the "callback-style" or using Java Streams. I like streams, so I will try and rebuild what you did, but with java.nio.Files and java.util.stream.Stream. Also I am going to use Java 11's var keyword, because I don't like typing out all the types unless absolutely necessary:

/**
 * Deletes all files with their names ending in ".ccc" or starting with
 * "_how_recover_anj" or "_how_recover_jav".
 * 
 * @param directoryName The directory to search in.
 * @return A map of all files which were attempted to be deleted and their
 *         deletion status.
 */
private static Map<File, Boolean> SearchAndDeleteFiles(String directoryName) throws IOException {
    try (var fileStream = Files.walk(Paths.get(directoryName))) {
        return fileStream.map(path -> path.toFile())
                .filter(file -> 
                        file.isFile()
                        && (file.getName().endsWith(".ccc")
                        || file.getName().startsWith("_how_recover_anj")
                        || file.getName().startsWith("_how_recover_jav")))
                .peek(file -> System.out.println(file))
                // Actually delete the file and record the status.
                .map(file -> Map.entry(file, file.delete()))
                .peek(entry -> System.out.printf("%s delete status: %s%n", entry.getKey(), entry.getValue()))
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
    } catch (IOException e) {
        throw new IOException(e);
    }
}

First you see, that I actually provide some documentation. Tell the user (even your future self), what this method is supposed to do, what is wants as parameters and returns as a result.

Also notice, that I removed the files argument. You changed that list and added the files that you traversed (not the ones that were successfully deleted) to that list. This is a code smell as you are modifying the incoming collection which could very possibly be immutable without checking.
In my solution, this is no longer necessary, as we don't need any recursion and actually return which files we traversed and attempted to delete.

Then I am following the recommendation written in the documentation and walk the file tree in a try-with-resources block. This makes sure, that in any case, the file handles to the directories are cleanly closed.

The stream is expressive and pretty self-explanatory, it first maps all the file paths to actual Java File objects, filters the ones matching your criteria, then prints the file name. You don't need to construct StringBuilders for every string you print, just more complex expressions and actually constructed strings.

Then it maps the file to itself and it's deletion status. This status is then printed, just like in your method and collected into a map.
The map is then returned.

If any exception is thrown, it is re-thrown and the stream of paths is cleanly closed.

The Files.walk method is implicitly recursive and lazy, so you don't need to worry about performance or traversing into sub-directories.

You could make this method even more flexible, if you want, but I hope this answers your question.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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