You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Rahul Akolkar <ra...@gmail.com> on 2009/11/30 17:07:39 UTC
Re: [JEXL] JexlContext API changes in JEXL-92 (was: svn commit:
r884650 [1/2] ...)
I have three comments plus two nits below (search for strings
"Comment" and "Nit"):
On Thu, Nov 26, 2009 at 12:17 PM, <he...@apache.org> wrote:
> Author: henrib
> Date: Thu Nov 26 17:17:42 2009
> New Revision: 884650
>
> URL: http://svn.apache.org/viewvc?rev=884650&view=rev
> Log:
> JEXL-92: refactored JexlContext, removed JexlHelper & HashMapContext
>
<snip/>
>
> Modified: commons/proper/jexl/trunk/src/main/java/org/apache/commons/jexl2/JexlContext.java
> URL: http://svn.apache.org/viewvc/commons/proper/jexl/trunk/src/main/java/org/apache/commons/jexl2/JexlContext.java?rev=884650&r1=884649&r2=884650&view=diff
> ==============================================================================
> --- commons/proper/jexl/trunk/src/main/java/org/apache/commons/jexl2/JexlContext.java (original)
> +++ commons/proper/jexl/trunk/src/main/java/org/apache/commons/jexl2/JexlContext.java Thu Nov 26 17:17:42 2009
> @@ -16,33 +16,84 @@
> */
> package org.apache.commons.jexl2;
>
> +import java.util.HashMap;
> import java.util.Map;
>
> /**
> - * Holds a Map of variables which are referenced in a JEXL expression.
> + * Manages variables which can be referenced in a JEXL expression.
> *
> * @since 1.0
> - * @author <a href="mailto:geirm@apache.org">Geir Magnusson Jr.</a>
> * @version $Id$
> */
> public interface JexlContext {
> /**
> - * Replaces variables in a JexlContext with the variables contained
> - * in the supplied Map. When setVars() is called on a JexlContext,
> - * it clears the current Map and puts each entry of the
> - * supplied Map into the current variable Map.
> - *
> - * @param vars Contents of vars will be replaced with the content
> - * of this Map
> + * Gets the value of a variable.
> + * @param name the variable's name
> + * @return the value
> */
> - void setVars(Map<String,Object> vars);
> -
> + Object getJexlVariable(String name);
> +
> + /**
> + * Sets the value of a variable.
> + * @param name the variable's name
> + * @param value the variable's value
> + */
> + void setJexlVariable(String name, Object value);
> +
<snip/>
Comment 1: I'd prefer the much simpler and elsewhere more widely used
method names of get/set/has in the JexlContext API. See, for example
(ignore the parent/delegation model bits, see get/set/has methods):
http://commons.apache.org/scxml/0.9/apidocs/org/apache/commons/scxml/Context.html
> /**
> - * Retrives the Map of variables associated with this JexlContext. The
> - * keys of this map correspond to variable names referenced in a
> - * JEXL expression.
> - *
> - * @return A reference to the variable Map associated with this JexlContext.
> + * A context that differentiates null valued variables and undefined ones.
> + * <p>A non-nullable context does not allow differentiating a variable whose
> + * value is null and an undefined one; thus the Nullable name for this kind of context.</p>
> */
> - Map<String,Object> getVars();
> + public interface Nullable extends JexlContext {
> + /**
> + * Checks whether a variable is defined in this context.
> + * <p>A variable may be defined with a null value; this method checks whether the
> + * value is null or if the variable is undefined.</p>
> + * @param name the variable's name
> + * @return true if it exists, false otherwise
> + */
> + boolean definesJexlVariable(String name);
> + }
> +
<snap/>
Comment 2: I think we should just have one JexlContext interface that
includes a has method (rather than the dichotomy of JexlContext vs.
JexlContext.Nullable). In addition to being simpler by reducing the
number of separable concepts to be aware of, it avoids instanceof
checks etc. as occur elsewhere in the code in this commit.
> + /**
> + * Wraps a map in a context.
> + * <p>Each entry in the map is considered a variable name, value pair.</p>
> + */
> + public static class Mapped implements Nullable {
<snip/>
Comment 3: As a matter of taste perhaps, I think we shouldn't use a
nested class here, rather define a basic JexlContext implementation in
its own source artifact. This request stems from the rather unsettling
effect a commit to the source artifact containing the fundamental
interface in a library has on people following changes to the library
(even though changes made may be compatible :-). Ideally, we'd change
JexlContext.java here and never touch that file again in the 2.x line
of releases.
> + /**
> + * The wrapped variable map.
> + */
> + protected final Map<Object,Object> map;
<snap/>
Nit 1: Map<String, Object> is better.
> + /**
> + * Creates an instance using an HashMap as the underlying variable storage.
> + */
> + public Mapped() {
> + this(null);
> + }
> + /**
> + * Creates an instance using a provided map as the underlying variable storage.
> + * @param vars the variables map
> + */
> + @SuppressWarnings("unchecked")
> + public Mapped(Map<?, ?> vars) {
> + map = (Map<Object,Object>) (vars == null? new HashMap<String,Object>() : vars);
> + }
> +
> + /** {@inheritDoc} */
> + public boolean definesJexlVariable(String name) {
> + return map.containsKey(name);
> + }
> +
> + /** {@inheritDoc} */
> + public Object getJexlVariable(String name) {
> + return map.get(name);
> + }
> +
> + /** {@inheritDoc} */
> + public void setJexlVariable(String name, Object value) {
> + map.put(name, value);
> + }
> + }
> +
> }
>
> Modified: commons/proper/jexl/trunk/src/main/java/org/apache/commons/jexl2/JexlEngine.java
> URL: http://svn.apache.org/viewvc/commons/proper/jexl/trunk/src/main/java/org/apache/commons/jexl2/JexlEngine.java?rev=884650&r1=884649&r2=884650&view=diff
> ==============================================================================
> --- commons/proper/jexl/trunk/src/main/java/org/apache/commons/jexl2/JexlEngine.java (original)
> +++ commons/proper/jexl/trunk/src/main/java/org/apache/commons/jexl2/JexlEngine.java Thu Nov 26 17:17:42 2009
> @@ -87,7 +87,39 @@
> * </p>
> * @since 2.0
> */
> -public class JexlEngine {
> +public class JexlEngine {
> +
> + /**
> + * Checks whether a variable is defined in a context.
> + * @param context the context
> + * @param name the variable's name
> + * @return true if the variable is defined, false otherwise
> + */
> + protected static boolean isVariableUndefined(JexlContext context, String name) {
> + if (context instanceof JexlContext.Nullable) {
> + return !((JexlContext.Nullable) context).definesJexlVariable(name);
> + }
> + return context.getJexlVariable(name) == null;
<snip/>
Nit 2: Example of instanceof check I mentioned above in this method.
-Rahul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [JEXL] JexlContext API changes in JEXL-92 (was: svn commit:
r884650 [1/2] ...)
Posted by Rahul Akolkar <ra...@gmail.com>.
On Mon, Nov 30, 2009 at 5:51 PM, henrib <he...@apache.org> wrote:
>
> Repaired damage through revision 885553.
> Hope this suits better.
>
Yup, thanks.
-Rahul
>
> Rahul Akolkar wrote:
>>
>> I have three comments plus two nits below (search for strings
>> "Comment" and "Nit"):
>> ...
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [JEXL] JexlContext API changes in JEXL-92 (was: svn commit:
r884650 [1/2] ...)
Posted by henrib <he...@apache.org>.
Repaired damage through revision 885553.
Hope this suits better.
Rahul Akolkar wrote:
>
> I have three comments plus two nits below (search for strings
> "Comment" and "Nit"):
> ...
>
--
View this message in context: http://n4.nabble.com/Re-JEXL-JexlContext-API-changes-in-JEXL-92-was-svn-commit-r884650-1-2-tp931391p931712.html
Sent from the Commons - Dev mailing list archive at Nabble.com.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org