Trying to get rid of a few if-else statements if possible.

Newbie & Debugging Questions
#1

Hey,

I’ve made a lot of progress on my little ‘battle-simulation’ program and I’ve gotten to a point where the class that controls the battle is getting a bit long. I’m pretty sure that, as I continue to add things to the simulation, the battle class will become even longer and tougher to figure out.

I’m already starting to go over all the code and comment it but I’m not sure if I can cut out a few if or else statements to make the code a bit shorter and still run the same.

If anyone sees anywhere that I can get rid of or change a few statements then thanks!

https://github.com/Valkryst/Project_02/blob/master/src/main/java/valkryst/systems/Battle.java
(I’m working on the class, mainly adding comments, as of posting this so you might notice a few comments pop up or a few statements disappear or change as I find things to comment on/change.)

#2

First you could “lift” code that is in both branches of an if/else, such as here:

//Player wins:
if (Math.max(cHealth, pHealth) == pHealth) {
    finished = true;
    p.setHealth(0.0);
    p.setIsAlive(false);
    c.setHealth(0.0);
    c.setIsAlive(false);
    p.setExperience(p.getExperience() + (c.getLevel() * 6));
    p.statistics.levelUp();
    System.out.println("You are both dead.");
    System.out.println(p.getName() + " wins.");
    break;
} //Creature wins:
else {
    finished = true;
    p.setHealth(0.0);
    p.setIsAlive(false);
    c.setHealth(0.0);
    c.setIsAlive(false);
    System.out.println("You are both dead.");
    System.out.println(c.getName() + " wins.");
}

Could be reduced to:

finished = true;
p.setHealth(0.0);
p.setIsAlive(false);
c.setHealth(0.0);
c.setIsAlive(false);
System.out.println("You are both dead.");

//Player wins:
if (Math.max(cHealth, pHealth) == pHealth) {
    p.setExperience(p.getExperience() + (c.getLevel() * 6));
    p.statistics.levelUp();
    System.out.println(p.getName() + " wins.");
    break;
} //Creature wins:
else {
    System.out.println(c.getName() + " wins.");
}

That’s the first tip I can give from reading that, will help a bit with length and readability, I think.

#3

No, that wouldn’t help at all. If you look at the code you’ll see that the function calls were specific to the if statements, and cannot be moved into a more global state. Sorry, but it seems as if you didn’t even look at his code…

As for the question, there really isn’t much you can do, it actually is quite simple. If you start to use a bunch of if statements inside of each other, then you know you’re most likely doing it wrong. As of right now, I don’t have anything that would make either statement smaller.

#4

Thanks for the replies.

Yep, opiop65 is right, that would break the program. Those statements are unique for each type of scenario so they can’t be taken out of their respective if/else statements without breaking that scenario as far as I can see.

#5 
if (cHealth <= 0.0 || pHealth <= 0.0) {

if(pIsFirst) {

if(!pIsFirst) {

These are your 3 main conditions. I think the code within each of these should be separate methods. It’s not necessarily less code. But, it’s definitely easier to read, and easier to find specific code.

#6

All of that particular code was in a higher-up if-block so it would not affect program flow.

I “lifted” only statements that were in fact in both scenarios, ones that would of been executed either way. So long as the exact order of execution does not matter (as in my example) or is not affected, the program logic is unchanged.

Compile and see!

EDIT:
Note that if the else block is actually another if, like this:

if {
    <snip>
}
@@else if {
    <snip>
}

Then the lift operation may not be correct, as it is possible that no block would be executed! Only ever lift code if it is common to all blocks, including an ending else block.

#7

No offense, but that’s a horrible coding practice. Just because it doesn’t affect other code now doesn’t mean it won’t later on. Just keep the code in the correct if statements and you’ll never have a problem.

#8

To be technical, he’s not wrong. Both conditions have some identical code. That can be moved outside the conditions with no bad effect. It’s not even really bad coding practice. Code duplication is bad coding practice.

finished = true;

This (for example), which is in each of the conditions within the first main condition (if (cHealth <= 0.0 || pHealth <= 0.0) {)
Can be moved outside all 4 of them, and just have a single statement at the top. Nothing about that will break the code.

#9

Use boolean variables.


final boolean playerLeads = pHealth > cHealth;
final boolean playerDead = pHealth < 0;
final boolean creatureDead = cHealth < 0;
final boolean playerWins = playerLeads && creatureDead;

if(playerDead)
{
  p.setHealth(0);
  p.isAlive(false);
}

if(creatureDead)
{
  c.setHealth(0);
  c.isAlive(false);
}

if(playerWins)
{
  et.cetera();
}

I’m guessing this is a problem (line 37):

cHealth = Math.abs(cHealth);
pHealth = Math.abs(cHealth);

Right now it is difficult to follow the code, so I may be missing something. Why have a setIsAlive(boolean)? Couldn’t you achieve the same thing be making setHealth automatically make the character dead and make sure the health is at least zero? Maybe you could have have a kill() and revive(health) method, too.

#10

Ok, i just noticed that, sorry! This is very true, you can always set these variables at the bottom under the if and else statements.

#11

O.o I have no idea how I missed that this entire time, guess I was partially wrong. =P

I’ve moved the statement ‘finished = true;’ as high as it can go within the if-else blocks and it looks like it should work after I went through and carefully checked out which code is part of which if-else block. I can’t test it at the moment because I’m working on another area of the program but it should work.

I’ll take a look, after typing this, for a few more things to move around.

@Several Kilo-Bytes
You posted while I was typing this up so I’ll take a better look at your comment in a minute but to answer your question the setIsAlive() method sets the isAlive() boolean in the Physiological class. I use this to figure out whether the battle is over or not and, down the road, I’ll be using it to decide a few other things as well.

Edit:
I’ve committed the changed code to github, still looking for more things to alter.

#12

Sorry for the double post, I’m not even sure if there is a rule about that on this forum. Anyway…

Setting the booleans that way works, when the battle starts, but does it keep updating itself as the code runs without me having to write statements to set the boolean? I’m having a blond moment here about that for some reason.

#13

*blonde xD

And no, as the modifier is final, it will compile the variables once and they will never change.

#14

It is a final variable so it is only assigned once inside the variable’s scope. If it is at the beginning of the method, it gets set once per method. If it is at the top of a loop, it gets set once per loop iteration.

Concerning setIsAlive(boolean): You would keep isAlive(), but setIsAlive(boolean) is redundant. Unless there exists an exception to the rule, it is implied that a character with 0 HP is not alive, so isAlive() could either return hp > 0; or setHealth could set a private variable isAlive to false automatically.

#15

But since its outside the scope of any loops or if statements won’t it never be reset to something else?

#16

It would go inside a loop.

while(true)
{
  final boolean playerLeads = pHealth > cHealth;
  final boolean playerDead = pHealth < 0;
  final boolean creatureDead = cHealth < 0;
  final boolean playerWins = playerLeads && creatureDead;

// .........

  if(playerWins)
  {
// .........
    break;
  }
  if(playerDead) // playerLoses
  {
// .........
    break;
  }
// No one has won
// ...
}

You could make it non-final and move it out of the loop if you don’t want to use break.

#17

Ah this is true. I never knew you could change the value of a variable with the final modifier after it had been compiled at runtime. Well, I learn more everyday!

#18

You can’t really, each time the loop is executed, playerLeads, etc are actually considered brand new variables, and can thus be declared final, as to the code, it is their “first” declaration.

#19

Ah, that makes a lot more sense now and it saves a few calculations. ^.^

I’ve modified and uploaded the new Battle class and now it’s back to looking for more, would optimizations be the word? Thanks!

#20

Usually I love to get rid if-else tree but I’m busy right now so I can’t take a look.

But here some advice. For advance problem, for example when you work as developer and have to continue previous work that consists 7000 lines of if-else, search for Karnaugh Map for boolean logical optimization.