If…else is not bad — excessive usage is It's a simple, easy-to-understand, and flexible logic control structure. But in reality, it's often used excessively. They can be bad in the following cases: Nested if-else or multiple level nesting (worse)
Nested IFs are powerful, but they become complicated quickly as you add more levels. One way to avoid more levels is to use IF in combination with the AND and OR functions. These functions return a simple TRUE/FALSE result that works perfectly inside IF, so you can use them to extend the logic of a single IF.
The only strict limit is that there can only be one if and one else per if/elif/else statement block, but there is no limit on the number of elif . Why can't I use an else statement?
If you cannot come up with a formula, you can use a table for such a limited number of outcomes:
final int[][] result = new int[][] {
{ 0, 0, 1, 2 },
{ 0, 0, 2, 1 },
{ 2, 1, 3, 3 },
{ 1, 2, 3, 3 }
};
return result[one][two];
Since your data set is so small, you can compress everything into 1 long integer and turn it into a formula
public int fightMath(int one,int two)
{
return (int)(0xF9F66090L >> (2*(one*4 + two)))%4;
}
This makes use of the fact everything is a multiple of 2
public int fightMath(int one,int two)
{
return (0xF9F66090 >> ((one << 3) | (two << 1))) & 0x3;
}
What can I say? The world needs magic, sometimes the possibility of something calls for its creation.
The essence of the function that solves OP's problem is a map from 2 numbers (one,two), domain {0,1,2,3} to the range {0,1,2,3}. Each of the answers has approached how to implement that map.
Also, you can see in a number of the answers a restatement of the problem as a map of 1 2-digit base 4 number N(one,two) where one is digit 1, two is digit 2, and N = 4*one + two; N = {0,1,2,...,15} -- sixteen different values, that's important. The output of the function is one 1-digit base 4 number {0,1,2,3} -- 4 different values, also important.
Now, a 1-digit base 4 number can be expressed as a 2-digit base 2 number; {0,1,2,3} = {00,01,10,11}, and so each output can be encoded with only 2 bits. From above, there are only 16 different outputs possible, so 16*2 = 32 bits is all that is necessary to encode the entire map; this can all fit into 1 integer.
The constant M is an encoding of the map m where m(0) is encoded in bits M[0:1], m(1) is encoded in bits M[2:3], and m(n) is encoded in bits M[n*2:n*2+1].
All that remains is indexing and returning the right part of the constant, in this case you can shift M right 2*N times and take the 2 least significant bits, that is (M >> 2*N) & 0x3. The expressions (one << 3) and (two << 1) are just multiplying things out while noting that 2*x = x << 1 and 8*x = x << 3.
I don't like any of the solutions presented except for JAB's. None of the others make it easy to read the code and understand what is being computed.
Here's how I would write this code -- I only know C#, not Java, but you get the picture:
const bool t = true;
const bool f = false;
static readonly bool[,] attackResult = {
{ f, f, t, f },
{ f, f, f, t },
{ f, t, t, t },
{ t, f, t, t }
};
[Flags] enum HitResult
{
Neither = 0,
PlayerOne = 1,
PlayerTwo = 2,
Both = PlayerOne | PlayerTwo
}
static HitResult ResolveAttack(int one, int two)
{
return
(attackResult[one, two] ? HitResult.PlayerOne : HitResult.Neither) |
(attackResult[two, one] ? HitResult.PlayerTwo : HitResult.Neither);
}
Now it is much more clear what is being computed here: this emphasizes that we are computing who gets hit by what attack, and returning both results.
However this could be even better; that Boolean array is somewhat opaque. I like the table lookup approach but I would be inclined to write it in such a way that made it clear what the intended game semantics were. That is, rather than "an attack of zero and a defense of one results in no hit", instead find a way to make the code more clearly imply "a low kick attack and a low block defense results in no hit". Make the code reflect the business logic of the game.
You can create matrix which contains results
int[][] results = {{0, 0, 1, 2}, {0, 0, 2, 1},{2, 1, 3, 3},{2, 1, 3, 3}};
When you want to get value you will use
public int fightMath(int one, int two) {
return this.results[one][two];
}
Other people have already suggested my initial idea, the matrix method, but in addition to consolidating the if statements you can avoid some of what you have by making sure the arguments supplied are in the expected range and by using in-place returns (some coding standards I've seen enforce one-point-of-exit for functions, but I've found that multiple returns are very useful for avoiding arrow coding and with the prevalence of exceptions in Java there's not much point in strictly enforcing such a rule anyway as any uncaught exception thrown inside the method is a possible point of exit anyway). Nesting switch statements is a possibility, but for the small range of values you're checking here I find if statements to be more compact and not likely to result in much of a performance difference, especially if your program is turn-based rather than real-time.
public int fightMath(int one, int two) {
if (one > 3 || one < 0 || two > 3 || two < 0) {
throw new IllegalArgumentException("Result is undefined for arguments outside the range [0, 3]");
}
if (one <= 1) {
if (two <= 1) return 0;
if (two - one == 2) return 1;
return 2; // two can only be 3 here, no need for an explicit conditional
}
// one >= 2
if (two >= 2) return 3;
if (two == 1) return 1;
return 2; // two can only be 0 here
}
This does end up being less readable than it might otherwise be due to the irregularity of parts of the input->result mapping. I favor the matrix style instead due to its simplicity and how you can set up the matrix to make sense visually (though that is in part influenced by my memories of Karnaugh maps):
int[][] results = {{0, 0, 1, 2},
{0, 0, 2, 1},
{2, 1, 3, 3},
{2, 1, 3, 3}};
Update: Given your mention of blocking/hitting, here's a more radical change to the function that utilizes propertied/attribute-holding enumerated types for inputs and the result and also modifies the result a little to account for blocking, which should result in a more readable function.
enum MoveType {
ATTACK,
BLOCK;
}
enum MoveHeight {
HIGH,
LOW;
}
enum Move {
// Enum members can have properties/attributes/data members of their own
ATTACK_HIGH(MoveType.ATTACK, MoveHeight.HIGH),
ATTACK_LOW(MoveType.ATTACK, MoveHeight.LOW),
BLOCK_HIGH(MoveType.BLOCK, MoveHeight.HIGH),
BLOCK_LOW(MoveType.BLOCK, MoveHeight.LOW);
public final MoveType type;
public final MoveHeight height;
private Move(MoveType type, MoveHeight height) {
this.type = type;
this.height = height;
}
/** Makes the attack checks later on simpler. */
public boolean isAttack() {
return this.type == MoveType.ATTACK;
}
}
enum LandedHit {
NEITHER,
PLAYER_ONE,
PLAYER_TWO,
BOTH;
}
LandedHit fightMath(Move one, Move two) {
// One is an attack, the other is a block
if (one.type != two.type) {
// attack at some height gets blocked by block at same height
if (one.height == two.height) return LandedHit.NEITHER;
// Either player 1 attacked or player 2 attacked; whoever did
// lands a hit
if (one.isAttack()) return LandedHit.PLAYER_ONE;
return LandedHit.PLAYER_TWO;
}
// both attack
if (one.isAttack()) return LandedHit.BOTH;
// both block
return LandedHit.NEITHER;
}
You don't even have to change the function itself if you want to add blocks/attacks of more heights, just the enums; adding additional types of moves will probably require modification of the function, though. Also, EnumSet
s might be more extensible than using extra enums as properties of the main enum, e.g. EnumSet<Move> attacks = EnumSet.of(Move.ATTACK_HIGH, Move.ATTACK_LOW, ...);
and then attacks.contains(move)
rather than move.type == MoveType.ATTACK
, though using EnumSet
s will probably be slightly slower than direct equals checks.
For the case where a successful block results in a counter, you can replace if (one.height == two.height) return LandedHit.NEITHER;
with
if (one.height == two.height) {
// Successful block results in a counter against the attacker
if (one.isAttack()) return LandedHit.PLAYER_TWO;
return LandedHit.PLAYER_ONE;
}
Also, replacing some of the if
statements with usage of the ternary operator (boolean_expression ? result_if_true : result_if_false
) could make the code more compact (for example, the code in the preceding block would become return one.isAttack() ? LandedHit.PLAYER_TWO : LandedHit.PLAYER_ONE;
), but that can lead to harder-to-read oneliners so I wouldn't recommend it for more complex branching.
Why not use an array?
I will start from the beginning. I see a pattern, the values goes from 0 to 3 and you want catch all possible values. This is your table:
0 & 0 = 0
0 & 1 = 0
0 & 2 = 1
0 & 3 = 2
1 & 0 = 0
1 & 1 = 0
1 & 2 = 2
1 & 3 = 1
2 & 0 = 2
2 & 1 = 1
2 & 2 = 3
2 & 3 = 3
3 & 0 = 2
3 & 1 = 1
3 & 2 = 3
3 & 3 = 3
when we look at this same table binary we see the following results:
00 & 00 = 00
00 & 01 = 00
00 & 10 = 01
00 & 11 = 10
01 & 00 = 00
01 & 01 = 00
01 & 10 = 10
01 & 11 = 01
10 & 00 = 10
10 & 01 = 01
10 & 10 = 11
10 & 11 = 11
11 & 00 = 10
11 & 01 = 01
11 & 10 = 11
11 & 11 = 11
Now maybe you already see some pattern but when I combine value one and two I see that you're using all values 0000, 0001, 0010,..... 1110 and 1111. Now let's combine value one and two to make a single 4 bit integer.
0000 = 00
0001 = 00
0010 = 01
0011 = 10
0100 = 00
0101 = 00
0110 = 10
0111 = 01
1000 = 10
1001 = 01
1010 = 11
1011 = 11
1100 = 10
1101 = 01
1110 = 11
1111 = 11
When we translate this back into decimal values we see an very possible array of values where the one and two combined could be used as index:
0 = 0
1 = 0
2 = 1
3 = 2
4 = 0
5 = 0
6 = 2
7 = 1
8 = 2
9 = 1
10 = 3
11 = 3
12 = 2
13 = 1
14 = 3
15 = 3
The array is then {0, 0, 1, 2, 0, 0, 2, 1, 2, 1, 3, 3, 2, 1, 3, 3}
, where it's index is simply one and two combined.
I'm not a Java programmer but you can get rid of all if statements and just write it down as something like this:
int[] myIntArray = {0, 0, 1, 2, 0, 0, 2, 1, 2, 1, 3, 3, 2, 1, 3, 3};
result = myIntArray[one * 4 + two];
I don't know if a bitshift by 2 is faster than multiplication. But it could be worth a try.
This uses a little bit of bitmagic (you're already doing it by holding two bits of information (low/high & attack/block) in a single integer):
I haven't run it, only typed it here, please doublecheck. The idea surely works.
EDIT: It is now tested for every input, works fine.
public int fightMath(int one, int two) {
if(one<2 && two<2){ //both players blocking
return 0; // nobody hits
}else if(one>1 && two>1){ //both players attacking
return 3; // both hit
}else{ // some of them attack, other one blocks
int different_height = (one ^ two) & 1; // is 0 if they are both going for the same height - i.e. blocker wins, and 1 if height is different, thus attacker wins
int attacker = one>1?1:0; // is 1 if one is the attacker, two is the blocker, and 0 if one is the blocker, two is the attacker
return (attacker ^ different_height) + 1;
}
}
Or should I suggest to separate the two bits of information into separate variables? Code based mostly on bit operations like this above is usually really hard to maintain.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With