commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <s_kitch...@paradise.net.nz>
Subject [digester] variable expansion
Date Thu, 04 Dec 2003 09:17:31 GMT
Hi Robert,

I think the code committed is just fine.

I like the versatile "Substitutor" interface; I can imagine all sorts of
uses for the ability to arbitrarily manipulate attribute values and body
text before rules see them other than just var expansion.

I also like the separation of this code out into a package, with only
the Substitutor interface in the main code.

Some *very* minor comments: 

--

In VariableSubstitutor, you use lazy creation for variableAttributes.
I'm not sure there is much point to that. Given that the user has set up
var expansion in the first place, the object will almost certainly be
created sometime. However the existing code has to evaluate an
if-statement on each call to determine if the object already exists. I
think it also makes the code more complex than it needs to be. Why not
create it in the constructor as soon as it is known if there is an
attributeExpander or not?

--

I think a comment is needed in the Substitutor "interface" specifying
that Digester guarantees never to hold the return value of the
substitute(Attributes) method past the next call to the method. It is
this behaviour that allows a single VariableAttributes object to be
reused by the VariableSubstitutor. This behaviour was obvious before,
when VariableAttributes was directly invoked from the Digester's
startElement method, but probably needs to be explicitly documented now.

--

You mention that you were considering whether it is necessary to have
Remy's VarExpander implementation as well as the MultiVariableExpander.
While it would be a shame to discard Remy's work, I don't see much
benefit from having an additional implementation.

The MultiVariableExpander does:
        for(int i=0; i<nEntries; ++i) {
            param = expand(
                param, 
                (String) markers.get(i), 
                (Map) sources.get(i));
        }
        return param;

The call to "expand" in the loop does exactly the same work as an
implementation that supports only "${foo}" would do.

The overhead of looping once (ie nEntries=1) is trivial. The overhead of
fetching the marker and map from ArrayLists and casting them is slightly
higher, but still pretty small I think. That's the only performance
difference I can see from an implementation customised for ant-like
variables only. Oh - and avoiding one string concatenation in the expand
method ("$" + "{").

The API would be *very* slightly simpler: setSource(map) rather than
addSource("$", map). I think this is more than offset by the complexity
of having multiple VarExpander implementations to choose from.

If the consensus is that the MultiVariableExpander is ok to use for the
simple case too, then it would probably make sense to rename it (eg
BaseVariableExpander).

--

The spelling "substituter" feels more natural to me than "substitutor".

cf.:
to write --> writer
to drive --> driver
to expand --> expander


Cheers,

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