commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <skitch...@apache.org>
Subject Re: [digester2] Additions
Date Sat, 12 Feb 2005 09:26:20 GMT
On Sat, 2005-02-12 at 08:28 +0100, Oliver Zeigermann wrote:
> Hi Simon,
> 
> I have now committed the first proposal for the XMLIO like rule
> manager called SupplementaryRuleManager. I also added a very basic
> test for it. I had to make a minor visibility change in
> DefaultRuleManager that it extends. Path now has match methods.
> 
> All this isn't perfect, especially Javadoc is missing and tests need
> to be extended, but I wanted to make this public for discussion as
> soon as possible.
> 
> So, comments?


One design goal was to avoid using protected members. The problem here
is that using protected for a *member variable* locks us into an
implementation forever (a protected member is part of the "external" api
that users can access). I think it preferable to add a public or
protected method that provides access to the member rather than exposing
the member directly. The method can later get the data in some manner
other than reading from a member if we change the implementation. Note
that having a method that returns a Map isn't a *whole* lot better. I
think what we need is a "copy-helper" type method rather than exposing
the namespace member just so a subclass can copy it. I can't think what
the best solution is at the moment, but I feel there's an elegant (and
probably fairly well known) solution nearby. How does clone() solve this
issue?

Note that I have (I think) been rigorous about avoiding protected
members in the o.a.c.digester2 package, but not so rigorous in the
action subpackage. I think it probably is a good idea to apply this to
the action package too, though.

=== 

I would suggest that the match method be placed somewhere other than on
the Path class. Digester could well provide multiple different matching
implementations in future. I think a static method on some XMLIOUtils
class might be appropriate. The user's hand-written Action can then
choose whether to use that matching algorithm or some other. All that
Path currently does is return a String representing the "canonical form"
of the Path, and that seems clean & focussed to me.

=== 

This bit in SupplementaryRuleManager will cause problems I think:
    public List getMatchingActions(String path) {
        List actionList = super.getMatchingActions(path);
        ...
        if (supplementaryAction != null) {
            actionList.add(supplementaryAction);
        }
The list the RuleManager returns should not be modified; it is actually
an internal data structure. This does point out that DefaultRuleManager
should be a little more defensive, perhaps returning 
  Collections.unmodifiableList(actionList)
which would cause the above add call to fail and throw an exception.
That would, however, cause an object to be created on each
getMatchingActions call, which I would prefer to avoid if possible.
How about this instead?
    public List getMatchingActions(String path) {
        List actionList = super.getMatchingActions(path);
        ...
        if (supplementaryAction != null) {
            newActionList = new ArrayList(actionList);
            newActionList.add(supplementaryAction)
        }

I suppose this could be done instead:
        if (supplementaryAction != null) { 
            if (actionList.contains(supplementaryAction) {
              // nothing to do, we must have added the
              // supplementary action to this list earlier
            } else {
              // permanently modify DefaultRuleManager's 
              // internal data structure; yecch!
              actionList.add(action)
            }
        }
but as the comments indicate, I think that having one class manipulate
another's internal data is not in terribly good taste, nor easy to
maintain.

=== 

As I mentioned earlier, I'm happy to put the "fallbackAction"
functionality into DefaultRuleManager. 

=== 

Why is there only facility for one fallback action, and one
supplementary action? Wouldn't having a List of actions for each be no
more difficult and potentially more useful?

=== 

On a minor SVN note, the SVN property "svn:keywords" needs to be set on
a file in order to enable expansion of $Id$ into the appropriate text.
This can be done manually via:
  svn propset svn:keywords "Id" filename
It takes a commit to update the repository. I suggest this would be nice
to do next time you update the SupplementaryRuleManager class, otherwise
that text in the first line of the file will never change. Adding the
following to your ~/.subversion/config file will ensure that each .java
file that you add to subversion automatically gets $Id$ expansion
enabled:
  [auto-props]
  *.java = svn:keywords=Id



> And, by the, now that I have done some work and I came across some new
> questions. Maybe they are just dumb, but what do you think about this?
> 
> (1) Why aren't relative paths to actions also cached?

The current DefaultRuleManager implementation is straight from the old
RulesBase class, except that absolute paths are expected to start with
"/".

I have plans for a major rework of DefaultRuleManager. In particular, I
would love to support /foo/bar[id='1']  or similar. I plan to have a
look at STX (stx.sourceforge.net) to see if I can steal ideas from there
too.

> (2) Why are the guts of Context accessible to the Action as well? They
> should only be interesting to the Digester core, not to the
> application. Why not adding an interface that hides the implentation
> details? Like
> 
> public interface Context {
> Path getCurrentPath();
> String getMatchPath();
> 
> ClassLoader getClassLoader();
> Object getRoot();
> boolean isEmpty();
> int getStackSize();
> Object peek();
> Object peek(int n);
> 
> putItem(ItemId id, Object data);
> Object getItem(ItemId id);
> }
> 
> and ContextImpl being what Context is now. Similar thing with Path. If
> you agree I could do the work.

Yes, I thnk you're right that Context/ContextImpl would be sensible. In
addition, there is some stuff curently in Context that really is meant
for the SAXHandler only, not the Action classes, so that would allow us
to make that distinction. How about we let Context stabilise a bit
first, though? I've found it is the class I change most often, and if we
have interface and implementation for it, then that's two places to
change things.

Is Path really complex enough to need interface + implementation
separation? I tend to think not, but am open to persuasion.

> 
> (3) Bringing this up again: Wouldn't it be more flexible to allow more
> than a String as a pattern in RuleManager#addRule(String pattern,
> Action action)?

If you can give an example of a pattern that cannot be reasonably
represented as a string, I'll happily back an additional
RuleManager#addRule(Pattern, Action) method.

But xpath, xquery, xslt, etc all use just strings to express which
elements are to be matched. So if it is good enough for the W3C, isn't
it good enough for digester?

Or have I misunderstood?



Regards,

Simon


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message