[jdom-commits] Follow-on to FilterList

[Jason Hunter]

FYI, for those looking at the FilterList code that went into the last
check-in, we have two areas that can still be cleaned up a little.  Here
are two questions I asked Alex Rosen and his answers...

-jh-

> In Document.java there are many checks regarding parentage and type.
> Shouldn't these be offloaded into FilterList now?

Element.java also, right?. That was my idea behind the "live list"
concept.
Currently, Element and Document store their children in regular
ArrayLists,
which means that there two list-modification entry points - the Element
and
Document methods like addContent() and removeContent(), and the
FilterList
methods. Both must do the same checking, resulting in duplicate code.

If, instead E and D stored their children in a "live list" (e.g. a
FilterList kind of thing), then the E and D methods could just call
through
to the list methods, which would do the checking. This live list would
be
exactly the same one as would be returned by getContent().

Hmmm... actually, since AttributeFilter and ElementContentFilter and
DocumentContentFilter have their important code in static methods, I
guess
Element and Document can call these static methods directly, to
eliminate
most of the duplicate code.

> In FilterList.java there are two comments like this:
>
>         // XXX leaves list in an inconsistent state if checkAdd() fails
>         Iterator iter = c.iterator();
>         while (iter.hasNext()) {
>             checkAdd(iter.next());
>         }
>
> But isn't that no longer true since it looks like the data is
> pre-checked?

The comment isn't very clear. The problem is that checkAdd() both checks
if
it's OK, and adjusts the parentage. So if the last item is not OK, the
ones
before it will have their parent set, but will not actually be added as
a
child of the parent object. The solution is to either separate these two
functions into two methods, and pre-check all objects before making any
changes; or to do some (kinda ugly) post-failure cleanup, like
Element.setContent() currently does.