Improve this image file browser (remove redundancy)?

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 listShuffle = Arrays.asList(comboFile);
List 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();
}
}

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 loadSubdirectory(String subdir) {
        List files = new ArrayList();
        File[] listing = new File(baseDir, subdir).listFiles();
        for (int i = 0; i  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 selectCardsFromDirectory(int numCards, String userDirectory) {
        List userFiles = loadSubdirectory(userDirectory);
        List defaultFiles = loadSubdirectory(DEFAULT_SUBDIRECTORY);

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

        List cards = new ArrayList();
        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.

StackExchange Code Review Q#37478, answer score: 8