Is there a way to shorten a conditional that contains a bunch of boolean comparisons?

Question

e.g

if("viewCategoryTree".equals(actionDetail)
                || "fromCut".equals(actionDetail)
                || "fromPaste".equals(actionDetail)
                || ("viewVendorCategory".equals(actionDetail))&&"viewCategoryTree".equals(vendorCategoryListForm.getActionOrigin())
                || ("viewVendorCategory".equals(actionDetail))&&"fromEdit".equals(vendorCategoryListForm.getActionOrigin())
                || "deleteSelectedItem".equals(actionDetail)
                || ("viewVendorCategory".equals(actionDetail))&&"fromLink".equals(vendorCategoryListForm.getActionOrigin())){
//do smth
}

I've tried something like this

if(check("deleteSelectedItem,viewCategoryTree,fromCut,fromPaste,{viewVendorCategory&&viewVendorCategory},{viewVendorCategory&&fromEdit},{viewVendorCategory&&fromLink}",actionDetail,actionOrigin)){
//do smth
}

public boolean check(String str, String ad, String ao){

    String oneCmp = "";
    String[] result = str.split(",");
    ArrayList adList = new ArrayList();
    ArrayList aoList = new ArrayList();
    for (int i=0; i<result.length; i++){
        oneCmp = result[i];
        Matcher m = Pattern.compile("\\{([^}]*)\\}").matcher(oneCmp);
        if(m.matches()){
            m.find();
            String agrp = m.group();
            String[] groupresult = agrp.split("[\\W&&[^!]]+");
            Boolean a = false;
            Boolean b = false;
            if(groupresult[0].startsWith("!")){
                a = !groupresult[0].substring(1).equals(ad);
            } else a = groupresult[0].equals(ad);
            if(groupresult[1].startsWith("!")){
                b = !groupresult[1].substring(1).equals(ao);
            }else b = groupresult[1].equals(ao);

            if(agrp.indexOf("&&")!=-1){
                if(!(a && b))return false;
            }
            else if(agrp.indexOf("||")!=-1){
                if(!(a || b))return false;
            }
        } else {
            if(oneCmp.indexOf("^")==-1){
                checklist(oneCmp,ad);
                        if(!checklist(oneCmp,ad))return false;
            }else{
            if(!checklist(oneCmp,ao))return false;
            }
        }
    }

    return false;
}

public boolean checklist(String str, String key){

    if(str.startsWith("!")){
        if(str.substring(1).equals(key))return false;
        }else { if (!str.substring(1).equals(key)) return false;
        }
    }

    return false;
}

is there a better way to do this ? thanks.

You should have asked this on http://codereview.stackexchange.com — Oscar Mederos, Mar 07 '11 at 04:50
@Oscar he *could* have asked this, but it is still on topic here. — , Mar 07 '11 at 11:41
@Will Take a look at Jeff's answer: http://meta.stackexchange.com/questions/82069/is-it-okay-to-have-a-stackexchange-site-that-is-a-subset-of-another-one — Oscar Mederos, Mar 07 '11 at 20:24
@Oscar Honestly, the question isn't about "here's my code, is it good or bad" but techniques for shortening `if` statements containing *lots* of boolean comparisons. Its more patterns than practices. Its definitely on topic here, and could possibly be considered on topic at codereview.SE. — , Mar 08 '11 at 13:39

score 2 · Answer 1 · answered Mar 07 '11 at 04:27

Move the check to a method that takes actionDetail as argument:

// Assumes vendorCategoryListForm is a member variable.
boolean check(String actionDetail) {
    return ("viewCategoryTree".equals(actionDetail)
            || "fromCut".equals(actionDetail)
            || "fromPaste".equals(actionDetail)
            || (("viewVendorCategory".equals(actionDetail))
                &&"viewCategoryTree".equals(vendorCategoryListForm.getActionOrigin()))
            || (("viewVendorCategory".equals(actionDetail))
                &&"fromEdit".equals(vendorCategoryListForm.getActionOrigin()))
            || "deleteSelectedItem".equals(actionDetail)
            || (("viewVendorCategory".equals(actionDetail))
                &&"fromLink".equals(vendorCategoryListForm.getActionOrigin())))
}

if (check(actionDetail)) {
    // do this
}

Himadri Choudhury · Answer 2 · 2011-03-07T04:41:42.123

2

How about creating an array of what you need to test against. And then some code like this:

arrayOfStrings = ["viewCategoryTree", ...]
match = false
for elem in arrayOfStrings:
   if elem == actionDetail:
       match = true
       break

The good thing about an array is that it is easily extensible: you can easily add/remove elements to it both statically and dynamically.

edited Mar 07 '11 at 04:41

answered Mar 07 '11 at 04:29

Himadri Choudhury

10,217
6
39
47

score 1 · Answer 3 · edited May 23 '17 at 11:47

1

Also kindly look at this post

Language Agnostic Credits to Galwegian

See Flattening Arrow Code for help.

   1. Replace conditions with guard clauses.
   2. Decompose conditional blocks into seperate functions.
   3. Convert negative checks into positive checks.

edited May 23 '17 at 11:47

Community

1
1

answered Mar 07 '11 at 05:21

Dead Programmer

12,427
23
80
112

score 0 · Answer 4 · answered Mar 07 '11 at 04:31

Honestly, that code is no more readable. I would better suggest to encapsulate that conditional check into some property for the type like if (control.IsApplicable) { // do smth }.

No matter either you parameterize by one or two arguments. But I suppose better solution is to have an array of matches that could be tested against and if matched then return true.

score 0 · Answer 5 · answered Mar 07 '11 at 04:45

I don't think you are going to improve on this without adding a bunch of complexity, both in terms of the notation that you use to express the conditions and the implementation of the "engine" that evaluates them.

The notation issue is that: while you may end up expressing the conditions in fewer characters, someone else reading your code has to figure out what that funky string literal really means.

Besides, anything clever you do could have an impact on performance. For instance, your attempt compiles and applies a regex multiple times for each call to check.

Stick with what you've got would be my advice.

score 0 · Answer 6 · answered Mar 07 '11 at 06:27

if(isValidActionDetail(actionDetail)
            || (isValidActionDetail(actionDetail)
            && ("viewCategoryTree".equals(vendorCategoryListForm.getActionOrigin()) 
                || "fromEdit".equals(vendorCategoryListForm.getActionOrigin())  
                || "fromLink".equals(vendorCategoryListForm.getActionOrigin())))){

//do smth
    }
}

public static boolean isValidActionDetail (String actionDetail) {
    return "viewCategoryTree".equals(actionDetail) || "fromCut".equals(actionDetail) 
           || "fromPaste".equals(actionDetail) || "deleteSelectedItem".equals(actionDetail) 
           || "viewVendorCategory".equals(actionDetail);
}

You can decompose in the above way, as the first step to refactoring your logic.

Is there a way to shorten a conditional that contains a bunch of boolean comparisons?

6 Answers6