Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

share|improve this question
 
Please a little bit more information. What is Context and ContextRule. What doe all these functions return? –  Bobby Oct 31 at 15:22
 
@Bobby as per your request i have edited the post –  Maciej Cygan Oct 31 at 15:32
add comment

2 Answers

up vote 5 down vote accepted

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.

share|improve this answer
 
Yup no javadocs :(. Thank you for your suggestions anyway :) this seems to be better then current block –  Maciej Cygan Oct 31 at 16:20
add comment

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.

share|improve this answer
1  
Interesting concept. I've never seen such a comparison chain before (resembles the builder pattern a little), interesting. –  Bobby Oct 31 at 21:21
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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