Am I implementing a generics-based Java factory correctly?

Question

I don't believe I am implementing the factory pattern correctly because the Application class' createDocument method accepts any class type, not just subclasses of Document.

In other words, is there a way I can restrict the createDocument method to only accept subclasses of Document?

• Document.java

  package com.example.factory;
  
  public abstract class Document {
      public Document() {
          System.out.println("New Document instance created: " + this.toString());
      }
  }
  

• DrawingDocument.java

  package com.example.factory
  
  public class DrawingDocument extends Document {
      public DrawingDocument() {
          System.out.println("New DrawingDocument instance created: " this.toString());
      }
  }
  

• Application.java

  package com.example.factory;
  
  public class Application {
      public <T> T createDocument(Class<T> documentClass) {
          try {
              return documentClass.newInstance();
          } catch (InstantiationException e) {
              throw new IllegalArgumentException(e);
          } catch (IllegalAccessException e) {
              throw new IllegalArgumentException(e);
          }
      };
  }
  

• Main.java

  package com.example.factory;
  
  public static void main(String[] args) {
      Application application = new Application();
      application.createDocument(DrawingDocument.class);
  }
  

Achromaticity

any reason to believe why you're not? – Cochran

You're returning a new instance and then immediately ignoring it. Is that intentional? – Hibernal

As I'm logging the instance creation, I assume the document would be used but in this example I have no need for it. – Achromaticity

What is your motivation for this? Why do you need a factory? From the code you have posted I can't see any advantage compared to using the constructor of the document class directly. Will the application create instances of subclasses of the given class at some later point or will it have to do extensive initialization? – Oversight

Answer 1

You should bound your generic so that it is only using T's that inherit Document. example:

public class Application {
    //Add extends Document after T
    public static <T extends Document> T createDocument(Class<T> documentClass) throws InstantiationException, IllegalAccessException {
        return documentClass.newInstance();
    };
}

Atwater

Beautiful, just what I was looking for! Apparently I need to wait three minutes to accept your answer, but I will do so once I am able. – Achromaticity

Answer 2

The code looks good. In a real implementation the factory method should not be declared to throw any of the reflection-related exceptions. And you will probably have some different code anyway to create the document.

The faxtory method should take a Class<? extends Document> as its parameter, so that one cannot ask it to create a String, for example.

[update:] Code sample:

public Document createDocument(Class<? extends Document> clazz) {
  try {
    return clazz.newInstance();
  } catch (InstantiationException e) {
    throw new IllegalArgumentException(e);
  }
}

Miamiami

Do you propose the try/catch block in the factory method? – Achromaticity

Yes, that would be good. You could encapsulate such an exception in an IllegalArgumentException and throw that instead. – Miamiami

Hmm... how would I go about implementing that refactoring? The try block would surround the return statement... :S – Achromaticity

Ah, so you're saying throw the IllegalArgumentException if the return throws the reflection exceptions? – Achromaticity

That code update you did... I'm wondering if my initial approach was better. Does this not require a cast from Document for subclasses? I have the following now: snipt.org/xPpp – Achromaticity

By the way, it was completely intentional that I didn't catch Exception but only InstantiationException. That way, any individual exceptions that might be thrown by the constructors themselves would be preserved as such. – Miamiami

Yeah, good point. I changed my code to catch the InstatiationException and the IllegalAccessException exceptions explicitly (updated my snippet). – Achromaticity

Answer 3

Where is the restriction to Document type in the factory ? Try

public <T extends Document> T createDocument(Class<T> documentClass) throws InstantiationException, IllegalAccessException {
    return documentClass.newInstance();
};

Evanesce