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.