HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Simple battle class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
battlesimpleclass

Problem

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

Solution

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;

Code Snippets

battle(Army playerArmy,Army enemyArmy,int armyModifier,int army2Modifier)
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)
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;

Context

StackExchange Code Review Q#72210, answer score: 16

Revisions (0)

No revisions yet.