So I have this method that get executed repeatedly
public static boolean isReady(String dirPath, int numPdfInPrintJob){
File dir = new File(dirPath);
String[] fileList = dir.list(new FilenameFilter(){
public boolean accept(File file, String filename) {
return (filename.toLowerCase().endsWith(".pdf"));
}
});
if(fileList.length >= numPdfInPrintJob) return true;
else return false;
}
This method using anonymous class
that will create a new instance of FilenameFilter
every time invoked and I invoke this method a lot. So I want to make this anonymous class
into a singleton
. So my initial thought is to create a new singleton
class that look like this
public class PdfFileNameFilter implements FilenameFilter{
private PdfFileNameFilter(){} //non-instantible
//guarantee to only have one instance at all time
public static final PdfFileNameFilter INSTANCE = new PdfFileNameFilter();
public boolean accept(File dir, String name) {
return (name.toLowerCase().endsWith(".pdf"));
}
}
Can I refactor this a bit more. I need to do ZipFileNameFilter
as well, and maybe many different file extension filter. Dont want to create a class for each filter. I need to refactor this开发者_JAVA百科 design a bit more. Maybe interface
come into place somewhere here.
If all you wanted to do was reduce memory usage you could have done
private static final FilenameFilter PDF_FILES = new FilenameFilter(){
public boolean accept(File file, String filename) {
return (filename.toLowerCase().endsWith(".pdf"));
}
}
If you want to create a singleton, the simplest way is
public enum PdfFileNameFilter implements FilenameFilter {
INSTANCE;
public boolean accept(File dir, String name) {
return (name.toLowerCase().endsWith(".pdf"));
}
}
It seems simpler to me to just use your existing anonymous class and make one instance that all your method invocations use.
private static final FilenameFilter PDF_FILTER = new FilenameFilter() {
public boolean accept(File file, String filename) {
return (filename.toLowerCase().endsWith(".pdf"));
}
}
public static boolean isReady(String dirPath, int numPdfInPrintJob){
File dir = new File(dirPath);
String[] fileList = dir.list(pdfFilter);
if(fileList.length >= numPdfInPrintJob) return true;
else return false;
}
This is a case where subclassing and making a singleton seems to be a tad overkill: you simply want only one instance to use right here, whereas a singleton is used when there is only one instance you will ever want to use.
You could use an enum
for this:
public enum ExtensionFilter implements FilenameFilter {
PDF(".pdf"),
ZIP(".zip");
private final String extension;
private ExtensionFilter(String extension) {
this.extension = extension;
}
@Override
public boolean accept(File dir, String name) {
return (name.toLowerCase().endsWith(extension));
}
}
Now you'll be able to use it like:
dir.list(ExtensionFilter.PDF)
You can also iterate through them if you need:
for ( FilenameFilter fileNameFilter : ExtensionFilter.values() ) {
....
}
You could also use a vararg as constructor argument to allow multiple extensions for the same filter and use the constant name as the default to make it simpler to use:
public enum ExtensionFilter implements FilenameFilter {
PDF,
ZIP(".zip", ".jar", ".war", ".ear");
private final String[] extensions;
private ExtensionFilter(String... extensions) {
if (extensions.length == 0) {
extensions = new String[] {"." + name().toLowerCase()};
}
this.extensions = extensions;
}
@Override
public boolean accept(File dir, String name) {
for (String extension : extensions) {
if (name.toLowerCase().endsWith(extension)) {
return true;
}
}
return false;
}
}
you can step away from the full singleton and use a private field to set up the extension
public class ExtensionFileNameFilter implements FilenameFilter{
private String extension;
private ExtensionFileNameFilter (String extension){this.extension=extension;}
public static final ExtensionFileNameFilter PDFINSTANCE = new ExtensionFileNameFilter (".pdf");
public static final ExtensionFileNameFilter ZIPINSTANCE = new ExtensionFileNameFilter (".zip");
//add instances as you need
public boolean accept(File dir, String name) {
return (name.toLowerCase().endsWith(extension));
}
}
Can I refactor this a bit more.
Yes, yes you can.
Assuming that wasn't the answer you were looking for (you should update yoru question to ask a more specific question), I wouldn't refactor it until you need it; YAGNI.
Once you have more code, like more FilenameFilters, is when you will see the possible refactorings. You will see common code, maybe an interface, stuff like that. Don't try to pre-maturely over-engineeer.
TDD is also the best way to do refactoring safely. If you have tests showing what code you actually need, lots of the extra stuff goes away, if any. And with a comprehensive test suite, you can refactor without hesitation because you know if your changes work or not based on whether your tests continue to pass.
For interest, this alternative accept
implementation would run much faster in a benchmark test. It does not create new stateful Objects or carry the other overhead of String.toLowerCase
, which is not required for your case.
public boolean accept(File file, String filename) {
int offset = s.length() - 4;
if (offset >= 0) {
if (s.charAt(offset) == '.') {
offset += 1;
if (s.regionMatches(offset, "pdf", 0, 3)) {
return true;
} else if (s.regionMatches(offset, "PDF", 0, 3)) {
return true;
}
}
}
return false;
}
If this was an execution hotspot and you were looking for optimization, something like that would help.
精彩评论