2
\$\begingroup\$

i am looking for a faster and more efficient way of this code. What the code does is simply compares message context of (user, application, service and operation) and compares it with some rules.

private boolean match(Context messageContext, ContextRule contextRule) {
        if(contextRule.getMessageContext().getUser().equals(ContextRuleEvaluator.WILDCARD)
            || (contextRule.getMessageContext().getUser().equals(messageContext.getUser()))) {
          if(contextRule.getMessageContext().getApplication().equals(ContextRuleEvaluator.WILDCARD)
              || (contextRule.getMessageContext().getApplication().equals(messageContext.getApplication()))) {
            if(contextRule.getMessageContext().getService().equals(ContextRuleEvaluator.WILDCARD)
                || (contextRule.getMessageContext().getService().equals(messageContext.getService()))) {
              if(contextRule.getMessageContext().getOperation().equals(ContextRuleEvaluator.WILDCARD)
                  || (contextRule.getMessageContext().getOperation().equals(messageContext.getOperation()))) {
                return true;
              }
            }
          }
        }

        return false;
      } 

Edit: Context and ContextRule are an interfaces

public interface Context {    
   public String getUser();      
   public void setUser(String user);      
   public String getApplication();      
   public void setApplication(String application);      
   public String getService();      
   public void setService(String service);      
   public String getOperation();      
   public void setOperation(String operation);
}   

public interface ContextRule {    
  public Context getMessageContext();      
  public int getAllowedConcurrentRequests();      
}

Any suggestions appreciated :)

\$\endgroup\$
2
  • \$\begingroup\$ Please a little bit more information. What is Context and ContextRule. What doe all these functions return? \$\endgroup\$
    – Bobby
    Oct 31, 2013 at 15:22
  • \$\begingroup\$ @Bobby as per your request i have edited the post \$\endgroup\$ Oct 31, 2013 at 15:32

2 Answers 2

5
\$\begingroup\$

First of all, your formatting seems messy, please fix it.


Did you rip it out or is there really zero documentation in there (JavaDoc)? Consider adding some, it will make your life and that of others easier.


First step would be to add an overload to match() which accepts two strings, like this:

private boolean match(Context messageContext, ContextRule contextRule) {
    return match(contextRule.getMessageContext().getUser(), messageContext.getUser())
            && match(contextRule.getMessageContext().getApplication(), messageContext.getApplication())
            && match(contextRule.getMessageContext().getService(), messageContext.getService())
            && match(contextRule.getMessageContext().getOperation(), messageContext.getOperation());
}

private boolean match(String value, String contextValue) {
    return value.equals(ContextRuleEvaluator.WILDCARD) || value.equals(contextValue);
}

This makes it already a lot easier to read. Naming of the variables is a little bit suboptimal, that's because I could not come up with better names right now.


Ultimately you should see if you can't extend and change Context to contain a match() function. This would require to change Context into an abstract class, so it might not be possible.

public abstract class Context {

    public abstract String getUser();

    public abstract void setUser(String user);

    public abstract String getApplication();

    public abstract void setApplication(String application);

    public abstract String getService();

    public abstract void setService(String service);

    public abstract String getOperation();

    public abstract void setOperation(String operation);

    /**
     * Matches this context against the given Context.
     *
     * @param context
     * @return
     */
    public boolean match(Context context) {
        return match(getUser(), context.getUser())
                && match(getApplication(), context.getApplication())
                && match(getService(), context.getService())
                && match(getOperation(), context.getOperation());
    }

    private boolean match(String value, String otherValue) {
        return value.equals(ContextRuleEvaluator.WILDCARD) || value.equals(otherValue);
    }
}

That would allow you to do this in your code:

return contextRule.getMessageContext().match(messageContext);

Which is very readable, even if you don't pack it into a function but call it directly.

\$\endgroup\$
1
  • \$\begingroup\$ Yup no javadocs :(. Thank you for your suggestions anyway :) this seems to be better then current block \$\endgroup\$ Oct 31, 2013 at 16:20
4
\$\begingroup\$

One way that you could clean up the comparisons is to look at using an object to represent each of the different checks that you need to make. See com.google.common.collect.ComparisonChain for a slightly different application of the same idea.

Basic idea being that you have an interface that exposes each of your checks, returning an instance of itself. The interface also has a "result" method at the end, that returns true or false.

Under the covers, you have two implementations of this object. One of them evaluates "false", and always returns a copy of itself. The other evaluates to "true", and returns itself if the comparison is true, and the other guy if the comparison is false.

So your client code looks like

boolean result = MagicCompareThing.create()
                 .match(lhs.getUser(),rhs.getUser())
                 .match(lhs.getApplication(),rhs.getApplication())
                 .match(lhs.getService(),rhs.getService())
                 .match(lhs.getOperation(),rhs.getOperation())
                 .result();

The strong resemblance to @Bobby's pattern is deliberate.

interface MagicCompareThing {
    boolean result();
    MagicCompareThing <T> match (T lhs, T rhs);

    static MagicCompareThing create() {
        return MagicCompareThing.TRUE;
    }

    private static final MagicCompareThing FALSE = new MagicCompareThing {
        boolean result () { return false; }
        MagicCompareThing <T> match(T lhs, T rhs) { return this; }
    } 

    private static final MagicCompareThing TRUE = new MagicCompareThing {
        boolean result () { return true; }
        MagicCompareThing <T> match (T lhs, T rhs) {
            if ( ContextRuleEvaluator.WILDCARD.equals(lhs) ) {
                return this;
            }
            if (lhs.equals(rhs)) {
                return this;
            }
            return FALSE;
        }
    }
}

Fundamentally, what this code is doing is noticing that the complicated ordering of comparisons is really just a state machine - false always transitions to itself, and true transitions both ways depending on the outcome of each comparison in turn.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Interesting concept. I've never seen such a comparison chain before (resembles the builder pattern a little), interesting. \$\endgroup\$
    – Bobby
    Oct 31, 2013 at 21:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.