1

I have faced this problem a few times in the past, but haven't really found a good solution/design for it.

The below example code will generate PDF doc from Entity (Company or Article)

public class Entity
{
    int id;
}

public class Company extends Entity
{
    private String HQ;
}

public class Article extends Entity
{
    private String title;
}

public interface EntityPDFGenerator
{
    void generate(Entity entity);
}

public class ArticlePDFGenerator implements EntityPDFGenerator
{
    public void generate(Entity entity)
    {
        Article article = (Article) entity;
        // create Article related PDF from entity
    }
}

public class CompanyPDFGenerator implements EntityPDFGenerator
{
    public void generate(Entity entity)
    {
        Company company = (Company) entity;
        // create Company related PDF
    }
}

Main class:

public class PDFGenerator
{
    public void generate(Entity entity)
    {
        EntityPDFGenerator pdfGenerator = getConcretePDFGenerator(entity);
        pdfGenerator.generate(entity);

    }

    // lets make the factory task simple for now
    EntityPDFGenerator getConcretePDFGenerator(Entity entity)
    {
        if(entity instanceof Article){
            return new ArticlePDFGenerator();
        }else{
            return new CompanyPDFGenerator();
        }
    }
}

In the above approach the problem is with the casting the Entity to the concrete type (casting can be dangerous in later stage of the code). I tried to make it with generics, but then I get the warning

Unchecked call to 'generate(T)'

Can I improve this code?

jaco0646
  • 15,303
  • 7
  • 59
  • 83
gubak
  • 343
  • 1
  • 4
  • 9

3 Answers3

1

Here, you go with the suggested changes:

public interface EntityPDFGenerator<T extends Entity> {
    void generate(T entity);
}


public class ArticlePDFGenerator implements EntityPDFGenerator<Article> {

    public void generate(Article entity)
    {
        // create Article related PDF from entity
    }
}


public class CompanyPDFGenerator implements EntityPDFGenerator<Company> {

    public void generate(Company entity)
    {
        // create Company related PDF
    }
}
A_C
  • 905
  • 6
  • 18
  • Thanks a lot for your answer. Question: How can I dynamically instantiate final PDF generators based on Entity type, lets say over a Factory? – gubak Nov 27 '18 at 15:14
  • @gubak that's a different question, yes. The accepted answer points to the right approach. Also, if you want to have many different types of instances to be created via the factory, then you can refer this link https://stackoverflow.com/questions/52950426/dynamic-reference-casting-depending-on-actual-object-type – A_C Nov 28 '18 at 14:19
0

I would suggest to move the getGenerator() method in the Entity class and override it in the Company and Article classes. Unless, of course, there is a good reason not to.

0

Short answer

Generics is not the right tool here. You can make the casting explicit:

public class CompanyPDFGenerator implements EntityPDFGenerator
{
    public void generate(Entity entity)
    {
        if (! (entity instanceof Company)) {
            throw new IllegalArgumentException("CompanyPDFGenerator works with Company object. You provided " + (entity == null ? "null" : entity.getClass().getName()));
        }
        Company company = (Company) entity;
        System.out.println(company);
        // create Company related PDF
    }
}

Or you can define some sort of data structure in the entity class and use only that in the printer:

public abstract class Entity
{
    int id;
    public abstract EntityPdfData getPdfData();
}

// ...

public class CompanyPDFGenerator implements EntityPDFGenerator
{
    public void generate(Entity entity)
    {
        EntityPdfData entityPdfData = entity.getPdfData();
        // create Company related PDF
    }
}

Long answer

Generics is useful if you know the types at compile-time. I.e. if you can write into your program that actual type. For lists it looks so simple:

// now you know at compile time that you need a list of integers
List<Integer> list = new ArrayList<>();

In your example you don't know that:

public void generate(Entity entity)
{
    // either Article or Company can come it. It's a general method
    EntityPDFGenerator pdfGenerator = getConcretePDFGenerator(entity);
    pdfGenerator.generate(entity);

}

Suppose you want to add type to the EntityPDFGenerator , like this:

public static interface EntityPDFGenerator<T extends Entity>
{
    void generate(T entity);
}

public static class ArticlePDFGenerator implements EntityPDFGenerator<Article>
{
    public void generate(Article entity)
    {
        Article article = (Article) entity;
        // create Article related PDF from entity
    }
}

public static class CompanyPDFGenerator implements EntityPDFGenerator<Company>
{
    public void generate(Company entity)
    {
        Company company = (Company) entity;
        // create Company related PDF
    }
}

This looks nice. However, getting the right generator will be tricky. Java generics is invariant. Even ArrayList<Integer> is not a subclass of ArrayList<Number>. So, ArticlePdfGenerator is not a subclass of EntityPDFGenerator<T extends Entity>. I.e. this will not compile:

<T extends Entity> EntityPDFGenerator<T> getConcretePDFGenerator(T entity, Class<T> classToken)
{
    if(entity instanceof Article){
        return new ArticlePDFGenerator();
    }else{
        return new CompanyPDFGenerator();
    }
}
Tamas Rev
  • 7,008
  • 5
  • 32
  • 49
  • "Or you can define some sort of data structure in the entity class and use only that in the printer:" - As for me it's not the responsibility of an `Entity` to return something like `EntityPdfData`. And if there should be further output formats added (TXT, JPG) then for each of them a method has to be added to every `Entity` type? This isn't loosly coupled. – LuCio Nov 27 '18 at 16:14
  • @LuCio you're right in general. However, we know too little about the whole code base to make such decisions. Ideally, the `Entity` makes it easy to access all the required data. If it is so, then we don't need the getter. If it isn't so then we can have a getter, like `getGenericEntityData()`. – Tamas Rev Nov 27 '18 at 16:33