commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rahul Akolkar <rahul.akol...@gmail.com>
Subject Re: [JEXL] JexlContext API changes in JEXL-92 (was: svn commit: r884650 [1/2] ...)
Date Mon, 30 Nov 2009 16:07:39 GMT
I have three comments plus two nits below (search for strings
"Comment" and "Nit"):

On Thu, Nov 26, 2009 at 12:17 PM,  <henrib@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


Mime
View raw message