ApacheFopFactory.java refactoring ideas

[Adrian Crum]

Currently, the ApacheFopFactory class does nothing more than return a FopFactory class instance.
Looking through the project, I see a lot of redundant code to render documents via FOP. I'd like to
refactor the ApacheFopFactory to include some helper methods that would eliminate much of the
redundant code.

Here is what I have in mind:

1. Rename ApacheFopFactory to something like ApacheFopWorker, OR create a new class that references
ApacheFopFactory.
2. Move a lot of the stream preparation and rendering code to ApacheFopWorker.
3. Update existing OFBiz code to use the new class.

There are two things I can see being accomplished:

1. Code reduction.
2. Decouple FOP code from the multitude of classes that currently use it. If OFBiz code only called
ApacheFopWorker class methods, then the code base would be shielded from FOP API changes. If the FOP
API changes, only the ApacheFopWorker would have to be updated.

It would be the same basic concept as the current FreeMarkerWorker class.

What do you think?

-Adrian

[jonwimp]

Nice!

I would recommend keeping ApacheFopFactory, and then creating ApacheFopWorker to use
ApacheFopFactory. Keep the 2 de-coupled; we never know when we gotta extend ApacheFopFactory.

 > If the FOP API changes, only the ApacheFopWorker would have to be updated.

Oh, you're so right. I did some work for an Opentaps client who blindly updated his Opentaps
installation with both Opentaps and OFBiz recent updates. Yup, wrong medication and complications
all over the place (hence my recent post with "OFBiz updates as medication" metaphor).

One sticky issue was the old FOP in his Opentaps. His PDFs weren't showing up correctly (or was it
just barcodes?). I had a difficult time upgrading his codes to support a newer FOP.

Your ApacheFopWorker will save us a lot of trouble as OFBiz grows together with FOP.

Jonathon

Adrian Crum wrote:

> Currently, the ApacheFopFactory class does nothing more than return a
> FopFactory class instance. Looking through the project, I see a lot of
> redundant code to render documents via FOP. I'd like to refactor the
> ApacheFopFactory to include some helper methods that would eliminate
> much of the redundant code.
>
> Here is what I have in mind:
>
> 1. Rename ApacheFopFactory to something like ApacheFopWorker, OR create
> a new class that references ApacheFopFactory.
> 2. Move a lot of the stream preparation and rendering code to
> ApacheFopWorker.
> 3. Update existing OFBiz code to use the new class.
>
> There are two things I can see being accomplished:
>
> 1. Code reduction.
> 2. Decouple FOP code from the multitude of classes that currently use
> it. If OFBiz code only called ApacheFopWorker class methods, then the
> code base would be shielded from FOP API changes. If the FOP API
> changes, only the ApacheFopWorker would have to be updated.
>
> It would be the same basic concept as the current FreeMarkerWorker class.
>
> What do you think?
>
> -Adrian
>
>

[Jacques Le Roux]

Yes, good idea. I agree with Jonathon in keeping ApacheFopFactory or paraphrasing Adrian "create a new class that references
ApacheFopFactory"

Jacques

De : "Jonathon -- Improov" <[hidden email]>

> Nice!
>
> I would recommend keeping ApacheFopFactory, and then creating ApacheFopWorker to use
> ApacheFopFactory. Keep the 2 de-coupled; we never know when we gotta extend ApacheFopFactory.
>
>  > If the FOP API changes, only the ApacheFopWorker would have to be updated.
>
> Oh, you're so right. I did some work for an Opentaps client who blindly updated his Opentaps
> installation with both Opentaps and OFBiz recent updates. Yup, wrong medication and complications
> all over the place (hence my recent post with "OFBiz updates as medication" metaphor).
>
> One sticky issue was the old FOP in his Opentaps. His PDFs weren't showing up correctly (or was it
> just barcodes?). I had a difficult time upgrading his codes to support a newer FOP.
>
> Your ApacheFopWorker will save us a lot of trouble as OFBiz grows together with FOP.
>
> Jonathon
>
> Adrian Crum wrote:
> > Currently, the ApacheFopFactory class does nothing more than return a
> > FopFactory class instance. Looking through the project, I see a lot of
> > redundant code to render documents via FOP. I'd like to refactor the
> > ApacheFopFactory to include some helper methods that would eliminate
> > much of the redundant code.
> >
> > Here is what I have in mind:
> >
> > 1. Rename ApacheFopFactory to something like ApacheFopWorker, OR create
> > a new class that references ApacheFopFactory.
> > 2. Move a lot of the stream preparation and rendering code to
> > ApacheFopWorker.
> > 3. Update existing OFBiz code to use the new class.
> >
> > There are two things I can see being accomplished:
> >
> > 1. Code reduction.
> > 2. Decouple FOP code from the multitude of classes that currently use
> > it. If OFBiz code only called ApacheFopWorker class methods, then the
> > code base would be shielded from FOP API changes. If the FOP API
> > changes, only the ApacheFopWorker would have to be updated.
> >
> > It would be the same basic concept as the current FreeMarkerWorker class.
> >
> > What do you think?
> >
> > -Adrian
> >
> >
>

[Adrian Crum]

Jonathon and Jacques,

Thanks for the input!

Actually, I ended up going the other way around - ApacheFopFactory.instance() does nothing more than
call ApacheFopWorker.getFactoryInstance().

I'll have a patch submitted to Jira in a few days for everyone to review.

-Adrian

Jacques Le Roux wrote:

> Yes, good idea. I agree with Jonathon in keeping ApacheFopFactory or paraphrasing Adrian "create a new class that references
> ApacheFopFactory"
>
> Jacques
>
> De : "Jonathon -- Improov" <[hidden email]>
>
>>Nice!
>>
>>I would recommend keeping ApacheFopFactory, and then creating ApacheFopWorker to use
>>ApacheFopFactory. Keep the 2 de-coupled; we never know when we gotta extend ApacheFopFactory.
>>
>> > If the FOP API changes, only the ApacheFopWorker would have to be updated.
>>
>>Oh, you're so right. I did some work for an Opentaps client who blindly updated his Opentaps
>>installation with both Opentaps and OFBiz recent updates. Yup, wrong medication and complications
>>all over the place (hence my recent post with "OFBiz updates as medication" metaphor).
>>
>>One sticky issue was the old FOP in his Opentaps. His PDFs weren't showing up correctly (or was it
>>just barcodes?). I had a difficult time upgrading his codes to support a newer FOP.
>>
>>Your ApacheFopWorker will save us a lot of trouble as OFBiz grows together with FOP.
>>
>>Jonathon
>>
>>Adrian Crum wrote:
>>
>>>Currently, the ApacheFopFactory class does nothing more than return a
>>>FopFactory class instance. Looking through the project, I see a lot of
>>>redundant code to render documents via FOP. I'd like to refactor the
>>>ApacheFopFactory to include some helper methods that would eliminate
>>>much of the redundant code.
>>>
>>>Here is what I have in mind:
>>>
>>>1. Rename ApacheFopFactory to something like ApacheFopWorker, OR create
>>>a new class that references ApacheFopFactory.
>>>2. Move a lot of the stream preparation and rendering code to
>>>ApacheFopWorker.
>>>3. Update existing OFBiz code to use the new class.
>>>
>>>There are two things I can see being accomplished:
>>>
>>>1. Code reduction.
>>>2. Decouple FOP code from the multitude of classes that currently use
>>>it. If OFBiz code only called ApacheFopWorker class methods, then the
>>>code base would be shielded from FOP API changes. If the FOP API
>>>changes, only the ApacheFopWorker would have to be updated.
>>>
>>>It would be the same basic concept as the current FreeMarkerWorker class.
>>>
>>>What do you think?
>>>
>>>-Adrian
>>>
>>>
>>
>
>

[jonwimp]

Seems a bit counter-intuitive, that design pattern. Or we could rename ApacheFopWorker to
ApacheFopFactoryHelper.

In any case, as long as we're decoupling correctly, the code organization will be crisp and clear,
easy to extend in future. We can deal with the names and delegation later, if we want to.

Jonathon

Adrian Crum wrote:

> Jonathon and Jacques,
>
> Thanks for the input!
>
> Actually, I ended up going the other way around -
> ApacheFopFactory.instance() does nothing more than call
> ApacheFopWorker.getFactoryInstance().
>
> I'll have a patch submitted to Jira in a few days for everyone to review.
>
> -Adrian
>
> Jacques Le Roux wrote:
>> Yes, good idea. I agree with Jonathon in keeping ApacheFopFactory or
>> paraphrasing Adrian "create a new class that references
>> ApacheFopFactory"
>>
>> Jacques
>>
>> De : "Jonathon -- Improov" <[hidden email]>
>>
>>> Nice!
>>>
>>> I would recommend keeping ApacheFopFactory, and then creating
>>> ApacheFopWorker to use
>>> ApacheFopFactory. Keep the 2 de-coupled; we never know when we gotta
>>> extend ApacheFopFactory.
>>>
>>> > If the FOP API changes, only the ApacheFopWorker would have to be
>>> updated.
>>>
>>> Oh, you're so right. I did some work for an Opentaps client who
>>> blindly updated his Opentaps
>>> installation with both Opentaps and OFBiz recent updates. Yup, wrong
>>> medication and complications
>>> all over the place (hence my recent post with "OFBiz updates as
>>> medication" metaphor).
>>>
>>> One sticky issue was the old FOP in his Opentaps. His PDFs weren't
>>> showing up correctly (or was it
>>> just barcodes?). I had a difficult time upgrading his codes to
>>> support a newer FOP.
>>>
>>> Your ApacheFopWorker will save us a lot of trouble as OFBiz grows
>>> together with FOP.
>>>
>>> Jonathon
>>>
>>> Adrian Crum wrote:
>>>
>>>> Currently, the ApacheFopFactory class does nothing more than return a
>>>> FopFactory class instance. Looking through the project, I see a lot of
>>>> redundant code to render documents via FOP. I'd like to refactor the
>>>> ApacheFopFactory to include some helper methods that would eliminate
>>>> much of the redundant code.
>>>>
>>>> Here is what I have in mind:
>>>>
>>>> 1. Rename ApacheFopFactory to something like ApacheFopWorker, OR create
>>>> a new class that references ApacheFopFactory.
>>>> 2. Move a lot of the stream preparation and rendering code to
>>>> ApacheFopWorker.
>>>> 3. Update existing OFBiz code to use the new class.
>>>>
>>>> There are two things I can see being accomplished:
>>>>
>>>> 1. Code reduction.
>>>> 2. Decouple FOP code from the multitude of classes that currently use
>>>> it. If OFBiz code only called ApacheFopWorker class methods, then the
>>>> code base would be shielded from FOP API changes. If the FOP API
>>>> changes, only the ApacheFopWorker would have to be updated.
>>>>
>>>> It would be the same basic concept as the current FreeMarkerWorker
>>>> class.
>>>>
>>>> What do you think?
>>>>
>>>> -Adrian
>>>>
>>>>
>>>
>>
>>
>
>

[Adrian Crum]

Jonathon -- Improov wrote:
> Seems a bit counter-intuitive, that design pattern. Or we could rename
> ApacheFopWorker to ApacheFopFactoryHelper.

You're right, but my thinking is the ApacheFopFactory class could be phased out. From my
perspective, it seems kind of silly to have a class that does nothing more than return an instance
of another class.

-Adrian

[Jacopo Cappellato]

Adrian,

I've not followed the details of your design, and so sorry if my comment
is out of scope here... the main reason for the ApacheFopFactory class
is that it implements a singleton.
Maybe you have already addressed this in another class but it was worth
of a mention.

Jacopo



Adrian Crum wrote:
> Jonathon -- Improov wrote:
>> Seems a bit counter-intuitive, that design pattern. Or we could rename
>> ApacheFopWorker to ApacheFopFactoryHelper.
>
> You're right, but my thinking is the ApacheFopFactory class could be
> phased out. From my perspective, it seems kind of silly to have a class
> that does nothing more than return an instance of another class.
>
> -Adrian

[Adrian Crum]

Jacopo Cappellato wrote:
> Adrian,
>
> I've not followed the details of your design, and so sorry if my comment
> is out of scope here... the main reason for the ApacheFopFactory class
> is that it implements a singleton.
> Maybe you have already addressed this in another class but it was worth
> of a mention.
>
> Jacopo

Yes, that is addressed in the new class. To summarize: I created a worker class that eliminates a
lot of redundant FOP code, and that worker class includes the singleton FopFactory instance.

-Adrian

[jonwimp]

Adrian,

The ApacheFopFactory also handles some stuff like fopPath, fop config file, base fonts defaults, etc.

Such attributes should be tied to the FopFactory. They certainly are not attributes of a Worker class.

The point of having a FopFactory class distinct from a Worker class is to cleanly encapsulate
Factory-related attributes inside a single entity. If we start putting Factory-related attributes
into the Worker class, we'd have a Worker that is overworked with a gigantic scope of maybe these
2 things:

1. Manage setup and calls to the Factory.

2. Manage Factory attributes.

The singleton thing can be achieved with a static member "fopFactory" inside the Worker. The
FopFactory must be a singleton in OFBiz, true. But that's not the point here about separating
Factory and Worker. It's about clean (and intuitive and logical) delegation of responsibilities to
Factory and Worker.

Jonathon

Adrian Crum wrote:

> Jacopo Cappellato wrote:
>> Adrian,
>>
>> I've not followed the details of your design, and so sorry if my
>> comment is out of scope here... the main reason for the
>> ApacheFopFactory class is that it implements a singleton.
>> Maybe you have already addressed this in another class but it was
>> worth of a mention.
>>
>> Jacopo
>
> Yes, that is addressed in the new class. To summarize: I created a
> worker class that eliminates a lot of redundant FOP code, and that
> worker class includes the singleton FopFactory instance.
>
> -Adrian
>
>

[Adrian Crum]

Jonathon,

Thank you very much for your input! All of the code that was in ApacheFopFactory is now in the
ApacheFopWorker.getFactoryInstance() method. It's not a big change and everything still works the same.

Getting singleton instances of classes from worker classes is done in other places in OFBiz
(FreeMarkerWorker.getDefaultOfbizConfig() for example), so I don't see a problem with doing the same
thing in this case.

-Adrian

Jonathon -- Improov wrote:

> Adrian,
>
> The ApacheFopFactory also handles some stuff like fopPath, fop config
> file, base fonts defaults, etc.
>
> Such attributes should be tied to the FopFactory. They certainly are not
> attributes of a Worker class.
>
> The point of having a FopFactory class distinct from a Worker class is
> to cleanly encapsulate Factory-related attributes inside a single
> entity. If we start putting Factory-related attributes into the Worker
> class, we'd have a Worker that is overworked with a gigantic scope of
> maybe these 2 things:
>
> 1. Manage setup and calls to the Factory.
>
> 2. Manage Factory attributes.
>
> The singleton thing can be achieved with a static member "fopFactory"
> inside the Worker. The FopFactory must be a singleton in OFBiz, true.
> But that's not the point here about separating Factory and Worker. It's
> about clean (and intuitive and logical) delegation of responsibilities
> to Factory and Worker.
>
> Jonathon
>
> Adrian Crum wrote:
>
>> Jacopo Cappellato wrote:
>>
>>> Adrian,
>>>
>>> I've not followed the details of your design, and so sorry if my
>>> comment is out of scope here... the main reason for the
>>> ApacheFopFactory class is that it implements a singleton.
>>> Maybe you have already addressed this in another class but it was
>>> worth of a mention.
>>>
>>> Jacopo
>>
>>
>> Yes, that is addressed in the new class. To summarize: I created a
>> worker class that eliminates a lot of redundant FOP code, and that
>> worker class includes the singleton FopFactory instance.
>>
>> -Adrian
>>
>>
>
>

[Adrian Crum]

Okay, it's done:

https://issues.apache.org/jira/browse/OFBIZ-1414

Comments welcome.

-Adrian

Adrian Crum wrote:

> Currently, the ApacheFopFactory class does nothing more than return a
> FopFactory class instance. Looking through the project, I see a lot of
> redundant code to render documents via FOP. I'd like to refactor the
> ApacheFopFactory to include some helper methods that would eliminate
> much of the redundant code.
>
> Here is what I have in mind:
>
> 1. Rename ApacheFopFactory to something like ApacheFopWorker, OR create
> a new class that references ApacheFopFactory.
> 2. Move a lot of the stream preparation and rendering code to
> ApacheFopWorker.
> 3. Update existing OFBiz code to use the new class.
>
> There are two things I can see being accomplished:
>
> 1. Code reduction.
> 2. Decouple FOP code from the multitude of classes that currently use
> it. If OFBiz code only called ApacheFopWorker class methods, then the
> code base would be shielded from FOP API changes. If the FOP API
> changes, only the ApacheFopWorker would have to be updated.
>
> It would be the same basic concept as the current FreeMarkerWorker class.
>
> What do you think?
>
> -Adrian
>
>

[jonwimp]

If it works, that's all that matters to me.

When and if we see a significant divergence in these 2 things:

1. How a FopFactory is gotten, along with possibly tons of attributes and config

2. How a FopFactory is used, along with possibly complex run-time parameters

then we talk about splitting the ApacheFopFactory and the Fop Worker.

I rest my case.

Jonathon

Adrian Crum wrote:

> Okay, it's done:
>
> https://issues.apache.org/jira/browse/OFBIZ-1414
>
> Comments welcome.
>
> -Adrian
>
> Adrian Crum wrote:
>
>> Currently, the ApacheFopFactory class does nothing more than return a
>> FopFactory class instance. Looking through the project, I see a lot of
>> redundant code to render documents via FOP. I'd like to refactor the
>> ApacheFopFactory to include some helper methods that would eliminate
>> much of the redundant code.
>>
>> Here is what I have in mind:
>>
>> 1. Rename ApacheFopFactory to something like ApacheFopWorker, OR
>> create a new class that references ApacheFopFactory.
>> 2. Move a lot of the stream preparation and rendering code to
>> ApacheFopWorker.
>> 3. Update existing OFBiz code to use the new class.
>>
>> There are two things I can see being accomplished:
>>
>> 1. Code reduction.
>> 2. Decouple FOP code from the multitude of classes that currently use
>> it. If OFBiz code only called ApacheFopWorker class methods, then the
>> code base would be shielded from FOP API changes. If the FOP API
>> changes, only the ApacheFopWorker would have to be updated.
>>
>> It would be the same basic concept as the current FreeMarkerWorker class.
>>
>> What do you think?
>>
>> -Adrian
>>
>>
>
>