commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Zeigermann <oliver.zeigerm...@gmail.com>
Subject Re: [digester2] Additions
Date Sun, 13 Feb 2005 16:24:51 GMT
> 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.

In case those member variables really change this would be a general
redesign I guess. In this case you might just as well create a copy
from DefaultRuleManager maybe called NewDefaultRuleManager or
DefaultRuleManager2 or whatever. I agree that passing the information
using a method is no better. And having something like a copy helper
is not enough as people might want have access to DefaultRuleManager 
guts in other scenarios as well.
 
> ===
> 
> 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.

OK. If you think so. Done.

> ===
> 
> 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

I see. You are right.

> 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.

Got that. I will think about that and provide something new.

> 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?

Maybe. Will do so.
 
> ===
> 
> 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

Tried that. Hopefully done now.

> > (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.

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

It is not about complexity, but a simple and intuitive interface for
the user who writes an Action. Certainly, push, pop and clear should
never be used by the user. Doing so would even be a hazard, I guess.
So, these setter methods should not be part of the interface.

Do you agree?

Oliver

---------------------------------------------------------------------
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