265

The following code does work how I need it to, but it's ugly, excessive or a number of other things. I've looked at formulas and attempted to write a few solutions, but I end up with a similar amount of statements.

Is there a type of math formula that would benefit me in this instance or are 16 if statements acceptable?

To explain the code, it's for a kind of simultaneous-turn-based game.. two players have four action buttons each and the results come from an array (0-3), but the variables 'one' & 'two' can be assigned anything if this helps. The result is, 0 = neither win, 1 = p1 wins, 2 = p2 wins, 3 = both win.

public int fightMath(int one, int two) {

    if(one == 0 && two == 0) { result = 0; }
    else if(one == 0 && two == 1) { result = 0; }
    else if(one == 0 && two == 2) { result = 1; }
    else if(one == 0 && two == 3) { result = 2; }
    else if(one == 1 && two == 0) { result = 0; }
    else if(one == 1 && two == 1) { result = 0; }
    else if(one == 1 && two == 2) { result = 2; }
    else if(one == 1 && two == 3) { result = 1; }
    else if(one == 2 && two == 0) { result = 2; }
    else if(one == 2 && two == 1) { result = 1; }
    else if(one == 2 && two == 2) { result = 3; }
    else if(one == 2 && two == 3) { result = 3; }
    else if(one == 3 && two == 0) { result = 1; }
    else if(one == 3 && two == 1) { result = 2; }
    else if(one == 3 && two == 2) { result = 3; }
    else if(one == 3 && two == 3) { result = 3; }

    return result;
}
6
  • 1
    @waqaslam:- This can help Java switch statement to handle two variables? Mar 19, 2014 at 9:34
  • 9
    Surely there is some logic here that can be generalised rather than brute forced? Surely there is some function f(a, b) that yields the answer in the general case? You haven't explained the logic of the calculation therefore all the answers are just lipstick on a pig. I would start by seriously rethinking your program logic, using int flags for actions is very outdated. enums can contain logic and are descriptive, this would allow you to write your code in a more modern manner. Mar 19, 2014 at 10:58
  • After reading the answers @Steve Benett provided in his alternate question linked above I can assume there is no straight forward formula answer to this as it is essentially the same as a database. I attempted to explain in the original question that I was making a simple game (fighter) and users have a selection of 4 buttons: blockHigh(0), blockLow(1), attackHigh(2) and attackLow(3). These numbers are held in an array until needed. Later on they are used by the function 'fightMath()' which calls playerOne's selections against playerTwos to give the result. No actual collision detection.
    – TomFirth
    Mar 19, 2014 at 12:37
  • 10
    If you have an answer, please post it as such. The extended discussion in the comments is hard to follow, especially when code is involved. If you want to talk about whether this question should have been migrated to Code Review, there's a Meta discussion about that. Mar 24, 2014 at 12:15
  • 1
    What do you mean by "the same as a database"? If these values are in the database, pull them from there. Otherwise, if it is really this complex, I would leave it like you have it and add business logic comments after each line so that people understand what is going on. It is better (to me) long and explicit - someone in the future can understand what is going on. If you put it in a map or try to save 8 lines of code, the upside is really small, and the downsize is bigger: you make it more and more confusing for someone who needs to read your code one day.
    – skaz
    Mar 26, 2014 at 6:53

26 Answers 26

604

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];
23
  • 8
    This is interesting as i've not seen this solution before. I'm not quite sure I understand the return result but will enjoy testing that.
    – TomFirth
    Mar 19, 2014 at 9:50
  • 4
    You don't need the assert, Java will throw an IndexOutOfBoundsException anyway if one or more indices are out of bounds.
    – JAB
    Mar 19, 2014 at 11:55
  • 44
    @JoeHarper If you want something easy to read, you won't be using magic numbers in the first place and you'll have a comment explaining the mapping. As it is, I prefer this version to the original, but for something maintainable in the long term I'd use an approach involving enumerated types or at least named constants.
    – JAB
    Mar 19, 2014 at 15:11
  • 14
    @JoeHarper "Theoretically" is one thing, "practically" is another. Of course I try to use descriptive naming (with the exception of the i/j/k convention for loop variables), named constants, arranging my code in a readable fashion, etc., but when the variable and function names start taking up more than 20 characters each I find it actually leads to less readable code. My usual approach is to try for comprehensible but succinct code with comments here and there to explain why the code is structured the way it is (vs. how). Putting the why in the names just clutters everything.
    – JAB
    Mar 19, 2014 at 16:08
  • 15
    I like this solution for this specific problem because the outcomes actually ARE dictated by an outcome matrix.
    – Trying
    Mar 20, 2014 at 16:10
200

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;
}

More bitwise variant:

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;
}

The Origin of the Magic Constant

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.

24
  • 83
    smart, but no one else reading the code will have a chance of understanding it. Mar 20, 2014 at 4:39
  • 112
    I think this is extremely bad practice. No one else except the author will understand that. You want to look at a piece of code and understand it quickly. But this is just a waste of time. Mar 20, 2014 at 10:57
  • 15
    I'm with @BalázsMáriaNémeth on this. Though very impressive, you should be coding for violent psychopaths!
    – OrhanC1
    Mar 20, 2014 at 11:34
  • 93
    All downvoters think that this is a hideous code smell. All upvoters think the same, but admire the cleverness behind it. +1 (Don't ever use this code.)
    – usr
    Mar 20, 2014 at 21:45
  • 5
    What a lovely example of write only code!
    – lealand
    Mar 21, 2014 at 4:22
99

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.

6
  • 68
    Nonsense. Most mildly experienced programmes will be able to appreciate the advice that is being given here, and apply the coding style to their own langauge. The question was how to avoid a string of ifs. This shows how. Mar 20, 2014 at 12:33
  • 7
    @user3414693: I am well aware that it is a Java question. If you read the answer carefully that becomes clear. If you believe that my answer is unwise then I encourage you to write your own answer that you like better. Mar 20, 2014 at 13:50
  • 1
    @EricLippert I like JAB's solution too. IMHO, enum type in C# leaves much to be desired. It doesn't follow the pit of success philosophy which rest of the features do. E.g. stackoverflow.com/a/847353/92414 Does c# team have any plan to create a new enum type (so as to avoid breaking existing code) which is designed better? Mar 20, 2014 at 17:35
  • @SolutionYogi: I don't much like enums in C# either, though they are the way they are for good historical reasons. (Mostly for compatibility with existing COM enums.) I'm not aware of any plans to add new gear for enums in C# 6. Mar 20, 2014 at 17:40
  • 3
    @SList no, comments don't run. OP did exactly what should be done; convert comments to clear code. See e.g. Steve McConnell Code Complete stevemcconnell.com/cccntnt.htm
    – djechlin
    Mar 24, 2014 at 12:46
87

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]; 
}
0
71

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, EnumSets 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 EnumSets 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.

10
  • I will definitely look into this but my current code allows me to use the int value of one and two to be reused as start points on my spritesheet. Although it doesn't require much additional code to allow for that.
    – TomFirth
    Mar 19, 2014 at 14:46
  • 2
    @TomFirth84 There's an EnumMap class that you could use to map the enums to your integer offsets (you could also use the ordinal values of the enum members directly, e.g. Move.ATTACK_HIGH.ordinal() would be 0, Move.ATTACK_LOW.ordinal() would be 1, etc., but that's more fragile/less flexible than explicitly associating each member with a value as adding enum values in between existing ones would throw off the count, which would not be the case with an EnumMap.)
    – JAB
    Mar 19, 2014 at 14:54
  • 7
    This is the most readable solution since it translates the code into something that is meaningful for the person reading the code. Mar 19, 2014 at 17:58
  • Your code, at least the one using enums, is wrong. Acording to the if statements in OP a succesfull block leads to a hit on the attacker. But +1 for meaningfull code.
    – Taemyr
    Mar 20, 2014 at 12:45
  • 2
    You can even add a attack(against) method to the Move enum, returning HIT when the move is a successful attack, BACKFIRE when the move is a blocked attack, and NOTHING when it's not an attack. This way you can implement it in general (public boolean attack(Move other) { if this.isAttack() return (other.isAttack() || other.height != this.height) ? HIT : BACKFIRE; return NOTHING; }), and override it on specific moves when needed (weak moves which any block can block, attacks that never backfire etc.)
    – rewritten
    Mar 20, 2014 at 21:34
50

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.

3
  • 2
    A bit-shift of 2 is almost definitely faster than a multiplication of 4. At best, the multiplication by 4, would recognize that 4 is 2^2 and do a bit shift itself(translated by the compiler potentially). Frankly, to me, the shift is more readable.
    – Cruncher
    Mar 19, 2014 at 15:32
  • I like your approach! It essentially flattens a 4x4 matrix into a 16 element array. Mar 19, 2014 at 20:17
  • 6
    Nowadays, if I'm not mistaken, the compiler will undoubtably recognize that you're multiplying by a power of two and optimize it accordingly. So for you, the programmer, the bit shift and the multiplication ought to have exactly the same performance. Mar 20, 2014 at 14:13
24

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.

3
  • 2
    I agree with this solution, looks a lot like what I had in mind in my comment on the main question. I would prefer to split it up into seperate variables, that would make it easier to add a middle attack for example in the future.
    – Yoh
    Mar 19, 2014 at 14:50
  • 2
    I just fixed some bugs in the code above after testing it, now it works well. Going further on the bit-manipulator's way I also came up with a one line solution, which is still not as mystical as the bitmask in other answers, but is still tricky enough to screw your mind: return ((one ^ two) & 2) == 0 ? (one & 2) / 2 * 3 : ((one & 2) / 2 ^ ((one ^ two) & 1)) + 1;
    – elias
    Mar 19, 2014 at 22:21
  • 1
    This is the best answer as any new programmer reading it will actually understand the magic happening behind all of those magic numbers.
    – bezmax
    Mar 20, 2014 at 18:31
21

To be quite honest, everyone has their own style of code. I wouldn't have thought performance would be affected too much. If you understand this better than using a switch case version, then carry on using this.

You could nest the ifs , so potentially there would be a slight performance increase for your last if checks as it wouldn't have gone through as many if statements. But in your context of a basic java course it probably won't benefit.

else if(one == 3 && two == 3) { result = 3; }

So, instead of...

if(one == 0 && two == 0) { result = 0; }
else if(one == 0 && two == 1) { result = 0; }
else if(one == 0 && two == 2) { result = 1; }
else if(one == 0 && two == 3) { result = 2; }

You'd do...

if(one == 0) 
{ 
    if(two == 0) { result = 0; }
    else if(two == 1) { result = 0; }
    else if(two == 2) { result = 1; }
    else if(two == 3) { result = 2; }
}

And just reformat it as you'd prefer.

This doesn't make the code look better, but potentially speeds it up a little I believe.

2
  • 4
    Don't know if it's really good practice but for this case, I would probably use nested switch statements. It would take more space but it would be really clear.
    – dyesdyes
    Mar 19, 2014 at 11:46
  • That would also work, but I guess it's a matter of preference. I actually prefer if statements as it's virtually saying what the code is doing. Not putting down your opinion of course, whatever works for you :). Upvote for the alternative suggestion though!
    – Joe Harper
    Mar 19, 2014 at 11:50
12

Let's see what we know

1: your answers are symmetrical for P1 (player one) and P2 (player two). This makes sense for a fighting game but is also something you can take advantage of to improve your logic.

2: 3 beats 0 beats 2 beats 1 beats 3. The only cases not covered by these cases are combinations of 0 vs 1 and 2 vs 3. To put it another way the unique victory table looks like this: 0 beats 2, 1 beats 3, 2 beats 1, 3 beats 0.

3: If 0/1 go up against each other then there's a hitless draw but if 2/3 go up against each then both hit

First, let us build a one-way function telling us if we won:

// returns whether we beat our opponent
public boolean doesBeat(int attacker, int defender) {
  int[] beats = {2, 3, 1, 0};
  return defender == beats[attacker];
}

We can then use this function to compose the final result:

// returns the overall fight result
// bit 0 = one hits
// bit 1 = two hits
public int fightMath(int one, int two)
{
  // Check to see whether either has an outright winning combo
  if (doesBeat(one, two))
    return 1;

  if (doesBeat(two, one))
    return 2;

  // If both have 0/1 then its hitless draw but if both have 2/3 then they both hit.
  // We can check this by seeing whether the second bit is set and we need only check
  // one's value as combinations where they don't both have 0/1 or 2/3 have already
  // been dealt with 
  return (one & 2) ? 3 : 0;
}

While this is arguably more complex and probably slower than the table lookup offered in many answers I believe it is a superior method because it actually encapsulates the logic of your code and describes it to anyone who's reading your code. I think this makes it a better implementation.

(It's been a while since I did any Java so apologies if the syntax is off, hopefully it is still intelligible if I've got it slightly wrong)

By the way, 0-3 clearly mean something; they're not arbitrary values so it would help to name them.

11

I hope I understand the logic correctly. How about something like:

public int fightMath (int one, int two)
{
    int oneHit = ((one == 3 && two != 1) || (one == 2 && two != 0)) ? 1 : 0;
    int twoHit = ((two == 3 && one != 1) || (two == 2 && one != 0)) ? 2 : 0;

    return oneHit+twoHit;
}

Checking one hit high or one hit low is not blocked and the same for player two.

Edit: Algorithm was not fully understood, "hit" awarded when blocking which I did not realize (Thx elias):

public int fightMath (int one, int two)
{
    int oneAttack = ((one == 3 && two != 1) || (one == 2 && two != 0)) ? 1 : (one >= 2) ? 2 : 0;
    int twoAttack = ((two == 3 && one != 1) || (two == 2 && one != 0)) ? 2 : (two >= 2) ? 1 : 0;

    return oneAttack | twoAttack;
}
3
  • Way to find the patter great result!
    – Chad
    Mar 20, 2014 at 13:44
  • 1
    I like the approach, but I'm afraid this solution completely lacks the possibility of hitting by blocking an attack (e.g. if one=0 and two=2 it returns 0, but 1 is expected according to the specification). Maybe you can work on it to get it right, but I'm not sure that the resulting code would be still this elegant, as this means that the lines will grow somewhat longer.
    – elias
    Mar 20, 2014 at 19:18
  • Did not realize that a "hit" was awarded for block. Thanks for pointing it out. Adjusted with very simple fix.
    – Chris
    Mar 20, 2014 at 19:50
10

I don't have experience with Java so there might be some typos. Please consider the code as pseudo-code.

I'd go with a simple switch. For that, you'd need a single number evaluation. However, for this case, since 0 <= one < 4 <= 9 and 0 <= two < 4 <= 9, we can convert both ints to a simple int by multiplying one by 10 and adding two. Then use a switch in the resulting number like this:

public int fightMath(int one, int two) {
    // Convert one and two to a single variable in base 10
    int evaluate = one * 10 + two;

    switch(evaluate) {
        // I'd consider a comment in each line here and in the original code
        // for clarity
        case 0: result = 0; break;
        case 1: result = 0; break;
        case 1: result = 0; break;
        case 2: result = 1; break;
        case 3: result = 2; break;
        case 10: result = 0; break;
        case 11: result = 0; break;
        case 12: result = 2; break;
        case 13: result = 1; break;
        case 20: result = 2; break;
        case 21: result = 1; break;
        case 22: result = 3; break;
        case 23: result = 3; break;
        case 30: result = 1; break;
        case 31: result = 2; break;
        case 32: result = 3; break;
        case 33: result = 3; break;
    }

    return result;
}

There's another short method that I just want to point out as a theoretical code. However I wouldn't use it because it has some extra complexity that you don't normally want to deal with. The extra complexity comes from the base 4, because the counting is 0, 1, 2, 3, 10, 11, 12, 13, 20, ...

public int fightMath(int one, int two) {
    // Convert one and two to a single variable in base 4
    int evaluate = one * 4 + two;

    allresults = new int[] { 0, 0, 1, 2, 0, 0, 2, 1, 2, 1, 3, 3, 1, 2, 3, 3 };

    return allresults[evaluate];
}

Really just additional note, in case I'm missing something from Java. In PHP I'd do:

function fightMath($one, $two) {
    // Convert one and two to a single variable in base 4
    $evaluate = $one * 10 + $two;

    $allresults = array(
         0 => 0,  1 => 0,  2 => 1,  3 => 2,
        10 => 0, 11 => 0, 12 => 2, 13 => 1,
        20 => 2, 21 => 1, 22 => 3, 23 => 3,
        30 => 1, 31 => 2, 32 => 3, 33 => 3 );

    return $allresults[$evaluate];
}
2
  • Java has no lamdas before 8th version Mar 20, 2014 at 20:09
  • 1
    This. For such a small number of inputs, I'd use a switch with a composite value (though it may be more readable with a multiplier greater than 10, such as 100 or 1000).
    – Medinoc
    Mar 24, 2014 at 14:57
7

Since you prefer nested if conditionals , here's another way.
Note that it doesn't use the result member and it doesn't change any state.

public int fightMath(int one, int two) {
    if (one == 0) {
      if (two == 0) { return 0; }
      if (two == 1) { return 0; }
      if (two == 2) { return 1; }
      if (two == 3) { return 2; }
    }   
    if (one == 1) {
      if (two == 0) { return 0; }
      if (two == 1) { return 0; }
      if (two == 2) { return 2; }
      if (two == 3) { return 1; }
    }
    if (one == 2) {
      if (two == 0) { return 2; }
      if (two == 1) { return 1; }
      if (two == 2) { return 3; }
      if (two == 3) { return 3; }
    }
    if (one == 3) {
      if (two == 0) { return 1; }
      if (two == 1) { return 2; }
      if (two == 2) { return 3; }
      if (two == 3) { return 3; }
    }
    return DEFAULT_RESULT;
}
10
  • why don't you have any else's?
    – FDinoff
    Mar 19, 2014 at 19:04
  • 3
    @FDinoff I could have used else chains but it'd made no difference. Mar 19, 2014 at 19:13
  • 1
    I know it is trivial, but wouldn't adding the else chains execute faster? in 3 out of 4 cases? I always make a habit of writing code to execute as fast as possible even if it is only a few cycles. Mar 20, 2014 at 17:34
  • 2
    @BrandonBearden here they wont make any difference (assuming the input is always in the range 0..3). The compiler will probably micro-optimize the code anyway. If we have long series of else if statements we can speed up the code with switch or look-up tables. Mar 20, 2014 at 19:09
  • How is that so? If one==0 it will run the code, then it will have to check if one==1 then if one==2 and finally if one==3 - If there were else if's in there, it wouldn't do the last three checks because it would exit the statement after the first match. And yes, you could further optimize by using a switch statement in place of the if (one... statements and then further using another switch inside the case of the one's. However, that is not my question. Mar 20, 2014 at 22:42
6

Try it with switch casing...

Take a look here or here for more info about it

switch (expression)
{ 
  case constant:
        statements;
        break;
  [ case constant-2:
        statements;
        break;  ] ...
  [ default:
        statements;
        break;  ] ...
}

You can add multiple conditions(not simultaneously) to it and even have a default option where no other cases have been satisfied.

PS: Only if one condition is to be satisfied..

If 2 conditions arise simultaneously.. I don't think switch can be used. But you can reduce your code here.

Java switch statement multiple cases

6

The first thing that occurred to me was essentially the same answer given by Francisco Presencia, but optimized somewhat:

public int fightMath(int one, int two)
{
    switch (one*10 + two)
    {
    case  0:
    case  1:
    case 10:
    case 11:
        return 0;
    case  2:
    case 13:
    case 21:
    case 30:
        return 1;
    case  3:
    case 12:
    case 20:
    case 31:
        return 2;
    case 22:
    case 23:
    case 32:
    case 33:
        return 3;
    }
}

You could further optimize it by making the last case (for 3) the default case:

    //case 22:
    //case 23:
    //case 32:
    //case 33:
    default:
        return 3;

The advantage of this method is that it is easier to see which values for one and two correspond to which return values than some of the other suggested methods.

1
  • This is a variation of another answer of mine here. Mar 21, 2014 at 22:39
6
((two&2)*(1+((one^two)&1))+(one&2)*(2-((one^two)&1)))/2
5
  • 4
    How long did it take you to arrive at this? Mar 25, 2014 at 11:43
  • 2
    @mbatchkarov About 10 minutes of reading the other answers, then 10 minutes of scribbling away with pencil and paper. Mar 25, 2014 at 22:20
  • 7
    I would be really sad if I had to maintain this.
    – Meryovi
    Mar 25, 2014 at 23:19
  • uhmmm... There is a bug: you are missing ; --unicorns
    – Alberto
    Mar 26, 2014 at 12:23
  • I agree with @Meryovi, props for being concise, yet awful like APL code
    – Ed Griebel
    Mar 26, 2014 at 14:40
4

You may use a switch case instead of mutiple if

Also to mention that since you have two variables then you have to merge the two variables to use them in switch

Check this Java switch statement to handle two variables?

0
3

As I draw a table between one/two and the result, I see one pattern,

if(one<2 && two <2) result=0; return;

The above would cut down atleast 3 if statements. I don't see a set pattern nor I am able to glean much from the code given - but if such logic can be derived, it would cut down a number of if statements.

Hope this helps.

3

A good point would be to define the rules as text, you can easier derive the correct formula then. This is extracted from laalto's nice array representation:

{ 0, 0, 1, 2 },
{ 0, 0, 2, 1 },
{ 2, 1, 3, 3 },
{ 1, 2, 3, 3 }

And here we go with some general comments, but you should describe them in rule terms:

if(one<2) // left half
{
    if(two<2) // upper left half
    {
        result = 0; //neither hits
    }
    else // lower left half
    {
        result = 1+(one+two)%2; //p2 hits if sum is even
    }
}
else // right half
{
    if(two<2) // upper right half
    {
        result = 1+(one+two+1)%2; //p1 hits if sum is even
    }
    else // lower right half
    {
        return 3; //both hit
    }
}

You could of course crunch this down to less code, but it is generally a good idea to understand what you code rather than finding a compact solution.

if((one<2)&&(two<2)) result = 0; //top left
else if((one>1)&&(two>1)) result = 3; //bottom right
else result = 1+(one+two+((one>1)?1:0))%2; //no idea what that means

Some explanation on the complicated p1/p2 hits would be great, looks interesting!

3

The shortest and still readable solution:

static public int fightMath(int one, int two)
{
    if (one < 2 && two < 2) return 0;
    if (one > 1 && two > 1) return 3;
    int n = (one + two) % 2;
    return one < two ? 1 + n : 2 - n;
}

or even shorter:

static public int fightMath(int one, int two)
{
    if (one / 2 == two / 2) return (one / 2) * 3;
    return 1 + (one + two + one / 2) % 2;
}

Doesn't contain any "magic" numbers ;) Hope it helps.

2
  • 2
    Formulas like these will make it impossible to change (update) the outcome of a combination at a later date. The only way would be to rework the entire formula.
    – SNag
    Mar 27, 2014 at 8:51
  • 2
    @SNag: I agree with that. The most flexible solution is using the 2D array. But the autor of this post wanted a formula and this is the best one you can get with only simple ifs and math.
    – P.W.
    Mar 27, 2014 at 10:07
1

I personally like to cascade ternary operators:

int result = condition1
    ? result1
    : condition2
    ? result2
    : condition3
    ? result3
    : resultElse;

But in your case, you can use:

final int[] result = new int[/*16*/] {
    0, 0, 1, 2,
    0, 0, 2, 1,
    2, 1, 3, 3,
    1, 2, 3, 3
};

public int fightMath(int one, int two) {
    return result[one*4 + two];
}

Or, you can notice a pattern in bits:

one   two   result

section 1: higher bits are equals =>
both result bits are equals to that higher bits

00    00    00
00    01    00
01    00    00
01    01    00
10    10    11
10    11    11
11    10    11
11    11    11

section 2: higher bits are different =>
lower result bit is inverse of lower bit of 'two'
higher result bit is lower bit of 'two'

00    10    01
00    11    10
01    10    10
01    11    01
10    00    10
10    01    01
11    00    01
11    01    10

So you can use magic:

int fightMath(int one, int two) {
    int b1 = one & 2, b2 = two & 2;
    if (b1 == b2)
        return b1 | (b1 >> 1);

    b1 = two & 1;

    return (b1 << 1) | (~b1);
}
1

Here's a fairly concise version, similar to JAB's response. This utilises a map to store which moves triumph over others.

public enum Result {
  P1Win, P2Win, BothWin, NeitherWin;
}

public enum Move {
  BLOCK_HIGH, BLOCK_LOW, ATTACK_HIGH, ATTACK_LOW;

  static final Map<Move, List<Move>> beats = new EnumMap<Move, List<Move>>(
      Move.class);

  static {
    beats.put(BLOCK_HIGH, new ArrayList<Move>());
    beats.put(BLOCK_LOW, new ArrayList<Move>());
    beats.put(ATTACK_HIGH, Arrays.asList(ATTACK_LOW, BLOCK_LOW));
    beats.put(ATTACK_LOW, Arrays.asList(ATTACK_HIGH, BLOCK_HIGH));
  }

  public static Result compare(Move p1Move, Move p2Move) {
    boolean p1Wins = beats.get(p1Move).contains(p2Move);
    boolean p2Wins = beats.get(p2Move).contains(p1Move);

    if (p1Wins) {
      return (p2Wins) ? Result.BothWin : Result.P1Win;
    }
    if (p2Wins) {
      return (p1Wins) ? Result.BothWin : Result.P2Win;
    }

    return Result.NeitherWin;
  }
} 

Example:

System.out.println(Move.compare(Move.ATTACK_HIGH, Move.BLOCK_LOW));

Prints:

P1Win
2
  • I would recommend static final Map<Move, List<Move>> beats = new java.util.EnumMap<>(); instead, it should be slightly more efficient.
    – JAB
    Mar 24, 2014 at 16:45
  • @JAB Yes, good idea. I always forget that type exists. And... how awkward it is to construct one! Mar 24, 2014 at 16:47
1

I'd use a Map, either a HashMap or a TreeMap

Especially if the parameters are not on the form 0 <= X < N

Like a set of random positive integers ..

Code

public class MyMap
{
    private TreeMap<String,Integer> map;

    public MyMap ()
    {
        map = new TreeMap<String,Integer> ();
    }

    public void put (int key1, int key2, Integer value)
    {
        String key = (key1+":"+key2);

        map.put(key, new Integer(value));
    }

    public Integer get (int key1, int key2)
    {
        String key = (key1+":"+key2);

        return map.get(key);
    }
}
1

static int val(int i, int u){ int q = (i & 1) ^ (u & 1); return ((i >> 1) << (1 ^ q))|((u >> 1) << q); }

1

Thanks to @Joe Harper as I ended up using a variation of his answer. To slim it down further as 2 results per 4 were the same I slimmed it down further.

I may come back to this at some point, but if there's no major resistance caused by multiple if-statements then I'll keep this for now. I will look into the table matrix and switch statement solutions further.

public int fightMath(int one, int two) {
  if (one === 0) {
    if (two === 2) { return 1; }
    else if(two === 3) { return 2; }
    else { return 0; }
  } else if (one === 1) {
    if (two === 2) { return 2; }
    else if (two === 3) { return 1; }
    else { return 0; }
  } else if (one === 2) {
    if (two === 0) { return 2; }
    else if (two === 1) { return 1; }
    else { return 3; }
  } else if (one === 3) {
    if (two === 0) { return 1; }
    else if (two === 1) { return 2; }
    else { return 3; }
  }
}
5
  • 13
    This is actually less readable than the original and does not reduce the number of if statements...
    – Chad
    Mar 19, 2014 at 14:40
  • @Chad The idea of this was to increase process speed and although it looks horrendous it's easily updated should I add more actions in the future. Saying this i'm now using a previous answer which I didn't fully understand before.
    – TomFirth
    Mar 19, 2014 at 15:15
  • 3
    @TomFirth84 Is there a reason that you are not following proper coding conventions for your if statements?
    – ylun
    Mar 19, 2014 at 16:50
  • @ylun: I had reduced the lines before pasting it on SO, not for readability but for sheer spam space. There are variations of practice on this page and sadly it's just the way i've learnt and am comfortable with.
    – TomFirth
    Mar 21, 2014 at 9:08
  • 2
    @TomFirth84 I don't think this is easily updated, the number of lines grows something like the product of the number of permitted values. Mar 25, 2014 at 22:56
0
  1. Use constants or enums to make the code more readable
  2. Try to split the code into more functions
  3. Try to use the symmetry of the problem

Here is a suggestion how this could look like, but using an ints here is still kind of ugly:

static final int BLOCK_HIGH = 0;
static final int BLOCK_LOW = 1;
static final int ATTACK_HIGH = 2;
static final int ATTACK_LOW = 3;

public static int fightMath(int one, int two) {
    boolean player1Wins = handleAttack(one, two);
    boolean player2Wins = handleAttack(two, one);
    return encodeResult(player1Wins, player2Wins); 
}



private static boolean handleAttack(int one, int two) {
     return one == ATTACK_HIGH && two != BLOCK_HIGH
        || one == ATTACK_LOW && two != BLOCK_LOW
        || one == BLOCK_HIGH && two == ATTACK_HIGH
        || one == BLOCK_LOW && two == ATTACK_LOW;

}

private static int encodeResult(boolean player1Wins, boolean player2Wins) {
    return (player1Wins ? 1 : 0) + (player2Wins ? 2 : 0);
}

It would be nicer to use a structured type for the input and the output. The input actually has two fields: the position and the type (block or attack). The output also has two fields: player1Wins and player2Wins. Encoding this into a single integer makes it harder to read the code.

class PlayerMove {
    PlayerMovePosition pos;
    PlayerMoveType type;
}

enum PlayerMovePosition {
    HIGH,LOW
}

enum PlayerMoveType {
    BLOCK,ATTACK
}

class AttackResult {
    boolean player1Wins;
    boolean player2Wins;

    public AttackResult(boolean player1Wins, boolean player2Wins) {
        this.player1Wins = player1Wins;
        this.player2Wins = player2Wins;
    }
}

AttackResult fightMath(PlayerMove a, PlayerMove b) {
    return new AttackResult(isWinningMove(a, b), isWinningMove(b, a));
}

boolean isWinningMove(PlayerMove a, PlayerMove b) {
    return a.type == PlayerMoveType.ATTACK && !successfulBlock(b, a)
            || successfulBlock(a, b);
}

boolean successfulBlock(PlayerMove a, PlayerMove b) {
    return a.type == PlayerMoveType.BLOCK 
            && b.type == PlayerMoveType.ATTACK 
            && a.pos == b.pos;
}

Unfortunately, Java is not very good at expressing those kinds of data-types.

-2

Instead do something like this

   public int fightMath(int one, int two) {
    return Calculate(one,two)

    }


    private int Calculate(int one,int two){

    if (one==0){
        if(two==0){
     //return value}
    }else if (one==1){
   // return value as per condtiion
    }

    }
2
  • 4
    You just created a private function that is wrapped by the public one. Why not just implement this in the public function? Mar 19, 2014 at 14:10
  • 5
    And you have not reduced the number of if statements.
    – Chad
    Mar 19, 2014 at 14:43

Not the answer you're looking for? Browse other questions tagged or ask your own question.