How to represent a long if-else tree in a concise manner

Question

Long story short, I've inherited a Java piece of code made of methods like this one:

@Override
public Action decide() {

    if (equalz(in.a, "LOC")) {//10
        if(( //20
                equalz(tmp.b, "BA")
                && notEquals(in.c,"U")
                && equalz(in.d,"Y")
            )||(
                equalz(tmp.b, "HV")
                && notEquals(in.c,"U")
                && equalz(in.d,"Y")
                && varEqualsOneOf(in.e,"FLG","FLR","RRG"))
        ) {
            if(equalz(tmp.b, "BA")) {//30
                if(varEqualsOneOf(in.e,"RRG","NL")//40
                    && lessThan(in.f,in.g)) {
                    return Action.AC015;
                } else {
                    if(varEqualsOneOf(in.e,"FLG","FLR")) {//50
                        return Action.AC015;
                    } else {
                        return Action.AC000;
                    }
                }
            } else {
                if (equalz(in.e,"RRG")) {//60
                    return Action.AC014;
                } else {
                    return Action.AC010;
                }
            }
        } else {
            if(equalsOrMissing(in.c,"U")    
                && varEqualsOneOf(in.e,"FLG","FLR")) {//70
                return Action.AC011;
            } else {
                return Action.AC000;
            }
        }
    } else {
        if(varEqualsOneOf(in.a,"MNC","BAN","LCI","CTV","LEA","INS")) {//80
            if (//90
                equalz(in.h,"A")
                || equalz(in.i,"Y")
            ) {
                return Action.AC000;
            } else {
                if(notEquals(in.h,"U")) {//100
                    if(greaterThan(in.j,in.k)) {//110
                        return Action.AC000;
                    } else {
                        if(varEqualsOneOf(in.e,"FLG","FLR")) {//120
                            if(notEquals(in.l,"U")) {//130
                                return Action.AC004;
                            } else {
                                if(oneOfVarsEqual(in.m,in.n,"Y")) {//140
                                    return Action.AC012;
                                } else {
                                    return Action.AC002;
                                }
                            }
                        } else {
                            if(//150
                                (
                                    equalz(in.e,"RRG")
                                    && varEqualsOneOf(in.a,"MNC","BAN","LCI","CTV","LEA")   
                                )
                                ||
                                (
                                    varEqualsOneOf(in.e,"RRG","NL")
                                    && equalz(in.a,"INS")
                                )
                            ) {
                                if (notEquals(in.l,"U")) {//160
                                    if (notEquals(in.o,"U")) {//170
                                        return Action.AC005;
                                    } else {
                                        return Action.AC018;
                                    }
                                } else {
                                    bp(false);
                                    if (equalz(in.n,"Y")) {//180
                                        return Action.AC012;
                                    } else {
                                        return Action.AC002;
                                    }
                                }
                            } else {
                                if (equalz(in.p,in.q)) {//190
                                    if (notEquals(in.o,"U")) {//200
                                        return Action.AC006;
                                    } else {
                                        return Action.AC008;
                                    }
                                } else {
                                    return Action.AC000;
                                }
                            }
                        }
                    }
                } else {
                    bp(false);
                    if (notEquals(in.l,"U")//210
                        && varEqualsOneOf(in.e,"FLG","FLR")) {
                        return Action.AC004;
                    } else {
                        return Action.AC000;
                    }
                }
            }
        } else {
            return Action.AC000;
        }
    }
}

where a series of conditions are checked to return the correct action to be performed. I don't find this solution very elegant, and furthermore I need a way to quickly tell the path that led to a certain output (logging it to the DB).

I'm thinking about a way to refactor the code and represent the condition tree in a compact fashion, so that it could be a bit easier to read, maintain and log.

I've thought about a bidimensional matrix having a row for every condition made up of 4 elements

the condition id
condition id (or return value) if current condition is true
condition id (or return value) if current condition is false
the condition itself

so, at the end i'd have this matrix

Object[][]matrix=
    {
            {10,20,70,  
                equalz(in.a, "LOC")},

            {20,30,80,  
                (equalz(tmp.b, "BA")
                && notEquals(in.c,"U")
                && equalz(in.d,"Y")
                )||(
                equalz(tmp.b, "HV")
                && notEquals(in.c,"U")
                && equalz(in.d,"Y")
                && varEqualsOneOf(in.e,"FLG","FLR","RRG"))},

            {30,40,70,  
                equalz(tmp.b, "BA")},

            {40,Action.AC015,50,    
                varEqualsOneOf(in.e,"RRG","NL")
                && lessThan(in.f,in.g)},

            {50,Action.AC015,Action.AC000,  
                varEqualsOneOf(in.e,"FLG","FLR")},

            {60,Action.AC014,Action.AC010,  
                equalz(in.e,"RRG")},

            {70,Action.AC011,Action.AC000,  
                equalsOrMissing(in.c,"U")   
                && varEqualsOneOf(in.e,"FLG","FLR")},

            {80,90,Action.AC000,    
                varEqualsOneOf(in.a,"MNC","BAN","LCI","CTV","LEA","INS")},

            {90,Action.AC000,100,   
                equalz(in.h,"A")|| equalz(in.i,"Y")},

            {100,110,210,   
                notEquals(in.h,"U")},

            {110,Action.AC000,120,  
                greaterThan(in.j,in.k)},

            {120,130,150,   
                varEqualsOneOf(in.e,"FLG","FLR")},

            {130,Action.AC004,140,  
                notEquals(in.l,"U")},

            {140,Action.AC012,Action.AC002, 
                oneOfVarsEqual(in.m,in.n,"Y")},

            {150,160,190,   
                    (equalz(in.e,"RRG")
                    && varEqualsOneOf(in.a,"MNC","BAN","LCI","CTV","LEA")   
                    )||(
                    varEqualsOneOf(in.e,"RRG","NL")
                    && equalz(in.a,"INS"))},

            {160,170,180,   
                    notEquals(in.l,"U")},

            {170,Action.AC005,Action.AC018, 
                    notEquals(in.o,"U")},

            {180,Action.AC012,Action.AC002, 
                    equalz(in.n,"Y")},

            {190,200,Action.AC000,  
                    equalz(in.p,in.q)},

            {200,Action.AC006,Action.AC008, 
                    notEquals(in.o,"U")},

            {210,Action.AC004,Action.AC000, 
                    notEquals(in.l,"U")
                    && varEqualsOneOf(in.e,"FLG","FLR")}
    }
;

driven by a method that jumps between conditions an logs the whole execution representing the path with the conditions' ID.

At the beginning I thought this could have been a good idea, but I'm not that sure now that it's written down.

How would you handle this problem? Is my solution totally worthless? Is there any other better way to achieve the goal?

This question probably belongs on [Code Review](https://codereview.stackexchange.com/). — David Arno, Aug 14 '17 at 13:23
Uhm, if that's the case I'll move the question there. I thought this could be the right place given that I'm not really interested in the code, but in higher level ideas to handle this somehow "generic" problem. — Luigi Cortese, Aug 14 '17 at 13:27
1. As painful as it will be, don't forget to write tests before refactoring. 2. What does `bp(false)` do? There are a couple of those in the code. 3. Whatever solution you choose, please use more meaningful names than these — Vincent Savard, Aug 14 '17 at 13:38
@VincentSavard 1) I have a very big set of input-ouput previously calculated to use as test after the refactoring 2) bp(boolean x) was an attempt to build a path to be logged, so that after every evaluation, a bp(true) or bp(false) would be invoked. I forgot to remove some of them 3) original names are more meaningful than that =) — Luigi Cortese, Aug 14 '17 at 13:46
Also beware the insidious `if {} if{}` combination. They are completely different blocks without any `else` condition joining them. I've run into too much code that had two `if` blocks formatted as if they were an `else` statement. — Berin Loritsch, Aug 14 '17 at 14:10
Possible duplicate of [Elegant ways to handle if(if else) else](https://softwareengineering.stackexchange.com/questions/122485/elegant-ways-to-handle-ifif-else-else) — gnat, Aug 14 '17 at 15:06
Is any of this data in a database? Can it be to maybe sort it out? There is no way anyone could get out of rewriting this whole thing. — johnny, Aug 15 '17 at 19:19
To clarify my response, which of the "original names are more meaningful"? I see 3 huge areas for improvement: the comments (What does "20", "30" mean?), the String constants, and the Action.ACnnns. Are all of them "meaningful" in the original code? — user949300, Aug 15 '17 at 22:31

score 8 · Answer 1 · answered Aug 14 '17 at 14:44

I wouldn't go with the matrix idea. While it is less code, I don't see it as being any more readable. If anything, I think it's harder to read because I can't mentally evaluate one thing and then disregard half the code left in the method. I have to check everything.

I would start by just working on reducing nesting. For example, looking at the outermost if block and it's else block you have this:

if (equalz(in.a, "LOC")) {
    ...
} else {
    if(varEqualsOneOf(in.a,"MNC","BAN","LCI","CTV","LEA","INS")) {
        ...
    } else {
        return Action.AC000;
    }
}

Since the outermost else block only has another nested if-else and no common code, you can move that out a level so you have this:

if (equalz(in.a, "LOC")) {
    ...
} else if(varEqualsOneOf(in.a,"MNC","BAN","LCI","CTV","LEA","INS")) {
    ...
} else {
    return Action.AC000;
}

If we apply that same principle to this block:

if(varEqualsOneOf(in.e,"RRG","NL")//40
    && lessThan(in.f,in.g)) {
    return Action.AC015;
} else {
    if(varEqualsOneOf(in.e,"FLG","FLR")) {//50
        return Action.AC015;
    } else {
        return Action.AC000;
    }
}

we get:

if(varEqualsOneOf(in.e,"RRG","NL")//40
    && lessThan(in.f,in.g)) {
    return Action.AC015;
} else if(varEqualsOneOf(in.e,"FLG","FLR")) {//50
    return Action.AC015;
} else {
    return Action.AC000;
}

Wait a minute. The if and the else if branch return the same thing. We can combine those. If we do so, we get:

if((varEqualsOneOf(in.e,"RRG","NL")/*40*/ && lessThan(in.f,in.g))
    || (varEqualsOneOf(in.e,"FLG","FLR") /*50*/)) {
    return Action.AC015;
} else {
    return Action.AC000;
}

Now that if condition is getting a bit messy, especially with all the poor names and magic numbers. This is where I would start making helper functions that just encapsulate the conditions and give them meaningful names. So I would get something like this:

if(IsCondition40(in) || IsCondition50(in)) {
    return Action.AC015;
} else {
    return Action.AC000;
}
.....
// obviously these still need better names but you get the idea.  Magic strings and numbers should be replaced, etc.
// also, forgive my poor java, not a language I use often enough to be any good at it
private boolean IsCondition40(In in) {
    return varEqualsOneOf(in.e,"RRG","NL") && lessThan(in.f,in.g);
}
private boolean IsCondition50(In in) {
    return varEqualsOneOf(in.e,"FLG","FLR");
}

And now it is starting to get somewhere. Keep picking it apart step by step and it will start to get better. Don't forget to run your tests after each go to make sure you didn't break something (you did make tests before starting this, right?).

Look for common return values and conditions and combine them or make helper functions. Reduce the nesting. Eventually you will get something much more readable.

I would also be tempted to move the IsConditionXX() methods into the **in** class, if possible. Of maybe `int ToSeeWhatConditionMyConditionWasIn()` :-) — user949300, Aug 15 '17 at 22:21

user949300 · Answer 2 · 2017-08-15T17:27:28.990

6

Change all of those "magic constant" Strings to meaningfully named constants. What the heck is "FLG"?
Then change all the meaningless action names. "Action.AC006" is not a useful name.
Change the meaningless comments from "//20", "//30"... (unless these are somehow clear to somebody working on the project) They don't seem to match up with the ACxxx numbers.

Only then try some of the refactorings mentioned in the other answers. Refactoring garbage in leads to refactored garbage out. :-)

And Another Thing Regarding OOP

Other than the value of tmp.b, all of the if statements involve the in object. (Unless I missed one...) Tell Don't Ask (or see this SO answer) suggests that the decide() method, however it gets rewritten, be moved to the in object. e.g.

public Action decideAction(String whateverTmpBReallyMeans);

This may or may not make sense (or even be possible) in your project, but it's worth considering.

edited Aug 15 '17 at 17:27

answered Aug 14 '17 at 23:52

user949300

8,679
2
26
35

A missing or non-descriptive label on a product case doesn't make the product garbage. It only means that you have no idea what's inside. The variable names and the structure of the `if` tree are unrelated to each other. Also, the OP hinted in the comments that the original names are more meaningful, so the variable names in the code provided might be replacements. – Stingy Aug 15 '17 at 10:43
@Stingy Well, with code you are generally _supposed to know what's inside_. Perhaps the original names are better. Did OP also change the comments? The comments `//20`, `//30` ... are hardly useful either, since they don't appear to match up with the ActionAC0xx numbers? – user949300 Aug 15 '17 at 16:31
I really can't believe that an answer that suggests giving variables meaningful names, in an example where it strongly applies, has received a downvote. – user949300 Aug 15 '17 at 17:04
@user949300 Does "give meaningful names" answer the question? As stated in a comment there are apparently meaningful names which I guess cannot be disclosed. This could just as easily have been a comment to the OP to transform the random gibberish into variable names and constants which have meaning to clarify the question. – Robzor Aug 15 '17 at 17:33
@user949300 I don't know if your comment about downvoting your answer was addressed at me, but I didn't downvote your answer. I didn't even object to your advice of giving variables meaningful names. My point was simply that the lack of organization in the `if`-tree is a completely separate matter from the variable names, and that refactoring the `if`-tree without changing the variable names is not necessarily worthless. – Stingy Aug 15 '17 at 21:07
On further review, I do now see the buried comment about original names being better. Whether this applied to the String constants, the Action.XXXs, the comments, or the `in` and `tmp` I don't know, but **all four** could use improvement. Added the OOPish section to add more depth to the answer. – user949300 Aug 15 '17 at 22:27

score 3 · Answer 3 · answered Aug 14 '17 at 14:30

I might not be answering your question here but allow me to give some knee-jerk reflections I had as I read the code.

This piece of code has many things which makes it seemingly unreadable to me and it stems from a lot the lack of understanding regarding what segments do and it being factored weirdly. I understand your desire to be able to follow the flow that got you to a certain output but this seems to simply be an effect of badly formatted code. If code is easy to follow you do not need a complex matrix to determine the path taken to a return statement.

Let's take this for starters:

if(( //20
        equalz(tmp.b, "BA")
        && notEquals(in.c,"U")
        && equalz(in.d,"Y")
    )||(
        equalz(tmp.b, "HV")
        && notEquals(in.c,"U")
        && equalz(in.d,"Y")
        && varEqualsOneOf(in.e,"FLG","FLR","RRG"))
) {

This is could be contained within a function call to better describe what actually is happening. Something simple such as the following would do the trick:

private boolean isFoobar(In in, Tmp tmp) {
     return (equalz(tmp.b, "BA") && notEquals(in.c,"U") && equalz(in.d,"Y"))
                || (equalz(tmp.b, "HV") && notEquals(in.c,"U") && equalz(in.d,"Y") && varEqualsOneOf(in.e,"FLG","FLR","RRG"));
}

Doing so increases reusability of the check while also making the code more readable. This because you reduce the amount of visual clutter while also, by giving the function a descriptive name, explain to the reader what you actually are trying to check.

The following segment could also benefit from the above approach once it has been refactored:

if(varEqualsOneOf(in.e,"RRG","NL")//40
    && lessThan(in.f,in.g)) {
    return Action.AC015;
} else {
    if(varEqualsOneOf(in.e,"FLG","FLR")) {//50
        return Action.AC015;
    } else {
        return Action.AC000;
    }
}

Has the logical equivalent:

if((varEqualsOneOf(in.e,"RRG","NL") && lessThan(in.f,in.g))
        || varEqualsOneOf(in.e,"FLG","FLR")) {
    return Action.AC015;
} else {
    return Action.AC000;
}

The statement:

return Action.AC000;

Is littered all over the place and seems to be the default ending point. In a few of the cases it seems reasonable to include it in one of the if-statements but consider using as a default return statement instead and removing a lot of the:

else {
    return Action.AC000;
}

If those Action items hold significance in just a few cases would it not be possible to create separate actions here which would also then signify the path taken? Consider the following statement:

if(greaterThan(in.j,in.k)) {//110
    return Action.AC000;
}

Which could be:

if(greaterThan(in.j,in.k)) {//110
    return Action.AC123;
}

My bottom line is the following:

The code you have can be vastly improved for readability purposes
First focus your efforts into readability refinements
Try to reduce depth of the if-statements
Try to return Action elements uniquely so that you don't have to do all the tracking for conditions but can simply just find where it was returned
The path matrix should only be used in case you cannot create clean code which is possible to follow by doing the above

Tracking the path might really be the way to go in your case, because as always it depends on the problem at hand but if I were you I would concentrate my efforts elsewhere first.

P.S. The varied formatting style used in this segment is really triggering my OCD.

score 2 · Answer 4 · answered Aug 15 '17 at 19:01

One option is to go with a rule set implementation. Essentially you define an interface for all the values you need to evaluate. Something like:

interface Rule {
    boolean matches(Thing a, Thing b); // etc...
}

Then you create instances for each scenario. This is basically equivalent to the matrix solution you suggested. The nesting disappears as each rule stands on it own. Ordering maybe important and that could be a downside.

Whether this is a good idea depends on whether you know what the actual rules are that came to define this mess or you can work back to them. One big advantage is this lends itself to more dynamic solutions such as putting rules in a DB or creating a DSL and even a real rules engine, if you are brave enough to venture down that path.

Stingy · Answer 5 · 2017-08-15T21:54:23.130

I was about to answer your question on Code Review, but then it was closed there as being off-topic, so I tracked this question here. I have the same suspicion as Robzor, which is that the actual problem is not that the if tree is long, or that it's an if tree at all. Rather, I think that the problem is simply that the if tree is a horrible mess – Robzor's description of it being "unreadable" doesn't seem to be an overstatement at all to me. The other two answers have already pointed out many problems with the code, but I have still some to add, for example, there are some duplicate checks, like in the beginning:

if((
            equalz(tmp.b, "BA")
            && notEquals(in.c,"U")
            && equalz(in.d,"Y")
        )||(
            equalz(tmp.b, "HV")
            && notEquals(in.c,"U")
            && equalz(in.d,"Y")
            && varEqualsOneOf(in.e,"FLG","FLR","RRG"))
    )

Here, the check for notEquals(in.c,"U") && equalz(in.d,"Y") can be extracted to the outside, since it is common for both OR arguments.

Also, I wonder what notEquals(in.c,"U") returns when in.c is missing. If it returns false, then notEquals is equivalent to !equalsOrMissing, meaning one of the two methods is redundant. And if it returns true, then notEquals is equivalent to !equalz, so one method is redundant either way.

If you have to deal with more of these inhumane monstrosities, I think the only way to get somewhere with them other than to the stages of desperation and insanity is to refactor the if trees so that it's actually possible to look at them without crying from sheer terror. I tried to do this with the code you provided in order to get an impression what logic actually hides behind this near-impenetrable facade of spaghetti code, and here is what I got:

@Override
public Action decide() {
    if (equalz(in.a, "LOC")) {
        if (notEquals(in.c, "U")
                && equalz(in.d, "Y")) {
            if (equalz(tmp.b, "BA")
                    && (varEqualsOneOf(in.e, "RRG", "NL")
                    && lessThan(in.f, in.g)
                    || varEqualsOneOf(in.e, "FLG", "FLR"))) {
                return Action.AC015;
            } else if (equalz(tmp.b, "HV")) {
                if (equalz(in.e, "RRG")) {
                    return Action.AC014;
                } else if (varEqualsOneOf(in.e, "FLG", "FLR")) {
                    return Action.AC010;
                }
            }
        } else if (!varEqualsOneOf(tmp.b, "BA", "HV")
                && equalsOrMissing(in.c, "U")
                && varEqualsOneOf(in.e, "FLG", "FLR")) {
            return Action.AC011;
        }
    } else if (varEqualsOneOf(in.a, "MNC", "BAN", "LCI", "CTV", "LEA", "INS")
            && !equalz(in.h, "A")
            && !equalz(in.i, "Y")
            && (notEquals(in.h, "U")
            && !greaterThan(in.j, in.k)
            || !notEquals(in.h, "U")
            && notEquals(in.l, "U")
            && varEqualsOneOf(in.e, "FLG", "FLR"))) {
        if (varEqualsOneOf(in.e, "FLG", "FLR", "RRG")
                || equalz(in.e, "NL")
                && equalz(in.a, "INS")) {
            if (!notEquals(in.l, "U")) {
                if (equalz(in.n, "Y")
                        || varEqualsOneOf(in.e, "FLG", "FLR")
                        && equalz(in.m, "Y")) {
                    return Action.AC012;
                } else {
                    return Action.AC002;
                }
            } else if (varEqualsOneOf(in.e, "FLG", "FLR")) {
                return Action.AC004;
            } else if (notEquals(in.o, "U")) {
                return Action.AC005;
            } else {
                return Action.AC018;
            }
        } else if (equalz(in.p, in.q)) {
            if (notEquals(in.o, "U")) {
                return Action.AC006;
            } else {
                return Action.AC008;
            }
        }
    }
    return Action.AC000;
}

I don't think it can get any simpler than this. It still looks complicated, but then, the algorithm itself seems to be complicated, and there's no way around that. I also might have made mistakes along the way, so don't trust this code to be equivalent to yours. But this simplified version of your original code, be it bug-free or not, might give you a basis for re-thinking whether it is really the nature if an if tree you are unhappy with, or simply the structure of the particular if tree whose lucky inheritor you are. And even if you decide that you would like to restructure the code, it will still be easier by orders of magnitude once you have disentangled the many branches of the if tree.

Wow, "good" work I guess! Convinces me that something else is needed to handle this mess, though not sure what. — user949300, Aug 15 '17 at 17:08

score 0 · Answer 6 · answered Aug 16 '17 at 19:26

I would consider a database.

       {20,30,80,  
            (equalz(tmp.b, "BA")
            && notEquals(in.c,"U")
            && equalz(in.d,"Y")
            )||(
            equalz(tmp.b, "HV")
            && notEquals(in.c,"U")
            && equalz(in.d,"Y")
            && varEqualsOneOf(in.e,"FLG","FLR","RRG"))},

I don't know enough about the data, but it seems those objects don't change much. Maybe,

Action

action_id (auto-increment)
action
description


1, AC000, something
2, AC015, somethingelse

action_condition

action_condition_id (auto-increment)
condition_id
action_id
condition

1, 40, 2, RRG
2, 40, 2, NL
...

SELECT * FROM action a, action_condition ac WHERE a.action_id = ac.action_id AND ? = ?

//Build your SELECT, used Prepared Statements, 
//be careful constructing to disallow SQL Injection.

Before I even attempted it, however, I'd rename everything.

How to represent a long if-else tree in a concise manner

6 Answers6