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?
Best Answer
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:
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 we apply that same principle to this block:
we get:
Wait a minute. The if and the else if branch return the same thing. We can combine those. If we do so, we get:
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:
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.