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

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.

1. Change all of those "magic constant" Strings to meaningfully named constants. What the heck is "FLG"?
2. Then change all the meaningless action names. "Action.AC006" is not a useful name.
3. 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.

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.

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.

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.

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.