1
\$\begingroup\$

My code works as expected but I am not sure how to optimize the code. The purpose of the code is to find files with given parameters and copy them to a separate folder.

import java.io.File;
import java.io.IOException;
import org.apache.commons.io.FileUtils;
import javax.swing.JOptionPane;
import java.io.File;
import java.io.IOException;
import java.awt.Frame;
import java.awt.image.RenderedImage;
import javax.media.jai.widget.ScrollingImagePanel;
import javax.media.jai.NullOpImage;
import javax.media.jai.OpImage;
import com.sun.media.jai.codec.SeekableStream;
import com.sun.media.jai.codec.FileSeekableStream;
import com.sun.media.jai.codec.TIFFDecodeParam;
import com.sun.media.jai.codec.ImageDecoder;
import com.sun.media.jai.codec.ImageCodec;




public class FileSearch {

    int sizeOfFile = Integer.parseInt((JOptionPane.showInputDialog(null,"Enter Size: ")));

    public void findFiles(File root) throws IOException {

        File[] listOfFiles = root.listFiles();
        for (int i = 0; i < listOfFiles.length; i++) {
            String iName = listOfFiles[i].getName();
            if (listOfFiles[i].isFile() && iName.endsWith(".tif")) {

                RenderedImage renderedImage[], page;
                File tiffFile = new File(listOfFiles[i].getAbsolutePath());

                SeekableStream seekableStream = new FileSeekableStream(tiffFile);
                ImageDecoder imageDecoder = ImageCodec.createImageDecoder("tiff",seekableStream, null);
                renderedImage = new RenderedImage[imageDecoder.getNumPages()];

                int tifPageCount = 0;
                for (int k = 0; k < imageDecoder.getNumPages(); k++) {
                    renderedImage[k] = imageDecoder.decodeAsRenderedImage(k);
                    tifPageCount++;
                }

                long fileSize = listOfFiles[i].length();

                if ((fileSize <= (sizeOfFile*1000)) && (tifPageCount <=15)) {
                    File file = new File("\\\\server\\filename\\TestFiles\\" + listOfFiles[i].getName());

                    if (file.exists()) {
                        System.out.println(file.getName() + " already exists");
                    } else {
                        File folder = new File("\\\\server\\filename\\TestFiles\\Files by Size\\Files of size " + ((sizeOfFile*1000) / 1000) + " KB");
                        if (!folder.exists()) {
                            if (folder.mkdir()) {
                                System.out.println("Folder : " + folder.getAbsolutePath());
                            }
                        }

                        try{
                            System.out.println(listOfFiles[i].getName() + " | " + FileUtils.byteCountToDisplaySize(fileSize));
                            FileUtils.copyFileToDirectory(listOfFiles[i], folder);                          
                        }
                        catch(IOException e){
                            System.out.println("Same files");
                            return;
                        }
                    }
                }
            }

            else if (listOfFiles[i].isDirectory()) {
                findFiles(listOfFiles[i]);
            }
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

Simplifying the notation

        File[] listOfFiles = root.listFiles();
        for (int i = 0; i < listOfFiles.length; i++) {
            String iName = listOfFiles[i].getName();
            if (listOfFiles[i].isFile() && iName.endsWith(".tif")) {

Consider

        for (File source : root.listFiles()) {
            if (source.isFile() && source.getName().endsWith(".tif")) {

This way we don't create an index variable that we don't need. Instead, we iterate over the list directly. This saves dereferencing.

I also got rid of the iName variable. This is more of a judgment call, but it was only used once. I find that unnecessary.

I prefer the name source, as it is clearer about the purpose of the file.

Direct answers

                RenderedImage renderedImage[], page;
                File tiffFile = new File(listOfFiles[i].getAbsolutePath());

                SeekableStream seekableStream = new FileSeekableStream(tiffFile);
                ImageDecoder imageDecoder = ImageCodec.createImageDecoder("tiff",seekableStream, null);
                renderedImage = new RenderedImage[imageDecoder.getNumPages()];

                int tifPageCount = 0;
                for (int k = 0; k < imageDecoder.getNumPages(); k++) {
                    renderedImage[k] = imageDecoder.decodeAsRenderedImage(k);
                    tifPageCount++;
                }

Consider

                File tiffFile = new File(source.getAbsolutePath());
                SeekableStream seekableStream = new FileSeekableStream(tiffFile);
                ImageDecoder imageDecoder = ImageCodec.createImageDecoder("tiff", seekableStream, null);

                int tifPageCount = imageDecoder.getNumPages();

This saves declaring the never used page.

It saves declaring and populating renderedImage when you only want to generate a count.

Separation of concerns

The real problem here is that the method does a bunch of different things. It takes a directory and traverses it. At each file, it checks if the file matches a characteristic. If the file matches, it takes an action. It produces output as it goes. Consider

public class FileFinder {

    private List<File> files = new ArrayList<>();
    private final FileMatcher matcher;
    private final File root;

    public FileFinder(FileMatcher matcher, File root) {
        this.matcher = matcher;
        this.root = root;
    }

    public List<File> findMatchingFiles() throws IOException {
        files = new ArrayList<>();
        findMatchingFiles(root);
        return files;
    }

    private void findMatchingFiles(File root) throws IOException {
        if (root.isFile()) {
            if (matcher.matches(root)) {
                files.add(root);
            }

            return;
        }

        if (!root.isDirectory()) {
            return;
        }

        for (File file : root.listFiles()) {
            findMatchingFiles(file);
        }
    }

}

This class and method has a single purpose, to traverse a file system from a particular root and find all the matching files. Rather than processing them in place, it returns the files to be processed by the caller. It doesn't even know how the files match, only that they do.

It's not clear to me that this needs the throws IOException. I didn't try it, as I didn't want to set up a test environment. If needed, this is how to do it. If not, you can just remove them.

public interface FileMatcher {

    boolean matches(File file);

}

Not much to the interface.

public class SuffixMatcher implements FileMatcher {

    private final String suffix;

    public SuffixMatcher(String suffix) {
        this.suffix = suffix;
    }

    public boolean matches(File file) {
        return file.getName().endsWith(suffix);
    }

}

Not much to the implementation either. The important part is that this abstracts the matching from the traversal. We could reuse this code for not just different suffixes but entirely different criteria.

public class TiffCopier {

    public static final int MAXIMUM_COUNT = 15;
    public final int FILE_SIZE;
    public final int MAXIMUM_SIZE;
    public final File FOLDER;
    public final String TARGET_PATH;

    public TiffCopier(int fileSize, String targetPath) throws IOException {
        FILE_SIZE = fileSize;
        MAXIMUM_SIZE = fileSize * 1000;
        TARGET_PATH = targetPath;

        FOLDER = new File(targetPath + "Files by Size\\Files of size " + fileSize + " KB");
    }

    public static int countPages(File source) throws IOException {
        File tiffFile = new File(source.getAbsolutePath());
        SeekableStream seekableStream = new FileSeekableStream(tiffFile);
        ImageDecoder imageDecoder = ImageCodec.createImageDecoder("tiff", seekableStream, null);

        return imageDecoder.getNumPages();
    }

    public void copy(File source) throws IOException, SameFilesException {
        long fileSize = source.length();
        if (fileSize > MAXIMUM_SIZE || countPages(source) > MAXIMUM_COUNT) {
            return;
        }

        File file = new File(TARGET_PATH + source.getName());
        if (file.exists()) {
            System.out.println(file.getName() + " already exists");
            return;
        }

        if (!FOLDER.exists()) {
            if (FOLDER.mkdir()) {
                System.out.println("Folder : " + FOLDER.getAbsolutePath());
            }
        }

        try {
            String sizeDisplay = FileUtils.byteCountToDisplaySize(fileSize);
            System.out.println(source.getName() + " | " + sizeDisplay);
            FileUtils.copyFileToDirectory(source, FOLDER);                          
        } catch(IOException e) {
            System.out.println("Same files");
            throw new SameFilesException();
        }
    }

    public void copyAll(File root) throws IOException {
        FileMatcher matcher = new SuffixMatcher(".tif");
        FileFinder finder = new FileFinder(matcher, root);

        for (File source : finder.findMatchingFiles()) {
            try {
                copy(source);
            } catch (SameFilesException e) {
                return;
            }
        }
    }

}

This breaks your single long method into smaller pieces.

There are still some separation of concerns issues. This still has output scattered throughout the execution of the program. I did localize it to just one of the smaller methods. But that method still does more things than it should. I'm not entirely sure of the point of the output, so I didn't try to overengineer a more elegant solution.

This program still halts execution on most IOException occurrences. It might be better to handle these and continue.

I still include throws IOException in places that may not need it. I would encourage you to remove the extras.

I did break out several utilities. For example, the countPages is now its own method. I left it here, although it could be moved into its own class. Suffix matching and file system traversal get their own reusable classes.

I did not implement SameFilesException. Its implementation should be relatively trivial, as it basically just rebrands an IOException occurring at a particular point.

I declared several constants, including two that are just operations performed repeatedly on the same datum. I find it easier to follow this more abstracted version.

I standardized on putting control statements between ending and beginning braces when that fit. E.g. } else { on the same line. You sometimes did that and sometimes put the control statement on a new line after a closing brace. The normal Java standard is all on the same line, but the most important thing is consistency. Please pick one style and stick to it throughout each program.

As I said, I didn't try to compile this much less test it. Beware of silly errors introduced in editing.

\$\endgroup\$
  • \$\begingroup\$ THANK YOU so much for taking out time and reviewing my code and for the suggestions. I highly appreciate it. \$\endgroup\$ – Maddy Dec 28 '16 at 15:49

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.