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
.