Java code refactoring: multiple instanceof operator usages

Question

Consider the following example where the classes TextFile, XmlFile, HtmlFile, ShellScriptFile, et cetera, are all subclasses of the class SimpleFile. I am writing a class FileOperations, which has a method that searches the contents of a generic file depending on its type.

Following is a code sample:

public searchFile(SimpleFile targetFile, String searchStr) {
     if (targetPage instanceof HtmlFile) {
         // search html file
     }
     else if (targetPage instanceof TextFile) {
         // search text file
     }
     else if (targetPage instanceof XmlFile) {
         // search xml file
     }
     else if (targetPage instanceof ShellScriptFile) {
         // search shell file
     }   
     ...
}

This structure smells bad to me. I am aware that this is the best case for polymorphism. But I do not have control over the File class or its subclasses.I cannot write to them.

Is there another way to go about cleaning this mess? Because the if-else structure is going to keep increasing as I add support for different file types.

Else, if I am stuck with this structure, then is instanceof the fastest operator in Java? What are its implications on performance?

Would it be better to use getClass() or isAssignableFrom() for the above case?

I appreciate your comments or suggestions!

Answer 1

First I would say that your example doesn't seem bad at all. It's a little verbose, and arguable not as cohesive as separating them into different methods but it looks fairly normal to me. I think a neater approach might be to use overloading instead. I can't speak for the difference in speed but from a maintenance and extensibility stance it will be easier to deal with in future.

public void searchFile(SimpleFile targetFile , String searchStr) {
     // SimpleFile generalized behavior.
     if (targetPage instanceof HtmlFile) searchFile((HtmlFile)targetFile, searchStr);
     else if (targetPage instanceof TextFile) searchFile((TextFile)targetFile, searchStr);
     else if (targetPage instanceof XmlFile) searchFile((XmlFile)targetFile, searchStr);
     else if (targetPage instanceof ShellScriptFile) searchFile((ShellScriptFile)targetFile, searchStr);
     else System.out.println("Subtype not recognised"); 
}
public void searchFile(HtmlFile targetFile , String searchStr) {
    // HtmlFile specific behavior
}
public void searchFile(TextFile targetFile , String searchStr) {
    // TextFile specific behavior
}
public void searchFile(XmlFile targetFile , String searchStr) {
    // XmlFile specific behavior
}
public void searchFile(ShellScriptFile targetFile , String searchStr) {
    // ShellScript specific behavior
}

In the event of a new subclass of SimpleFile, it will default to the SimpleFile version of the method. In the event that the type is not known at compile time (as brought up in the comments) the most general searchFile() method can be used to check and redistribute the object accordingly.

Edit: As a note on the performance impact of multiple use of instanceof, the consensus appears to be that it's irrelevant, and that the use of modern instanceof is pretty fast anyway.

Answer 2

If you could modify File and its subclasses, you could try using the Visitor pattern. Since you cannot, I recommend that you consolidate your type testing switch statement into a single method so that you will only need to modify one switch.

public Enum FileHandler {
   HTML(){
      public search(File file, String searchstr){ /* ... */}
      public prettyPrint(File file){ /* ... */}
   },
   XML(){ /* ... */};
   // ... end list of enum implementations

   public static FileHander get(File file){
      // Put your master switch statement here
      if (targetPage instanceof HtmlFile) {
         return HTML;
      }
      else if (targetPage instanceof XmlFile) {
         return XML;
      }
      // ...
   }
}

And your code to use the enum class will look something like this, avoiding the dreaded switch statement:

public searchFile(SimpleFile targetFile, String searchStr) {
   FileHandler.get(targetFile).search(targetFile, searchStr);
}

Answer 3

you could use interpreter-design-pattern or chain-of-responsibility-design-pattern to solve this problem. these patterns seem just the perfect fix for this problem.

an example for interpreter-design-pattern, comment if you need an example for chain-of-responsibility-design-pattern

Client:

public Integer wordCountOnPage (Page samplePage) 
{
    ArrayList list = new ArrayList<Page>();
    list.add(new XmlPage());

    list.add(new HtmlPage());

    list.add(new JsonPage());

    for (int index = 0 ; index < list.size(); index ++)

    {
        Page eachPage = (Page) list.get(index); 
        if (eachPage.intercept(samplePage)){
            Integer i = eachPage.wordCountOnPage(samplePage);
        }

    }
    return 1;
}


public class XmlPage implements Page {

@Override
public Boolean intercept(Page page) {
    // TODO Auto-generated method stub
    return page instanceof XmlPage;
}

@Override
public Integer wordCountOnPage(Page page) {
    // TODO Auto-generated method stub
    return null;
}

}

public interface Page {

Boolean intercept(Page page);
Integer wordCountOnPage(Page page);
}

Answer 4

My approach would be to create an interface FileSearcher with a single method that performs the business logic. Create one implementation of this interface per file type. Then, create a registry which maps subclasses of SimpleFile to their corresponding implementation of FileSearcher. In your search file method, rather than doing the instanceof checks, look up the matching FileSeacher implementation at runtime and delegate to it.

With this implementation you may run into trouble when there are subclasses of HtmlFile etc., because you would have to map HtmlFileSearcher to all subclasses of HtmlFile. In that case, add another method to FileSearcher, named canHandle(Class<SimpleFile> runtimeClass) that allows FileSearcher implementations to signal what file class they understand. In the registry, rather than looking it up in the map, walk over all registered FileSearchers.