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

[BurntPizza]

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:

System.out.println(c.getName() + " is dead.");
System.out.println(p.getName() + " wins.");

Could be put into, say [icode]printStatus();[/icode] or something.

Another thing to do, in this code here:

//Player wins:
if (playerWins) {
    p.setExperience(p.getExperience() + (c.getLevel() * 6));
    p.statistics.levelUp();
    System.out.println(p.getName() + " wins.");
}
//Creature wins:
else {
    System.out.println(c.getName() + " wins.");
}

You could get rid of the serveral [icode]playerWins[/icode] if/else blocks entirely by simply:

System.out.println((playerWins ? p : c).getName() + " wins.");

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.

[Several_Kilo-Bytes]

@BurntPizza

@opiop65

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.

public class Point
{
/**
 * The sum of any two numbers multiplied by amazing_constant is the average of those two numbers.
 */
  public final static int amazing_constant;

  public final double x, double y; // Must be assigned in constructor

  public Point(double a, double b)
  {
     // x and y do not need to be set yet because they aren't used here
     System.out.println("The average of a and b is " + (a + b) * amazing_constant);
     // but must be assigned somewhere
     x = a;
     y = b;
  }

  public double distanceTo(Point p)
  {
    final double dx, dy; // Local variables can be final
    final double r; // If a local variable is never used, it will still compile
    dx = p.x -x;
    dy = p.y - y;
    return Math.sqrt(dx * dx + dy * dy);
  }

  public static Point getCentroid(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 used
    final double multiplier;
     // Local variables, like all final variables must be assigned a value before they are used.
    double sumX = 0, sumY = 0;
    for(Point p : points)
    {
       sumX += p.x;
    }
    if(points.size() * 3 - 1 == 5)
    {
      multiplier = amazing_constant;
    }
    else
    {
      multiplier = 1.0 / points.size();
    }
    return new Point(sumX * multiplier, sumY * multiplier);
  }

  static
  {
    int z = 0;
    for(int i = 1; i <= 256; i *= 2) z ++;
    amazing_constant = z / 4.0; // Yes a final variable can be assigned this way.
  }
}

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.

[anon951759]

@BurntPizza

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:

System.out.println(c.getName() + " is dead.");
System.out.println(p.getName() + " wins.");

Could be put into, say [icode]printStatus();[/icode] or something.

Another thing to do, in this code here:

//Player wins:
if (playerWins) {
    p.setExperience(p.getExperience() + (c.getLevel() * 6));
    p.statistics.levelUp();
    System.out.println(p.getName() + " wins.");
}
//Creature wins:
else {
    System.out.println(c.getName() + " wins.");
}

You could get rid of the serveral [icode]playerWins[/icode] if/else blocks entirely by simply:

System.out.println((playerWins ? p : c).getName() + " wins.");

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.

[BurntPizza]

The engine is usually more important than the paint!

I like to think of the ternary operator like this: The expression [icode](condition ? object_or_value1 : object_or_value2)[/icode] is equivalent to calling this method:

Object ternary(boolean condition, Object object1, Object object2) {
    if(condition) {
        return object1;
    } else return object2;
}

Of course, it returns a primitive value if passed that, but can also return object references, as in my example, so pretend that the method is overloaded for all primitive types as well.

Anybody feel free to correct my thinking if it is wrong/misleading, or if there are other subtleties to point out.

[namrog84]

While its not neccesarily saving ‘lines’ it does make some things modular
e.g.
you have these 2 lines in at least 4 cases

p.setHealth(0.0);
p.setIsAlive(false);
c.setHealth(0.0);
c.setIsAlive(false);

The only difference is p and c. You can stick it in a method and call either or

resetEntity(p); // or alternative resetEntity(c);
...
void resetEntity(Entity e){
    e.setIsAlive(false);
    e.setHealth(0,0);
}

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?)

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?

??? ??? :point: :persecutioncomplex: :

Again, some of these are just coding style more than anything?

Edit:

Just saw a major code/logic reducer, improvement from

if (c.getUsesArmor()) {
    if (!pIsDefending)
        cHealth -= p.getBaseDamage() + p.weapon.getWeaponDamage(p.getMyWeapon()) - c.getArmorPoints();
    else
        cHealth -= (p.getBaseDamage() + p.weapon.getWeaponDamage(p.getMyWeapon()) - c.getArmorPoints()) / 2;
} else {
    if (!pIsDefending) 
        cHealth -= p.getBaseDamage() + p.weapon.getWeaponDamage(p.getMyWeapon());
    else 
        cHealth -= (p.getBaseDamage() + p.weapon.getWeaponDamage(p.getMyWeapon())) / 2;
}

to become something like

int defending = (pIsDefending ? 2: 1);  //  defending = 2 half damage, !defending = 1 full damage
int armor = (c.getUsesArmor() ? c.getArmorPoints() : 0);
cHealth -= (p.getBaseDamage() + p.weapon.getWeaponDamage(p.getMyWeapon()) - armor) / defending;

thats 11 lines to 3 lines. and this happens in at least 2 places

[65K]

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

[anon951759]

@65K

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.

Jury class: https://github.com/Valkryst/Project_02/blob/master/src/main/java/valkryst/systems/battle/Jury.java

Most of the tactic related code is within the Tactics class but I’m not sure if I want to move the rest of the tactic related code from the Battle class.

Thanks!