patternjavaModerate
Simple battle class
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
What is a
The naming of the parameters is inconsistent:
Any of these would be more logical:
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
Object-oriented design
The
It would be useful for the function to return the winner of the battle.
Setting
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
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
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.