Keiron Liddle wrote:
As far as I am concerned it is largely irrelevant whether the particular
layout design is 100% correct.
There will never be a design which is 100% correct. However, some designs
are easier to comprehend than others. The HEAD design has a few stumbling
blocks for beginners.
Let's start with LMiter. The class name is not only badly abbreviated,
it is also misleading if spelled out a LayoutManagerIterator. Commonly,
iterators are named after the collection the are iterating, not after
the data they expect to be in the collection. The main problem is that
the collection with said LayoutManagers is purely virtual and never
exists as an object, which makes it very hard to grasp where the
layout managers come from. This severly disrupts tracking the control
flow. Furthermore, just looking at the LMiter source it appears to be
horrible, for once because of the creation with a base iterator, and
secondly the because of the preLoadNext() which is not called in next().
This indicates that some interesting things happen outside of the LMiter
class and a that encapsulation is violated. Also, from the implementation
of LMiter it is *still* not clear what's iterated here. Suggestion: either
implement a LayoutManagerList and an associated LayoutManagerListIterator
and make them self contained and transparently caching, or move the state
keeping and access into the LayoutManager itself.
There is also the BreakPosPossIterator which also doesn't iterate over a
tangible collection. In fact if you see
public void addAreas(PositionIterator parentIter, ...
there is no way to say what the heck is iterated here. Furthermore, this
iterator violates common Java iterator patterns, in particular
while ((childLM = breakPosIter.getNextChildLM()) != null) {
childLM.addAreas(breakPosIter, lc);
should better be
while (breakPosIter.hasNext()) {
childLM = breakPosIter.next().getChildLM();
childLM.addAreas(breakPosIter, lc);
}
Also, such an iterator is often created by something which is not the
collection iterated over but with constraints which represent a
subcollection of the collection iterated over:
PositionIterator breakPosIter =
new BreakPossPosIter(blockBreaks, iStartPos,
lfp.getLeafPos() + 1);
This makes it even harder to keep track where the iterator actually points
and what this means.
A third point, uncanny in itself but still minor overall is the distinction
between positions and break possiblities. This implies that either positions
are used for something else beside break possibilities, or break possibilities
can be something else than positions. But most of the source implies that
neither will ever be the case. Unfortunately, while examining a layout manager
implementation, the users sees first that a collection with break possiblities
is created, and then there is a loop over positions which adds areas. It would
be easier to understand if the same abstraction, break possibility, was used
in both parts of the code. One of the possiblities is to unify break
possiblities and positions, another would be to use break possibilities only
and retrieve positions from them as needed.
Now the biggest issue: the layout managers itself. At the first glance it is
not obvious why they should be first class objects at all, in particular as
a cursory examination of the code shows a one-to-one correspondence to FOs
both for classes and instances. Well, layout managers abstract the layout
*process*, however, the deep layout manager class hierarchy is mainly designed
around code reuse, which makes it really hard to understand. There is a reason
that design rules recommend against overuse of inheritance for code reuse and
deep inheritance hierarchies. There is only so much someone can hold in the
brain. Nevertheless, this is something I don't know how to fix on the cheap.
HTH
J.Pietschmann
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, email: [EMAIL PROTECTED]
