Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question

3 Answers 3

up vote 8 down vote accepted

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.

share|improve this answer
    
Thank you, I thought the Game Screen was doing to much but was overruled by the rest of the team. I didn't create all of the code in this class. What I posted is my code. Next time I post I'll make sure to include the variable types if they are not declared in the posted code. –  user1793408 Dec 16 '13 at 13:55

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.

share|improve this answer
    
Thank you for the comments. I will start using the file constructor to create the paths. I am still struggling with Java's foreach loop while I can do a for loop off the top of my head. I will start using it more. The directory check is not done here as it has been done during the init method when the combobox was populated with directory listings. The method calls for setbackImage and setfrontImage weren't created by me, they are in another class. In your last comment are you saying to drop the "List" part? I was attempting to do code name commenting to reduce comments. –  user1793408 Dec 16 '13 at 14:19
    
Yes, I meant to drop the list parts, it's unnecessary. cards tells you that it is a collection, array or list or something similar. If someone is only reading through it it is easier to parse. If someone wants to change something, they need to check the declaration anyway. –  Bobby Dec 16 '13 at 14:23

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);
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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