5
\$\begingroup\$

I wrote a program that finds empty folders and subfolders in a directory. It's the first program I've ever written in Java, and I'd like some critique. I feel like I'm not utilizing classes in a very coherent manner. Also, I think I'm naming variables poorly, because as I write my program I find myself constantly having to rename methods/classes/variables to clarify their meaning.

Part 1: Under the Hood

package emptyfolders;

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

public class EmptyFolders {
    public static void main(String[] args) {
    }

    private List seekEmpty(String dirname, List emptyFoldersTemp) {
        // This method goes through all folders that are directly contained in a
        // directory, and appends them to a list if they are empty. Used
        // recursively for directories containing multiple folders

        // Get the appropriate directory from user input
        File dir = new File(dirname);

        // Populate an array with files/folders in the directory
        File[] folderContents = dir.listFiles();

        // Iterate through every file/folder
        for (File currentContent : folderContents) {
            // Disregard files, acquire folders
            if (currentContent.isDirectory()) {
                // If this folder is empty, add it to the list
                if (currentContent.listFiles().length == 0) {
                    emptyFoldersTemp.add(currentContent);
                // If not, run this method on the folder
                } else if (currentContent.listFiles().length >= 1) {
                    seekEmpty(currentContent.toString(),emptyFoldersTemp);
                }   
            }
        }

        // Return a list containing all empty folders currently found
        return emptyFoldersTemp;
    }


    List listLooper(String folder) {
        // An outer program that helps seekEmpty with its recursion, instantiate
        EmptyFolders directory = new EmptyFolders();

        // Create a temporary list that holds empty folders
        List emptyFoldersTemp = new ArrayList();

        //Run seekEmpty 
        List emptyFolders = directory.seekEmpty(folder, emptyFoldersTemp);

        //Return the list of empty subfolders
        return emptyFolders;
    }
}

listLooper feels redundant, especially the part where it causes the program to return the List Object through two methods. All the stuff there was originally in the main method, but I needed the method to take a single String (for reasons you will see later, it grabs a directory from a UI). Should I get rid of it, and if so, how?

Part 2: The UI

private void listButtonActionPerformed(java.awt.event.ActionEvent evt) {                                           
    // When the "List" button is pressed, pull the directory from the
    // text field, and search that directory for empty folders and subfolders

    //Get directory from directory field and double all the backslashes so 
    //for functional purposes, note that since backslashes are literals,
    //they are doubled in the "replace" method
    String targetDirectory = directoryField.getText().replace("\\","\\\\");

    //Instantiate an instance of the EmptyFolder finder
    EmptyFolders emptyFolderFinder = new EmptyFolders();

    //Run the empty folder finder recursive method (separate file)
    List emptyFolders = emptyFolderFinder.listLooper(targetDirectory);

    //Clear the text area, if it contains text
    if (! emptyFoldersArea.getText().equals("")) {
        emptyFoldersArea.setText("");
    }

    for (Object emptyFolder : emptyFolders) {
        //Append all empty folders found in the target directory
        emptyFoldersArea.append(emptyFolder.toString() + "\n");
    }
}                                          

private void browseButtonActionPerformed(java.awt.event.ActionEvent evt) {                                             
    // When the "Browse..." button is pressed, the UI prompts the user to
    // select the directory he or she wants to check for empty folders, and
    // the program calls a method to do so

    //Instantiate an instance of the file browser
    JFileChooser fileBrowser = new JFileChooser();

    //Rename title, specify that we are only concerned with folders
    fileBrowser.setDialogTitle("Select target directory");
    fileBrowser.setFileSelectionMode(JFileChooser.DIRECTORIES_ONLY);

    //Return the directory on affirmation and write it to UI
    int returnValue = fileBrowser.showDialog(this, null);
    if (returnValue == JFileChooser.APPROVE_OPTION) {
        String targetDirectory = fileBrowser.getSelectedFile().toString();
        directoryField.setText(targetDirectory);
    } else {
    }
}

I built a Swing UI using Netbeans, so I'm cutting that code out. What I care about are two buttons in the UI. One is a browse button that the user uses to pick a directory, and the other is a button that runs the program. The list of empty folders is displayed in an empty area. I don't know how to better integrate this with my program, I feel like it sticks out in a way that doesn't flow with the rest of my program.

I'm taking college programming classes when I come back from the break, so this was my way of exposing myself to Java early, so any advice before I get started is appreciated (fingers crossed that CS is right for me).

\$\endgroup\$
  • 1
    \$\begingroup\$ You're currently in danger of infinite recursion if the folder structure is cyclical (eg, a shortcut for Desktop is placed on the Desktop...). I doubt Windows or other OSs have any of these by default, but you can't guarantee that some random program (or person) has at some point (actually, Win7 has an option to show a link to the User directory on the desktop, which would do this). The common strategy is to retain a list of all items visited (linked lists have similar problems). Also, you probably want Set, as there's no reason to store a folder more than once. \$\endgroup\$ – Clockwork-Muse Jan 6 '14 at 8:35
7
\$\begingroup\$

There are a few small items, and a couple of larger ones that concern me in your code...

  • You have an empty main method under your hood.... it serves no purpose, so remove it ;-)
  • you are not using the generic typing for your List. In this case, your code should look like:

    List<File> emptyFolders = emptyFolderFinder.listLooper(targetDirectory);
    

    and the listLooper method should return List<File>.

  • in your listLooper method you declare your List<File> and then pass it in to the seelEmpty method. This method returns the exact same List instance back, so it should not have to return a value at all. The input List<File> is an 'accumulator' that you add things to. There is no reason to return it as well.

  • seekEmpty takes a String dirname as a parameter. This should not be a String, but a File. You already have the file when you call the method, so you convert the file to String, and then immediately convert it back. just keep it as a File the whole time.
  • in seekEmpty (again) you have the code which checks for the number of files in a Directory:

    if (currentContent.listFiles().length == 0) { .... } else if (currentContent.listFiles().length >= 1) { ... }

    the else-if makes no sense because, if the length is not == 0 then it can only be >= 1. It should be a simple } else { and not have an else if at all.

OK, now for some of the bigger things...

You ask whether you should get rid of the listLooper method. The answer is probably no. You are setting up a recursive call chain with the seekEmpty method. You often need an 'entry' to a recursive method, and the listLooper is this method. The method will pre-and-post process the data required by, and created by the recursion.

Right, you want to find directories which have no files....

What I don't like about your code is that it calls 'listFiles()` twice. We can fix that with/in the recursion....

A method like the following will serve you better, I think:

/** This method goes through all folders that are directly contained in a
  * directory, and appends them to a list if they are empty. Used
  * recursively for directories containing multiple folders
  *
  * @param dir is the directory to check for content
  * @param emptyFolders is he List we add empty folders to.
  */
private void seekEmpty(File dir, List<File> emptyFolders) {

    // Populate an array with files/folders in the directory
    File[] folderContents = dir.listFiles();
    if (folderContents.length == 0) {
        // we are empty, add us.
        emptyFolders.add(dir);
    }

    // Iterate through every file/folder
    for (File content : folderContents) {
        // Disregard files, acquire folders
        if (content.isDirectory()) {
            // check if this folder is empty
            seekEmpty(content, emptyFolders);
        }
    }
}

This significantly simplifies the recursion.

\$\endgroup\$
  • \$\begingroup\$ Thanks! You've helped realize the importance of coding with increased emphasis on efficiency and, IMO, maintainability. Are there any resources on good coding practices that you can recommend? \$\endgroup\$ – itsmichaelwang Jan 8 '14 at 6:36
  • 2
    \$\begingroup\$ @Zapurdead Joshua Bloch ... Effective Java, then Java Concurrency In Practice ... can't go wrong. \$\endgroup\$ – rolfl Jan 8 '14 at 6:44

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.