Simple battle class

Question

I'm new to Java, and this code could likely be squashed down a whole heap. How can I make this code simpler and easier to read?

chance.base is a percentage. If it is = 40, it has a 40% chance of returning true.

public class battleManager {
void battle(Army playerArmy,Army enemyArmy,int armyModifier,int army2Modifier){
 if(armyModifier > 40| army2Modifier > 40){
 System.out.println("note: modifiers were over maximum (40)");
 }
 chanceManager chance = new chanceManager();
 chance.base = (50 + armyModifier) - army2Modifier;
 int armyCount = playerArmy.units, army2Count = enemyArmy.units;
 boolean armyAlive = true, army2Alive = true, playerWon = false;
 while(armyAlive & army2Alive){
 if(chance.generate()){
 army2Count--;
 }
 else{
 armyCount--;
 }
 if(armyCount == 0){
 armyAlive = false;
 playerWon = false;
 }
 if(army2Count == 0){
 army2Alive = false;
 playerWon = true;
 }
 }
 chance.base = 25;
 int armyKilled, army2Killed;
 armyKilled = playerArmy.units - armyCount;
 army2Killed = enemyArmy.units - army2Count;
 for (int unit = 0; unit < armyKilled; unit++){
 if(chance.generate()){
 armyCount++;
 }
 }
 for (int unit = 0; unit < army2Killed; unit++){
 if(chance.generate()){
 army2Count++;
 }
 }
 // this is for debugging
 System.out.println("army survivors:");
 System.out.println(armyCount);
 System.out.println("army2 survivors:");
 System.out.println(army2Count);
 if(playerWon){
 System.out.println("player army won");
 }else
 System.out.println("Player army lost");
 // end of debugging
 playerArmy.units = armyCount;
}

• njzk2: 40, 50 and 25 are en.wikipedia.org/wiki/Magic_number_%28programming%29 . Avoid them

Answer 1

Naming

Class names should start with an UpperCase letter by convention.

What is a battleManager? What is a choiceManager? Why not ColonelSanders or VicePresidentOfChoice? Joking aside, the naming could be more descriptive. How about BattleSimulator and BernoulliTrial?

The naming of the parameters is inconsistent:

battle(Army playerArmy,Army enemyArmy,int armyModifier,int army2Modifier)

Any of these would be more logical:

battle(Army playerArmy, Army enemyArmy, int playerModifier, int enemyModifier)
battle(Army army1, Army army2, int army1Modifier, int army2Modifier)
battle(Army attacker, Army defender, int attackerModifier, int defenderModifier)

The last option would be particularly appealing if the situation being modelled is asymmetrical (i.e., the attacker army is attempting to move into the defender's territory). Typically, the rules would require the attacker to have at least n units, while the defender might have no units at all. (An assertion at the beginning of the battle method to enforce such a rule would be a good idea too.)

Object-oriented design

The battle method should be public, to be consistent with the class.

It would be useful for the function to return the winner of the battle.

Setting chance.base to modify the probability is not recommended, since you are allowing others to meddle with the internals of the chance object. Three better approaches are:

• BernoulliTrial battleProb = new BernoulliTrial(50 + army1Modifier - army2Modifier);
• battleProb.setProbability(50 + army1Modifier - army2Modifier);
• battleProb.generate(50 + army1Modifier - army2Modifier);

See below for a more creative naming suggestion — I think it reads more nicely.

Algorithm

There is a fighting phase and a healing phase.

After healing, you resuscitate some of playerArmy's casualties. However, you do not do likewise for the enemy army. Either that is an oversight, or one of the healing loops is superfluous. I'll assume that only the winning army should be healed.

Rather than setting the unit count after the loop, you could manipulate the army's unit count directly within the loop.

You would be better off without the armyAlive and army2Alive flags.

assert(attacker.units > 0);
BernoulliTrial attack = new BernoulliTrial(50 + attackerModifier - defenderModifier);
int attackerCasualties = 0, defenderCasualties = 0;
// Begin fighting
while (attacker.units > 0 && defender.units > 0) {
 if (attack.success()) {
 defender.units--;
 defenderCasualties++;
 } else {
 attacker.units--;
 attackerCasualties++;
 }
}
Army winner = (attacker.units > 0) ? attacker : defender;
// Heal the winning army
BernoulliTrial heal = new BernoulliTrial(25);
int winnerCasualties = (winner == attacker) ? attackerCasualties : defenderCasualties;
while (winnerCasualties-- > 0) { 
 if (heal.success()) {
 winner.units++;
 }
}
return winner;

• Rob Audenaerde: The ternary inside the for loop is not very readable. :p
• njzk2: the int units variable is not used and its name is confusing because of winner.units. Plus I don't see the point of decrementing it. I would do int healingCandidates = (ternary); while (healingCandidates-- > 0) {...}. I also don't like that there are 2 ternaries.
• 200_success: @RobAu Rewrote the loop by popular demand (Rev 5).

Answer 2

• Naming is inconsistent. Either armies should be army1, army2 or related variables should be playerArmyAlive, enemyArmyAlive etc.

• Symmetry is broken. It is reasonable to expect enemyArmy.units = army2Count at the end of the method (otherwise why bother calculating army2Count?).

  More on symmetry. If both armies are to be resurrected indeed, factor that out in a method

  void resurrect(Army army, int killed) {
   while (killed-- > 0) {
   if (chance.generate()) {
   army.units++;
   }
   }
  }
  

• Instantiating chance for each battle doesn't look right. It feels like a member of battleManager. Same goes for magic constants 40 and 25.

• armyAlive is redundant; it is synonymous to armyCount == 0 predicate, which really feels like an Army method.

Answer 3

I see a lot that we could help you with.

Operaters.

You use :

while(armyAlive & army2Alive)

You could rewrite it as :

while(armyAlive && army2Alive).

Short explanation : if first is false, it won't check statement 2. Your case both operands are always checked.
Long explication here.

Use correct methods

Name may be strange, but what I want to say is, you are in the manager.
You want to do the battle between 2 Army's.
Why not returning the Army what won?
As you logical think about it, the battle just need to happen and you need to know in the guy who has won.

Setting the right things at the right place.

If it was me I would implement the method isAlive in the Army class like this :

public boolean isAlive () {
 return units > 0;
}
public void loseAUnit() {
 units--;
}

Then changing the code to :

int playerStartUnits = playerArmy.units, enemyStartUnits = enemyArmy.units;
while(playerArmy.isAlive() && enemyArmy.isAlive()){
 if(chance.generate()){
 enemyArmy.loseAUnit();
 }
 else{
 playerArmy.loseAUnit();
 }
}
Army winner = playerArmy.isAlive()?playerArmy:enemyArmy;
resurrect(playerArmy,playerStartUnits,chance);
resurrect(enemyArmy,playerStartUnits,chance);
return winner;

For the regenerating, @vnp already mentioned how you could do it, just he didn't mentioned that you would have to send the chancegenerator also to the method.

I just find it strange that you only set the player his units back and not the enemy his troops after the resurrection.
What I mean with that is that you resurrect the enemy troops also, but never set them back.
Or you delete the resurrection of enemy troops or you do set them back.

Answer 4

A few that I could grasp:

• Usage of bitwise operators (| and &) instead of logical operators (|| and &&). Though its correct but ensure you do not use it wrongly.
• boolean armyAlive, army2Alive can be avoided. Instead use the count.

• Combine the System.out.println-s into one line (if related).

  System.out.println("army survivors: " + armyCount);
  

• Use DEBUG flag to contain the logs so that its easy to switch between Debugging and otherwise.

• Updating arguments is not good. Assume them to be final. Instead return what needs to be updated. (Opinions for/against the same are invited).

  playerArmy.units = armyCount;
  

• playerWon can be calculated outside the while loop as follows:

  while(armyAlive & army2Alive){
   if(chance.generate()){
   army2Count--;
   }
   else{
   armyCount--;
   }
  }
  playerWon = (army2Count == 0);`
  

• If suppose the playerArmy or enemyArmy is null, have a validation check.

• A grievous mistake: if armyCount and army2Count is <=0 , it will be a while before you know it.
• Do you want to know which modifier is >40. If yes, add it to the logs.

Answer 5

void battle(Army playerArmy, Army enemyArmy, int armyModifier, int army2Modifier){

Try to avoid parallel data structures. They make the system more fragile, as you have to maintain the relation. If you need a parallel structure, try to keep the name consistent. Here you seem to be calling it both enemyArmy and army2.

Why not make a modifier field in Army? Then you'd be guaranteed to associate the right modifier with the right Army.

Consider renaming modifier to something like strength or battlePower that is clearer about what it does. You could be modifying almost anything. It's hardly better than number.

Also, any time you find yourself naming variables something like one and two, there's a good chance that you should be using a Collection. Possibly a List in this case.

if(armyModifier > 40| army2Modifier > 40){
while(armyAlive & army2Alive){

You are using | and &. You should be using the logical-only operators (|| and &&). The latter versions short-circuit and don't process the second operand if they can determine the expression value from the first operand. You should almost always use them. It won't really matter here, as the second expressions are pretty simple. But there are some cases where using the short-circuit operators can improve performance.

if ( armyModifier > 40 || army2Modifier > 40 ) {
while ( armyAlive && army2Alive ) {

Even here, there may be some slight performance improvement.

 if(armyCount == 0){

This kind of check is risky. What happens if you change your battles so that there is a \1ドル\%\$ chance that you do double damage? Then it would be possible for your armyCount to go directly from \1ドル\$ to \$-1\$. Your army might win with negative troops, since once it goes negative it can't get to \0ドル\$. If this happens to both armies, you may continue until you go out of the integer bounds.

 if ( armyCount <= 0 ) {

This is safer and more robust in the face of code changes. It will catch circumstances that a straight equality check would miss.

// this is for debugging
System.out.println("army survivors:");
System.out.println(armyCount);
System.out.println("army2 survivors:");
System.out.println(army2Count);
if(playerWon){
 System.out.println("player army won");
}else
 System.out.println("Player army lost");
// end of debugging

You shouldn't have this kind of debugging code if you are sending your code out for review. By the time that you get a review, you should have removed this. Consider writing unit tests to handle this kind of debugging. Then it isn't cluttering your code when you run it for real.

I'd consider moving some of your logic to your Army class. For example, you could replace the armyAlive variable with a method call: playerArmy.isActive(). You could replace your manual decrement of armyCount with playerArmy.takeCasualty() or playerArmy.takeCasualties(1). Later you could have a playerArmy.healCasualties() method.

Doing this not only simplifies battle, it reduces repeated code. Assuming you pass the armies as a List named armies, you could say:

for ( Army army : armies ) {
 army.healCasualties(chance);
}

Now you no longer care how many armies are fighting. You don't have to add a new loop for each one. You just need to call the appropriate method for each, and the loop takes care of the "for each" part. Now if you change the heal logic, you can change it for both armies without having to modify two pieces of code in parallel.

You might not even pass armies to your battle function. If you had a constructor for battleManager, you could pass things like the armies and heal percentage then. Perhaps battle takes no parameters.

Alternately, if you are going to keep things the way that they are, you should make battle a static method. That makes more sense for a method that does not depend on any class fields.

public static void battle(List<Army> armies) {

This assumes that you also moved the modifiers into Army.

• 200_success: The modifiers might not be properties of the respective armies. Maybe they are functions of how much money is in the bank, or citizen morale, or technological advancement.