Q - Internal API preference? (It started with a 'simple' TODO...)

[Andreas Delmelle <an...@telenet.be>]

Hi FOP devs and other interested parties,

Apologies in advance for the rather long post...

Note - If code structure and style is not your thing, feel free to ignore the whole post, otherwise, you may just want to go get a drink and some snacks, and bear with me. ;)

A few weeks ago, as I started browsing the codebase to re-familiarize myself, I noticed a TODO in a comment in the layoutmgr.KnuthSequence class, decided to have a crack at resolving it, and... 
Well, after spending some hours reshuffling things, I have already triggered and dealt with enough cascading changes that I am seriously thinking about committing it all to a branch.

One of the things that got me wondering is the extensive usage of the "standard" Java Collections API. Admitted, it's all very convenient to add new code for newcomers. It is relatively easy to donate patches if you are familiar with the basic Java API and have 'example' code blocks a few lines up or in the superclass... 
On the other hand, over time, as more and more new pieces got added, and these patterns got basically copy-pasted all over the place, I feel this convenience may have actually made the code *less* comprehensible overall.

What I was thinking --and what may have prompted someone[*] else to put that TODO there in the first place-- is that the layout engine, internally, could be refactored to rely *entirely* upon KnuthSequences, in turn extracted as an interface.
Explicitly avoid implementing or extending the List interface there, and instead, just create a basic AbstractKnuthSequence implementation that serves as a wrapper around a java.util.List, encapsulating all the List interactions into proprietary methods, rather than implementing the List interface itself.

That way, the methods defined in the interface and the base class can be (re)designed and named such, that FOP's own code in the LayoutManagers may eventually become easier to read and follow (?)
If properly implemented, *any* List implementation can be passed to the AbstractKnuthSequence constructor, rather than always using an ArrayList, as it does now (which currently makes it a not-so-good idea to use KnuthSequence across the board).

Where we now have:

List contents = new java.util.$ListType();

This would become:

KnuthSequence contents = new $SequenceType( new java.util.$ListType() );

At the same time, that would also be virtually the only reference to the JCF, and the remainder of the method body would be written more in terms of the KnuthSequence interface. A Java 'dialect', if you will, that we then have complete control over.
If deemed necessary, a KnuthSequence can still provide a List view of itself, although I would make it read-only (i.e. Collections.unmodifiableList()). Any mutations (writes) would be handled via either a restricted set of API methods, or via an Iterator or ListIterator... 

So far, the improvements in my sandbox are minimal, but definitely already noticeable. Changes are still very localised, though, as I have not yet gotten around to trying to change this in high-impact areas (like the main LayoutManager interface). I have, in the meantime, adapted all the inline level LMs to work exclusively with KnuthSequences, which seems to work pretty well.

OTOH, I have not yet fully ironed out some measures I had to put in, to ease the transition, like making it possible for a KnuthSequence to behave like a ListElement, to be able to have a generic List<ListElement> backing the sequence.
Not entirely sure how best to tackle that one. Current solution: extracted ListElement interface and implemented it in AbstractKnuthSequence. There seemed to be only a handful of places in the layoutmgr.inline package where a switch from the interface to the class was really necessary to get it all working, so perhaps there is a better way. The concept of nested sequence-elements just looked kind of cool, for now... Maybe we should keep them, just for that. ;)

Now, before I move further towards the block level LMs, I thought I'd throw it out there, and see what comes back.

Thoughts, anyone? Any strong preferences (for or against)? Positive/negative experiences with such stuff (encapsulating standard Java API calls in proprietary interfaces vs. just sticking to plain ol' standard Java)?

Looking forward to your feedback,

[*] I traced it back to a commit made by Adrian, but cannot be sure if he himself added it, or whether the comment was put there by Alexander Kiel who submitted the patch. http://svn.apache.org/viewvc?view=revision&revision=825646


KR[**]

Andreas

[**] BTW, Clay pointed out to me off-list that this abbreviation may not be as common as I took it to be. For those wondering, it is short for "Kind Regards", although Clay's suggestion of "Keep it Real" would also work for me. :)

Re: Q - Internal API preference? (It started with a 'simple' TODO...)

[Andreas Delmelle <an...@telenet.be>]

Hi Glenn

> On 27 May 2015, at 18:36, Glenn Adams <gl...@skynav.com> wrote:
> 
> One significant risk in a major rewrite of an important section is regression. This is particularly true given the paucity of tests in FOP. You will have to assume that such a rewrite is going to produce a number of regressions (while continuing to pass the test suite).

Yep, also very true... 
To reduce that risk, it is likely best to follow up on the idea of creating that branch first, and give others a chance to have a look and run some local tests for a couple of months before merging it back after a proper vote.

After all, I am not so presumptuous as to consider myself infallible. As I recall, one other significant change I worked on locally, way back when, got committed to trunk directly and, benefits aside, turned out to cause quite some issues in production environments later on, after a major release, mainly due to insufficient regression testing (PropertyCache, anyone? :/)

Rest assured that I will not make that same mistake twice.


Thanks!

KR

Andreas

Re: Q - Internal API preference? (It started with a 'simple' TODO...)

[Glenn Adams <gl...@skynav.com>]

One significant risk in a major rewrite of an important section is
regression. This is particularly true given the paucity of tests in FOP.
You will have to assume that such a rewrite is going to produce a number of
regressions (while continuing to pass the test suite).

On Wed, May 27, 2015 at 9:45 AM, Andreas Delmelle <
andreas.delmelle@telenet.be> wrote:

> > On 27 May 2015, at 01:59, Luis Bernardo <lm...@gmail.com> wrote:
> >
>
> Hi Luis
>
> >
> > In my view any code that does more becomes more complex, not less
> comprehensible. The same happens with FOP.
>
> Very true, indeed. I recall having argued that same point on this list in
> the past.
> "Doing more with less" is, IMO, executive lingo that is almost always
> meant to hide some uglier truths... ;)
>
> It is more a matter of making sure that new code that gets added will
> follow the "right" path, if you will (whatever that may mean). No criticism
> there. After all, I *was* part of the team for years, so know full well to
> what extent time and resource constraints have influenced some of those
> decisions.
>
> > If you want to rewrite the layout engine and do it with less code then
> go for it. You will get a +1 from me.
>
> OK, good to know.
>
> Just to be clear: it is not so much a 'rewrite' as it is optimising or
> streamlining what is already there, and it remains to be seen whether it
> will literally lead to 'less' code, since it would require introduction of
> a few new interfaces/classes. Overall, I would expect the total LOC to go
> up, slightly, but I have not yet finished so cannot say anything for sure
> there at this point.
>
> Thanks!
>
>
> KR
>
> Andreas
>

Re: Q - Internal API preference? (It started with a 'simple' TODO...)

[Andreas Delmelle <an...@telenet.be>]

> On 27 May 2015, at 01:59, Luis Bernardo <lm...@gmail.com> wrote:
> 

Hi Luis

> 
> In my view any code that does more becomes more complex, not less comprehensible. The same happens with FOP.

Very true, indeed. I recall having argued that same point on this list in the past. 
"Doing more with less" is, IMO, executive lingo that is almost always meant to hide some uglier truths... ;)

It is more a matter of making sure that new code that gets added will follow the "right" path, if you will (whatever that may mean). No criticism there. After all, I *was* part of the team for years, so know full well to what extent time and resource constraints have influenced some of those decisions.

> If you want to rewrite the layout engine and do it with less code then go for it. You will get a +1 from me.

OK, good to know.

Just to be clear: it is not so much a 'rewrite' as it is optimising or streamlining what is already there, and it remains to be seen whether it will literally lead to 'less' code, since it would require introduction of a few new interfaces/classes. Overall, I would expect the total LOC to go up, slightly, but I have not yet finished so cannot say anything for sure there at this point.

Thanks!


KR

Andreas

Re: Q - Internal API preference? (It started with a 'simple' TODO...)

[Luis Bernardo <lm...@gmail.com>]

In my view any code that does more becomes more complex, not less 
comprehensible. The same happens with FOP.

If you want to rewrite the layout engine and do it with less code then 
go for it. You will get a +1 from me.

On 5/26/15 8:23 PM, Andreas Delmelle wrote:
> Hi FOP devs and other interested parties,
>
> Apologies in advance for the rather long post...
>
> Note - If code structure and style is not your thing, feel free to ignore the whole post, otherwise, you may just want to go get a drink and some snacks, and bear with me. ;)
>
> A few weeks ago, as I started browsing the codebase to re-familiarize myself, I noticed a TODO in a comment in the layoutmgr.KnuthSequence class, decided to have a crack at resolving it, and...
> Well, after spending some hours reshuffling things, I have already triggered and dealt with enough cascading changes that I am seriously thinking about committing it all to a branch.
>
> One of the things that got me wondering is the extensive usage of the "standard" Java Collections API. Admitted, it's all very convenient to add new code for newcomers. It is relatively easy to donate patches if you are familiar with the basic Java API and have 'example' code blocks a few lines up or in the superclass...
> On the other hand, over time, as more and more new pieces got added, and these patterns got basically copy-pasted all over the place, I feel this convenience may have actually made the code *less* comprehensible overall.
>
> What I was thinking --and what may have prompted someone[*] else to put that TODO there in the first place-- is that the layout engine, internally, could be refactored to rely *entirely* upon KnuthSequences, in turn extracted as an interface.
> Explicitly avoid implementing or extending the List interface there, and instead, just create a basic AbstractKnuthSequence implementation that serves as a wrapper around a java.util.List, encapsulating all the List interactions into proprietary methods, rather than implementing the List interface itself.
>
> That way, the methods defined in the interface and the base class can be (re)designed and named such, that FOP's own code in the LayoutManagers may eventually become easier to read and follow (?)
> If properly implemented, *any* List implementation can be passed to the AbstractKnuthSequence constructor, rather than always using an ArrayList, as it does now (which currently makes it a not-so-good idea to use KnuthSequence across the board).
>
> Where we now have:
>
> List contents = new java.util.$ListType();
>
> This would become:
>
> KnuthSequence contents = new $SequenceType( new java.util.$ListType() );
>
> At the same time, that would also be virtually the only reference to the JCF, and the remainder of the method body would be written more in terms of the KnuthSequence interface. A Java 'dialect', if you will, that we then have complete control over.
> If deemed necessary, a KnuthSequence can still provide a List view of itself, although I would make it read-only (i.e. Collections.unmodifiableList()). Any mutations (writes) would be handled via either a restricted set of API methods, or via an Iterator or ListIterator...
>
> So far, the improvements in my sandbox are minimal, but definitely already noticeable. Changes are still very localised, though, as I have not yet gotten around to trying to change this in high-impact areas (like the main LayoutManager interface). I have, in the meantime, adapted all the inline level LMs to work exclusively with KnuthSequences, which seems to work pretty well.
>
> OTOH, I have not yet fully ironed out some measures I had to put in, to ease the transition, like making it possible for a KnuthSequence to behave like a ListElement, to be able to have a generic List<ListElement> backing the sequence.
> Not entirely sure how best to tackle that one. Current solution: extracted ListElement interface and implemented it in AbstractKnuthSequence. There seemed to be only a handful of places in the layoutmgr.inline package where a switch from the interface to the class was really necessary to get it all working, so perhaps there is a better way. The concept of nested sequence-elements just looked kind of cool, for now... Maybe we should keep them, just for that. ;)
>
> Now, before I move further towards the block level LMs, I thought I'd throw it out there, and see what comes back.
>
> Thoughts, anyone? Any strong preferences (for or against)? Positive/negative experiences with such stuff (encapsulating standard Java API calls in proprietary interfaces vs. just sticking to plain ol' standard Java)?
>
> Looking forward to your feedback,
>
> [*] I traced it back to a commit made by Adrian, but cannot be sure if he himself added it, or whether the comment was put there by Alexander Kiel who submitted the patch. http://svn.apache.org/viewvc?view=revision&revision=825646
>
>
> KR[**]
>
> Andreas
>
> [**] BTW, Clay pointed out to me off-list that this abbreviation may not be as common as I took it to be. For those wondering, it is short for "Kind Regards", although Clay's suggestion of "Keep it Real" would also work for me. :)