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!

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.

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.

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.

Those statements are unique for each type of scenario so they can't be taken out of their respective if/else statements

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:

1 2 3 4 5 6

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.

Those statements are unique for each type of scenario so they can't be taken out of their respective if/else statements

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!

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.

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.

1

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.

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.

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.

1

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.

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.

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-BytesYou 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.

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.

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.

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.

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.

Another thing, this time more for readability than reducing logic: Any time you have a bunch of statements that are used identically in many places, you can "un-inline" them, by putting them in a private method (with a good, descriptive name!), and replace all usages of those statement bundles with that method. Example, these two statements are used together a lot:

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.

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.

Oh, final variables are very cool. If you are programming in Java it is very helpful to understand how variable assignment works. Final variables can be static, local, or field variables. They can only be assigned to once and must be assigned to before they are read, but only need an assignment if they can be read.

publicclassPoint{/** * The sum of any two numbers multiplied by amazing_constant is the average of those two numbers. */publicfinalstaticintamazing_constant;

publicfinaldoublex, doubley; // Must be assigned in constructor

publicPoint(doublea, doubleb) {// x and y do not need to be set yet because they aren't used hereSystem.out.println("The average of a and b is " + (a + b) * amazing_constant);// but must be assigned somewherex = a;y = b; }

publicdoubledistanceTo(Pointp) {finaldoubledx, dy; // Local variables can be finalfinaldoubler; // If a local variable is never used, it will still compiledx = p.x -x;dy = p.y - y;returnMath.sqrt(dx * dx + dy * dy); }

publicstaticPointgetCentroid(List<Point> points) {// A final variable can get more than one value, but every execution path must write to it exactly once before it is usedfinaldoublemultiplier;// Local variables, like all final variables must be assigned a value before they are used.doublesumX = 0, sumY = 0;for(Pointp : points) {sumX += p.x; }if(points.size() * 3 - 1 == 5) {multiplier = amazing_constant; }else {multiplier = 1.0 / points.size(); }returnnewPoint(sumX * multiplier, sumY * multiplier); }

Parameters can also be final, but it's not that useful to do so because there is no way to make values referred to read-only and all user defined types are Objects. But an object with only final public variables is useful since you know those values will never change even if you share it with another section of code. Look at the length variable of the String class. It is a final public int and its value depends on what gets passed to String's constructor.

It's important to know final variables are not C style defines. They are much more elegant and can help avoid bugs rather than cause them.

Another thing, this time more for readability than reducing logic: Any time you have a bunch of statements that are used identically in many places, you can "un-inline" them, by putting them in a private method (with a good, descriptive name!), and replace all usages of those statement bundles with that method. Example, these two statements are used together a lot:

Of course, this only would replace the System.out calls in the blocks, although in this specific case it eliminates the if/else entirely.

Again, not actually reducing logic really, but much more concise and readable, in my opinion.Do elsewhere accordingly as you see fit.

Thanks.

Putting those print messages into their own method is a good idea but it wouldn't be worth doing at the moment since they're only there for testing purposes at the moment. Instead of working on a GUI and all that fancy graphic-related stuff I'm just creating the basics of the game and testing everything with print statements. Thanks though. ^.^

The ternary (is that the correct word for it?) operator seems like it would be pretty useful in this case. I barely know how to use it but I'll take a look tomorrow afternoon and see if I can figure it out well enough so that I can start using it in the program.

Its trivial change, but makes the 'main meaty' chunk have 4 less lines' though it does't reduce total file line count at all.

The same goes for lines 94-112 and 128-146(maybe 146?)

1 2 3 4 5 6 7

if(pIsFirst){SomeMethod();}...if(!pIsFirst){SomeMethod();}

would reduce about 15 LoC and reduce some of the 'clutter' in the meaty part of the code

Another thing, is in your if(playerDead || creatureDead)you call finished = true;However, in all conditions you use "break" thus negating the effect of finished = true. If you are using break, then why not just use while(true) ?Thus eliminating 'finished' boolean completely?

Before starting to just shuffle a few statements, you could fix the basic design problem: Battle does too many things.1. It watches the game state and looks out for the winner.2. It implements the game AI.

Two different unrelated tasks should be split up in dedicated classes.Then it is rather uncommon to run a main loop inside the constructor.

Before starting to just shuffle a few statements, you could fix the basic design problem: Battle does too many things.1. It watches the game state and looks out for the winner.2. It implements the game AI.

Two different unrelated tasks should be split up in dedicated classes.Then it is rather uncommon to run a main loop inside the constructor.

Battle | -- Jury -- AI

I've just started moving a few things around and this seems like a pretty good idea. I've moved out all of the code that decides if anyone has won or lost the battle to the new Jury class.After that I started to use the ternary operator to get rid of a few print statements and then I got rid of the whole 'finished' boolean variable and started just using 'break;' instead to exit the loop.

java-gaming.org is not responsible for the content posted by its members, including references to external websites,
and other references that may or may not have a relation with our primarily
gaming and game production oriented community.
inquiries and complaints can be sent via email to the info‑account of the
company managing the website of java‑gaming.org