current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

We get the search string from the UI. Based on the search parameters we need to create a back-end query. This is similar to "Feed Item Query Language(FIQL)". I have wrote the code to find the logical operators in the search parameters like || (or operator) and ; (And operator).

The code is working as expected, but I want to know if we can improve the code.

For example, the below search parameter:

  • And operator (;) is outer group
  • Or operator (||) is inner group
status:eq(A);sourceKey.entityId:eq(36KAG);[hostProductLineCode:eq(YB)||hostProductLineCode:eq(PB)]

package com.guthyrenker.soma.ws.rest.v1.client;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.apache.commons.lang.StringUtils;
import org.junit.Assert;

public class TestOperator
{
    public static void main(String[] args) {


        String outerOperator = null;

        String filter = "status:eq(A)";
        outerOperator = validateAndGetOuterOperator(filter);
        Assert.assertEquals("Outer operator is and condition",";", outerOperator);

        filter = "status:eq(A);";
        outerOperator = validateAndGetOuterOperator(filter);
        Assert.assertEquals("Outer operator is and condition",";", outerOperator);

        filter = "status:eq(A);sourceKey.entityId:eq(36KAG);[hostProductLineCode:eq(YB)||hostProductLineCode:eq(PB)]";
        outerOperator = validateAndGetOuterOperator(filter);
        Assert.assertEquals("Outer operator is and condition",";", outerOperator);

        filter =
                "status:eq(A)||sourceKey.entityId:eq(36KAG)||[hostProductLineCode:eq(YB);hostProductLineCode:eq(PB)]||[productLineCode:eq(YB);productLineCode:eq(PB)]";
        outerOperator = validateAndGetOuterOperator(filter);
        Assert.assertEquals("Outer operator is or condition","||", outerOperator);

        filter =
                "status:eq(A)||sourceKey.entityId:eq(36KAG)||[hostProductLineCode:eq(YB);hostProductLineCode:eq(PB)]||[productLineCode:eq(YB);productLineCode:eq(PB)]||readyDate:eq(2014-01-01T00:00:00)";
        outerOperator = validateAndGetOuterOperator(filter);
        Assert.assertEquals("Outer operator is or condition","||", outerOperator);

        filter = "[hostProductLineCode:eq(YB);hostProductLineCode:eq(PB)]||[productLineCode:eq(YB);productLineCode:eq(PB)]";
        outerOperator = validateAndGetOuterOperator(filter);
        Assert.assertEquals("Outer operator is or condition","||", outerOperator);

        filter = "[hostProductLineCode:eq(YB)||hostProductLineCode:eq(PB)];[productLineCode:eq(YB)||productLineCode:eq(PB)]";
        outerOperator = validateAndGetOuterOperator(filter);
        Assert.assertEquals("Outer operator is and condition",";", outerOperator);

        filter =
                "status:eq(A);sourceKey.entityId:eq(36KAG);[hostProductLineCode:eq(YB)||hostProductLineCode:eq(PB)];readyDate:eq(2014-01-01T00:00:00)";
        outerOperator = validateAndGetOuterOperator(filter);
        Assert.assertEquals("Outer operator is and condition",";", outerOperator);

        // Exception Cases

        filter = "[hostProductLineCode:eq(YB);hostProductLineCode:eq(PB)];[productLineCode:eq(YB)||productLineCode:eq(PB)]";
        try {
            outerOperator = validateAndGetOuterOperator(filter);
        } catch (IllegalArgumentException e) {
            Assert.assertTrue(true);
        }

        try {
        filter =
                "[hostProductLineCode:eq(YB)||hostProductLineCode:eq(PB)];[productLineCode:eq(YB)||productLineCode:eq(PB)||[productLineCode:eq(ZB)]]";
            outerOperator = validateAndGetOuterOperator(filter);
        } catch (IllegalArgumentException e) {
            Assert.assertTrue(true);
        }

        try {
            filter = "status:eq(A);status:eq(P)||status:eq(C)";
            outerOperator = validateAndGetOuterOperator(filter);
        } catch (IllegalArgumentException e) {
            Assert.assertTrue(true);
        }

        try {
            filter = "status:eq(A);sourceKey.entityId:eq(36KAG);[hostProductLineCode:eq(YB)||hostProductLineCode:eq(PB)]||status:eq(C)";
            outerOperator = validateAndGetOuterOperator(filter);
        } catch (IllegalArgumentException e) {
            Assert.assertTrue(true);
        }

        try {
            filter = "status:eq(A)-sourceKey.entityId:eq(36KAG);[hostProductLineCode:eq(YB)||hostProductLineCode:eq(PB)]";
            outerOperator = validateAndGetOuterOperator(filter);
        } catch (IllegalArgumentException e) {
            Assert.assertTrue(true);
        }
    }

    public static String validateAndFindSubgroupOperator(String filter) {

        String previousOperator = null;
        String curOperator = null;

        String REGEX_SUBGROUP = "\\[(.*?)\\]";

        Pattern pattern = Pattern.compile(REGEX_SUBGROUP);
        Matcher matcher = pattern.matcher(filter);

        List<String> matches = new ArrayList<String>();
        while (matcher.find()) {
            String group = matcher.group();
            matches.add(group);
        }


        for(String subGroupFilter : matches){
            curOperator = validateGroup(subGroupFilter);
            if(previousOperator!=null && !curOperator.equals(previousOperator)){
                throw new IllegalArgumentException("All subgroups should have same operator.");
            }else {
                 String subGroupNoBraces = subGroupFilter.substring(1, subGroupFilter.length()-1);
                 if(subGroupNoBraces.indexOf("[")>0 || subGroupNoBraces.indexOf("]")>0){
                     throw new IllegalArgumentException("Nested subgroups are not allowed.");
                 }
            }
            previousOperator = curOperator;
        }
        return curOperator;
    }

    public static String validateAndFindOuterOperator(String filter) {

        String REGEX_SUBGROUP = "\\[(.*?)\\]";

        filter = filter.replaceAll(REGEX_SUBGROUP, "");

        return validateGroup(filter);
    }

    public static String validateGroup(String filter) {

        // Parameter regular expression
        String REGEX_PARAMETER = "([A-Za-z0-9.]+):([A-Za-z]+)(\\({1})(\\')?([a-zA-Z0-9-+,:]+)(\\')?(\\){1})";

        if ((filter.indexOf(";") >= 0 && filter.indexOf("||") >= 0)) {
            throw new IllegalArgumentException("Expected same operator, but found different operator.");
        } else if (filter.indexOf("||") >= 0) { // or logical operator is found for the subgroups
            return "||";
        } else if (filter.indexOf(";") >= 0 || filter.matches(REGEX_PARAMETER)) { // and logical operator is found for the subgroups
            return ";";
        } else {
            throw new IllegalArgumentException("Filter is not valid.");
        }
    }

    private static String validateAndGetOuterOperator(String filter){
        String subGroupOperator = validateAndFindSubgroupOperator(filter);
        String outerGroupOperator = validateAndFindOuterOperator(filter);
        return outerGroup;
    }


}
share|improve this question

closed as off-topic by tim, Vogel612, Pimgd, Marc-Andre, Malachi Oct 29 '14 at 14:08

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – tim, Vogel612, Pimgd, Marc-Andre, Malachi
If this question can be reworded to fit the rules in the help center, please edit the question.
    
Welcome to CodeReview. Your code does not compile (you are returning outerGroup in validateAndGetOuterOperator, but it doesn't exist). Is this maybe just a copy-paste error? –  tim Oct 29 '14 at 10:36
    
I have fixed that issue, i just renamed it. –  srinivasa Kankipati Oct 29 '14 at 11:47
    
can you fix it here in the question as well? I'm assuming you want to return outerGroupOperator (in that case, all your tests pass), but why are you calling validateAndFindSubgroupOperator then? For that matter, that method seems to be entirely unused. –  tim Oct 29 '14 at 11:57

2 Answers 2

up vote 2 down vote

Wrapper Methods

Methods that don't do anything except call another method should be removed. validateAndGetOuterOperator seems to be such a method.

Naming

  • only class level constants should be all uppercase.
  • what's the difference between find and get in your method names?
  • all your method names start with validate. Either just remove it (I think the names don't lose meaning without it), or remove it and change your class name (LogicValidator or something).

Unit Tests

Create actual unit tests. It's great that you have tests, but because they are all in main, as soon as one test fails, the other tests are not executed anymore. It should look something like this:

import org.junit.Assert;
import org.junit.Test;
import test2.TestOperator;

public class TestOperatorTest {

    @Test
    public void noAnd() {
        String filter = "status:eq(A)";
        String outerOperator = TestOperator.validateAndGetOuterOperator(filter);
        Assert.assertEquals(";", outerOperator);
    }

    @Test
    public void trailingAnd() {
        String filter = "status:eq(A);";
        String outerOperator = TestOperator.validateAndGetOuterOperator(filter);
        Assert.assertEquals(";", outerOperator);
    }

    @Test
    public void andAndInnerOr() {
        String filter = "status:eq(A);sourceKey.entityId:eq(36KAG);[hostProductLineCode:eq(YB)||hostProductLineCode:eq(PB)]";
        String outerOperator = TestOperator.validateAndGetOuterOperator(filter);
        Assert.assertEquals(";", outerOperator);
    }

    @Test(expected=IllegalArgumentException.class)
    public void invalidBecauseX() {
        String filter = "[hostProductLineCode:eq(YB);hostProductLineCode:eq(PB)];[productLineCode:eq(YB)||productLineCode:eq(PB)]";
        String outerOperator = TestOperator.validateAndGetOuterOperator(filter);
    }
    // [...]

}

Note that I removed your comments, as they were not that helpful in determining what a test does. I did give the test methods names that I think describe the test cases better.

share|improve this answer
up vote 1 down vote

Your regular expressions could use some work:

public static String validateAndFindOuterOperator(String filter) {

    String REGEX_SUBGROUP = "\\[(.*?)\\]";

    filter = filter.replaceAll(REGEX_SUBGROUP, "");

    return validateGroup(filter);
}

The above code should have the regular expression compiled outside the method, as a constant:

private static final Pattern REGEX_SUBGROUP = Pattern.compile("\\[(.*?)\\]");

public static String validateAndFindOuterOperator(String filter) {
    filter = REGEX_SUBGROUP.matcher(filter).replaceAll("");
    return validateGroup(filter);
}

Similarly, the pattern:

    // Parameter regular expression
    String REGEX_PARAMETER = "([A-Za-z0-9.]+):([A-Za-z]+)(\\({1})(\\')?([a-zA-Z0-9-+,:]+)(\\')?(\\){1})";

should be streamlined a lot more. There are three things I suggest. First, make it a Pattern constant, second, the {1} is redundant, along with all the grouping braces. Third, make it case-insensitive to reduce the repetition.

private static final Pattern REGEX_PARAMETER = Pattern.compile(
      "[a-z0-9.]+:[a-z]+\\('?[a-z0-9-+,:]+'?\\)", Pattern.CASE_INSENSITIVE);

Note that the above pattern (and your original code) will match the invalid input ABC:DEF('onequote). I would fix it by having the expression:

private static final Pattern REGEX_PARAMETER = Pattern.compile(
      "[a-z0-9.]+:[a-z]+\\(('[a-z0-9-+,:]+'|[a-z0-9-+,:]+)\\)", Pattern.CASE_INSENSITIVE);

which allows either quoted values, or unquoted values, but not half-quoted values.

share|improve this answer

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