Improve this image file browser (remove redundancy)?

Question

Let me start by saying that the code works as is but I think it has a lot of redundancy. It is written this way as I kept getting nullpointerexceptions if I try to put the fileCounter > gameDiff if/else inside one for loop.

The program allows a user to create directories and add images to them. It will also display a certain number of images based on the users selection. If the user's chosen directory doesn't have enough images, it drops back to the default directory to add more.

String userDir = cboImages.getSelectedItem().toString();
File filePath = new File(baseDir + "/" + userDir + "/");

comboFile = filePath.listFiles();
int fileCounter = 0;
for (int ctr = 0; ctr < comboFile.length; ctr++) {
    if (comboFile[ctr].isFile()) {
        fileCounter++;
    }
}

filePath = new File(baseDir + "/default/");
defaultFile = filePath.listFiles();

List<File> listShuffle = Arrays.asList(comboFile);
List<File> defaultShuffle = Arrays.asList(defaultFile);
Collections.shuffle(listShuffle);
Collections.shuffle(defaultShuffle);

if(fileCounter > gameDiff){
    for (int ctr = 0; ctr < gameDiff; ctr++) {

        Card card = new Card();
        card.setbackImage(new ImageIcon(cardBackImage));

        Image img = null;

        if (listShuffle.get(ctr).isFile()) {
            img = new ImageIcon(listShuffle.get(ctr).getPath()).getImage();
        }
        Image dimg = img.getScaledInstance(144, 216, Image.SCALE_SMOOTH);
        card.setfrontImage(new ImageIcon(dimg));

        // Populate card with db results
        cardsList.add(card);    
    }
}
else{
    for (int ctr = 0; ctr < gameDiff; ctr++) {

        Card card = new Card();
        card.setbackImage(new ImageIcon(cardBackImage));

        Image img = null;

        if (ctr < listShuffle.size()) {
            if (listShuffle.get(ctr).isFile()) {
                if (fileCounter <= gameDiff) {
                    img = new ImageIcon(listShuffle.get(ctr).getPath()).getImage();
                }
            }
        } else if (ctr < gameDiff) {
            if (defaultShuffle.get(ctr).isFile()) {
                img = new ImageIcon(defaultShuffle.get(ctr).getPath()).getImage();
            }
        }

        Image dimg = img.getScaledInstance(144, 216, Image.SCALE_SMOOTH);
        card.setfrontImage(new ImageIcon(dimg));

        // Populate card with db results
        cardsList.add(card);
    }
}

Using two separate groups insures that the chosen directory images get added.

Answer 1

First of all, Java code doesn't really make sense outside of the context of a class and a method. Sometimes you can get away without the class, but the method signature is definitely important. In this case, you've included references to variables without a type: are these arguments to a method? Members of a class?

Second, your description of what this code does has too many ands in it. Code should be organized in terms of abstractions. For instance, if you describe something this way, it's a bit confusing as to what it actually does: "It lets you write arrays of bytes to a magnetic medium and causes the read heads to seek to a particular place on the disk and also it lets you refer to a group of bytes by a name, etc, etc." Instead you might start from the bottom: "Files are sequences of bytes intended for long-term storage; they support the following operations: append, truncate, etc. Files are organized into a directory structure by a filesystem, which is built on top of a storage device, etc." This description divides the details into various levels of abstraction.

I'll give a new listing and afterward describe why I made the changes I did.

class Card {
    private final ImageIcon frontImage;
    private final ImageIcon backImage;

    public Card(ImageIcon frontImage, ImageIcon backImage) {
        this.frontImage = frontImage;
        this.backImage = backImage;
    }
    // Probably more goes on here
}

class Deck {
    private static final String DEFAULT_SUBDIRECTORY = "default";
    private final String baseDir;
    private final ImageIcon cardBackImage;

    private List<File> loadSubdirectory(String subdir) {
        List<File> files = new ArrayList<File>();
        File[] listing = new File(baseDir, subdir).listFiles();
        for (int i = 0; i < listing.length; i++) {
            if (listing[i].isFile()) {
                files.add(listing[i]);
            }
        }
        return files;
    }

    public List<Card> selectCardsFromDirectory(int numCards, ComboBox cboImages) {
        return selectCardsFromDirectory(numCards, cboImages.getSelectedItem().toString());
    }

    /** Selects numCards at random from the user's directory,
        filling in from the default directory if the user has
        too few */
    public List<Card> selectCardsFromDirectory(int numCards, String userDirectory) {
        List<File> userFiles = loadSubdirectory(userDirectory);
        List<File> defaultFiles = loadSubdirectory(DEFAULT_SUBDIRECTORY);

        Collections.shuffle(userFiles);
        Collections.shuffle(defaultFiles);

        List<Card> cards = new ArrayList<Card>();
        for (int i = 0; i < numCards; i++) {
            ImageIcon frontImage;
            if (i < userFiles.size()) {
                frontImage = loadScaledImage(userFiles.get(i));
            } else {
                frontImage = loadScaledImage(defaultFiles.get(i - userFiles.size()));
            }
            cards.add(new Card(frontImage, cardBackImage));
        }
        return cards;
    }

    private ImageIcon loadScaledImage(File file) {
        Image image = new ImageIcon(file.getPath()).getImage();
        return new ImageIcon(
            image.getScaledInstance(IMAGE_WIDTH, IMAGE_HEIGHT, Image.SCALE_SMOOTH));
    }
}

This is a quick and dirty refactoring and I haven't compiled it to check that it all builds, but it should give you the idea. In particular, the code is not presented in any reasonable order.

It appears that you had a bug in the code; you use fileCounter to determine how many normal files are in a directory, and only take fileCounter files from the user directory, but you don't skip past directories, etc. I've modified the logic.

This Deck class knows about too many things. Why does it know about the disk layout? Why does it know about elements of the GUI (ie cboImages)? Try reading up on MVP or MVC design for ideas of how to restructure your code to have better encapsulation. Some thoughts:

• Separate the shuffling logic out of the class that deals with the file system; that should be as simple as moving loadSubdirectory into a new class.
• Remove the overload of selectFilesFromDirectory that takes a ComboBox; the GUI should extract the String before calling into this class.

I've pulled out some helper functions. These do two things: reduce code duplication, and improve abstraction. As a good rule of thumb, most methods shouldn't exceed 25 lines. By pulling out the file-loading logic, I've made it so you don't need to think about how exactly the files are loaded when you're reading selectFilesFromDirectory.

Card has some problems as a class. It is odd that you are default-constructing it, and then constructing it more afterward. I've redesigned part of it.

ctr is not a very good name for a generic loop counter. Most programmers use i, j, and other one-letter names for short loops, or actually descriptive names. For this reason, when reading the code, my brain tries to figure out what ctr means. I'm lucky; English is my first language -- for some people who don't have English as their first language, unusual abbreviations of English words are difficult to decipher.

Answer 2

I'll give you some general comments about your code and style.

---

File filePath = new File(baseDir + "/" + userDir + "/");

That's bad. The File class has a constructor especially designed to create joined paths:

File filePath = new File(baseDir, userDir);

No more dealing with path separators.

---

comboFile = filePath.listFiles();
int fileCounter = 0;
for (int ctr = 0; ctr < comboFile.length; ctr++) {
    if (comboFile[ctr].isFile()) {
        fileCounter++;
    }
}

This loop is unnecessary complicated. Use a for loop whenever you need to know the index of what you iterate over or if it is not something compatible with the foreach loop. You should always use a foreach loop by default.

int fileCounter = 0;
for (File file : filePath.listFiles()) {
     if (file.isFile()) {
          fielCounter++;
     }
}

---

There is no check that the directories you us actually exist, does it not matter or are they guarantied to exist?

---

File filePath = new File(baseDir + "/" + userDir + "/");
...
filePath = new File(baseDir + "/default/");

First, do not reuse variables if they will be holding two distinct values or have been used in between. Second, your naming of variables could be better.

File absoluteUserDir = new File(baseDir, userDir);

Actually, this doesn't make much sense, it would be better if userDir would hold this path directly:

File userDir = new File(baseDir, cboImages.getSelectedItem().toString());

And the last one:

File defaultDir = new File(baseDir, "default");

---

card.setbackImage(new ImageIcon(cardBackImage));
card.setfrontImage(new ImageIcon(dimg));

There's a typo here, the b and f should be uppercase.

---

Image dimg = img.getScaledInstance(144, 216, Image.SCALE_SMOOTH);

Again, this is not the best name this variable could have:

Image scaledImage = ...

---

cardsList.add(card);

There's no need to pre- or postfix variables with their type. cards alone does already tell the reader that it is a collection or list of some sort.

Answer 3

This is a suggested improvement on @ruds refactoring. I liked his refactoring, particularly his i - userFiles.size() trick. But conditional in a loop still looks ugly. Uninitialized declarations and multiple assignments to a local variables are not good either. It can be improved easily though, once the above trick is noted.

public List<Card> selectCardsFromDirectory(int numCards, String userDirectory) {
    List<File> userFiles = loadSubdirectory(userDirectory);
    List<File> defaultFiles = loadSubdirectory(DEFAULT_SUBDIRECTORY);

    Collections.shuffle(userFiles);
    Collections.shuffle(defaultFiles);

    ArrayList<File> allFiles = new ArrayList<File>();
    allFiles.addAll(userFiles);
    allFiles.addAll(defaultFiles);

    List<Card> cards = new ArrayList<Card>();
    for (int i = 0; i < numCards; i++) {
        ImageIcon frontImage = loadScaledImage(allFiles.get(i));
        cards.add(new Card(frontImage, cardBackImage));
    }
    return cards;
}

With guava selectCardsFromDirectory can be further condensed, at the expense of some usual Java clutter, like this:

public List<Card> selectCardsFromDirectory(int numCards, String userDirectory) {
    List<File> userFiles = loadSubdirectory(userDirectory);
    List<File> defaultFiles = loadSubdirectory(DEFAULT_SUBDIRECTORY);

    Collections.shuffle(userFiles);
    Collections.shuffle(defaultFiles);

    return FluentIterable
        .from(Iterables.concat(userFiles, defaultFiles))
        .transform(FILE_TO_CARD)
        .limit(numCards)
        .toList();
}

// just some glue
private static final Function<File, ImageIcon> FILE_TO_CARD 
    = new Function<File, ImageIcon>() {
        public ImageIcon apply(File file) {
            return fileToCard(file);
        }
    };

private static final fileToCard(File file) {
    ImageIcon frontImage = loadScaledImage(file);
    return new Card(frontImage, cardBackImage);
}