orderMultipleBlockers and orderBlockingMultipleAttackers - moved into Combat class.

Combat - adjusted visibility of methods, moved some methods closer to caller

src/main/java/forge/game/ai/AiAttackController.java

@@ -460,7 +460,7 @@ public class AiAttackController {
         } else {
             // 1. assault the opponent if you can kill him
             if (bAssault) {
-                return c.getDefendingPlayers().get(0);
+                return getDefendingPlayers(c).get(0);
             }
             // 2. attack planeswalkers
             List<Card> pwDefending = c.getDefendingPlaneswalkers();
@@ -475,6 +475,17 @@ public class AiAttackController {
     final boolean LOG_AI_ATTACKS = false;
 
 
+    public final List<Player> getDefendingPlayers(Combat combat) {
+        final List<Player> defending = new ArrayList<Player>();
+        for (final GameEntity o : combat.getDefenders()) {
+            if (o instanceof Player) {
+                defending.add((Player) o);
+            }
+        }
+        return defending;
+    }
+
+    
     /**
      * <p>
      * Getter for the field <code>attackers</code>.
@@ -806,7 +817,7 @@ public class AiAttackController {
                             }
                         }
                         if (!found) {
-                            defender = combat.getDefendingPlayers().get(0);
+                            defender = getDefendingPlayers(combat).get(0);
                         }
                     }
                 }

src/main/java/forge/game/ai/ComputerUtil.java

@@ -984,7 +984,7 @@ public class ComputerUtil {
             Combat combat = new Combat(ai.getOpponent());
             List<Card> attackers = ai.getOpponent().getCreaturesInPlay();
             for (Card att : attackers) {
-                if (CombatUtil.canAttackNextTurn(att)) {
+                if (CombatUtil.canAttackNextTurn(att, ai)) {
                     combat.addAttacker(att, att.getController().getOpponent());
                 }
             }

src/main/java/forge/game/phase/Combat.java

@@ -23,6 +23,9 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+
+import org.apache.commons.lang3.tuple.Pair;
+
 import com.google.common.collect.Lists;
 
 import forge.Card;
@@ -46,7 +49,7 @@ import forge.util.maps.MapOfLists;
  * @version $Id$
  */
 public class Combat {
-    private final Player attackerPlayer;
+    private final Player playerWhoAttacks;
     // Defenders, as they are attacked by hostile forces
     private final MapOfLists<GameEntity, AttackingBand> attackedEntities = new HashMapOfLists<GameEntity, AttackingBand>(CollectionSuppliers.<AttackingBand>arrayLists());
     // Blockers to stop the hostile invaders
@@ -55,15 +58,15 @@ public class Combat {
     private final HashMap<Card, Integer> defendingDamageMap = new HashMap<Card, Integer>();
     
     
-    private Map<Card, List<Card>> orderBlockerDamageAssignment = new HashMap<Card, List<Card>>();
+    private Map<Card, List<Card>> attackersOrderedForDamageAssignment = new HashMap<Card, List<Card>>();
-    private Map<Card, List<Card>> orderAttackerDamageAssignment = new HashMap<Card, List<Card>>();
+    private Map<Card, List<Card>> blockersOrderedForDamageAssignment = new HashMap<Card, List<Card>>();
 
 
     public Combat(Player attacker) {
-        attackerPlayer = attacker;
+        playerWhoAttacks = attacker;
 
         // Create keys for all possible attack targets
-        for (Player defender : attackerPlayer.getOpponents()) {
+        for (Player defender : playerWhoAttacks.getOpponents()) {
             this.attackedEntities.ensureCollectionFor(defender);
             List<Card> planeswalkers = CardLists.filter(defender.getCardsIn(ZoneType.Battlefield), CardPredicates.Presets.PLANEWALKERS);
             for (final Card pw : planeswalkers) {
@@ -73,31 +76,13 @@ public class Combat {
     }
     
     public final Player getAttackingPlayer() {
-        return this.attackerPlayer;
+        return this.playerWhoAttacks;
-    }
-
-    public final boolean isCombat() {
-        for(Collection<AttackingBand> abs : attackedEntities.values()) {
-            if(!abs.isEmpty())
-                return true;
-        }
-        return false;
     }
 
     public final List<GameEntity> getDefenders() {
         return Lists.newArrayList(attackedEntities.keySet());
     }
 
-    public final List<Player> getDefendingPlayers() {
-        final List<Player> defending = new ArrayList<Player>();
-        for (final GameEntity o : attackedEntities.keySet()) {
-            if (o instanceof Player) {
-                defending.add((Player) o);
-            }
-        }
-        return defending;
-    }
-
     public final List<Card> getDefendingPlaneswalkers() {
         final List<Card> pwDefending = new ArrayList<Card>();
         for (final GameEntity o : attackedEntities.keySet()) {
@@ -108,24 +93,6 @@ public class Combat {
         return pwDefending;
     }
 
-    // Damage to whatever was protected there. 
-    public final void addDefendingDamage(final int n, final Card source) {
-        final GameEntity ge = this.getDefenderByAttacker(source);
-
-        if (ge instanceof Card) {
-            final Card planeswalker = (Card) ge;
-            planeswalker.addAssignedDamage(n, source);
-
-            return;
-        }
-
-        if (!this.defendingDamageMap.containsKey(source)) {
-            this.defendingDamageMap.put(source, n);
-        } else {
-            this.defendingDamageMap.put(source, this.defendingDamageMap.get(source) + n);
-        }
-    }
-
     public final List<AttackingBand> getAttackingBandsOf(GameEntity defender) {
         return Lists.newArrayList(attackedEntities.get(defender));
     }
@@ -149,9 +116,7 @@ public class Combat {
             System.out.println("Trying to add Attacker " + c + " to missing defender " + defender);
             return;
         }
-        
+
-        
-        
         if (band == null || !attackersOfDefender.contains(band)) {
             band = new AttackingBand(c, defender);
             attackersOfDefender.add(band);
@@ -170,7 +135,7 @@ public class Combat {
         return null;
     }
     
-    private final GameEntity getDefenderByAttacker(final AttackingBand c) {
+    public final GameEntity getDefenderByAttacker(final AttackingBand c) {
         for(Entry<GameEntity, Collection<AttackingBand>> e : attackedEntities.entrySet()) {
             for(AttackingBand ab : e.getValue()) {
                 if ( ab == c )
@@ -217,12 +182,6 @@ public class Combat {
         return ab != null;
     }
 
-    /**
-     * TODO: Write javadoc for this method.
-     * @param card
-     * @param currentDefender
-     * @return
-     */
     public boolean isAttacking(Card card, GameEntity defender) {
         for(Entry<GameEntity, Collection<AttackingBand>> ee : attackedEntities.entrySet()) 
             for(AttackingBand ab : ee.getValue())
@@ -252,6 +211,7 @@ public class Combat {
         
     }
 
+    // Some cards in Alpha may UNBLOCK an attacker, so second parameter is not always-true
     public final void setBlocked(final Card attacker, boolean value) {
         getBandOfAttacker(attacker).setBlocked(value); // called by Curtain of Light, Dazzling Beauty, Trap Runner
     }
@@ -314,24 +274,51 @@ public class Combat {
         return getDefenderPlayerByAttacker(attacker);
     }
 
-    public void setAttackerDamageAssignmentOrder(final Card attacker, final List<Card> blockers) {
+    /** If there are multiple blockers, the Attacker declares the Assignment Order */
-        this.orderAttackerDamageAssignment.put(attacker, blockers);
+    public void orderBlockersForDamageAssignment() {
-    }
 
-    public void setBlockerDamageAssignmentOrder(final Card blocker, final List<Card> attackers) {
+        for(Collection<AttackingBand> abs : attackedEntities.values())
-        this.orderBlockerDamageAssignment.put(blocker, attackers);
+            for (final AttackingBand band : abs) {
-    }
+                if (band.isEmpty()) continue; // there should not be empty bands
 
+                Collection<Card> blockers = blockedBands.get(band);
+                if ( blockers == null || blockers.isEmpty() )
+                    continue;
+
+                for(Card attacker : band.getAttackers()) {
+                    List<Card> orderedBlockers = blockers.size() <= 1 
+                            ? Lists.newArrayList(blockers) 
+                            : playerWhoAttacks.getController().orderBlockers(attacker, (List<Card>)blockers); // we know there's a list
+                    // Damage Ordering needs to take cards like Melee into account, is that happening?
+                    blockersOrderedForDamageAssignment.put(attacker, orderedBlockers);
+                }
+            }
+    }
+    
+    public void orderAttackersForDamageAssignment() {
+        // If there are multiple blockers, the Attacker declares the Assignment Order
+        for (final Card blocker : getAllBlockers()) {
+            List<Card> attackers = getAttackersBlockedBy(blocker);
+            // They need a reverse map here: Blocker => List<Attacker>
+            
+            Player blockerCtrl = blocker.getController();
+            List<Card> orderedAttacker = attackers.size() <= 1 ? attackers : blockerCtrl.getController().orderAttackers(blocker, attackers);
+
+            // Damage Ordering needs to take cards like Melee into account, is that happening?
+            attackersOrderedForDamageAssignment.put(blocker, orderedAttacker);
+        }
+    }
+    
     // removes references to this attacker from all indices and orders
     private void unregisterAttacker(final Card c, AttackingBand ab) {
-        orderAttackerDamageAssignment.remove(c);
+        blockersOrderedForDamageAssignment.remove(c);
         
         Collection<Card> blockers = blockedBands.get(ab);
         if ( blockers != null ) {
             for (Card b : blockers) {
                 // Clear removed attacker from assignment order 
-                if (this.orderBlockerDamageAssignment.containsKey(b)) {
+                if (this.attackersOrderedForDamageAssignment.containsKey(b)) {
-                    this.orderBlockerDamageAssignment.get(b).remove(c);
+                    this.attackersOrderedForDamageAssignment.get(b).remove(c);
                 }
             }
         }
@@ -340,21 +327,31 @@ public class Combat {
 
     // removes references to this defender from all indices and orders
     private void unregisterDefender(final Card c, AttackingBand bandBeingBlocked) {
-        this.orderBlockerDamageAssignment.remove(c);
+        this.attackersOrderedForDamageAssignment.remove(c);
         for(Card atk : bandBeingBlocked.getAttackers()) {
-            if (this.orderAttackerDamageAssignment.containsKey(atk)) {
+            if (this.blockersOrderedForDamageAssignment.containsKey(atk)) {
-                this.orderAttackerDamageAssignment.get(atk).remove(c);
+                this.blockersOrderedForDamageAssignment.get(atk).remove(c);
             }
         }
     }
+    
+    private void removeAttackerFromBand(AttackingBand ab, Card c) {
+        ab.removeAttacker(c);
+
+        // removed
+        if (ab.isEmpty()) {
+            blockedBands.remove(ab);
+            for(Collection<AttackingBand> abs: attackedEntities.values())
+                abs.remove(ab);
+        }
+    }
 
     // remove a combatant whose side is unknown
     public final void removeFromCombat(final Card c) {
         AttackingBand ab = getBandOfAttacker(c);
         if (ab != null) {
             unregisterAttacker(c, ab);
-            ab.removeAttacker(c);
+            removeAttackerFromBand(ab, c);
-            // no need to remove empty bands
         }
 
         // if not found in attackers, look for this card in blockers
@@ -369,6 +366,7 @@ public class Combat {
 
     public final void removeAbsentCombatants() {
         // iterate all attackers and remove them
+        List<Pair<AttackingBand, Card>> toRemoveAtk = new ArrayList<>();
         for(Entry<GameEntity, Collection<AttackingBand>> ee : attackedEntities.entrySet()) {
             for(AttackingBand ab : ee.getValue()) {
                 List<Card> atk = ab.getAttackers();
@@ -376,11 +374,15 @@ public class Combat {
                     Card c = atk.get(i);
                     if ( !c.isInPlay() ) {
                         unregisterAttacker(c, ab);
-                        ab.removeAttacker(c);
+                        toRemoveAtk.add(Pair.of(ab, c));
                     }
                 }
             }
         }
+        // remove as a separate step to avoid modifications with iterator open in for-loop
+        for(Pair<AttackingBand, Card> pair : toRemoveAtk) {
+            removeAttackerFromBand(pair.getKey(), pair.getValue());
+        }
 
         Collection<Card> toRemove = Lists.newArrayList();
         for(Entry<AttackingBand, Collection<Card>> be : blockedBands.entrySet()) {
@@ -423,7 +425,7 @@ public class Combat {
 
         for (final Card blocker : blockers) {
             if (blocker.hasDoubleStrike() || blocker.hasFirstStrike() == firstStrikeDamage) {
-                List<Card> attackers = this.orderBlockerDamageAssignment.get(blocker);
+                List<Card> attackers = this.attackersOrderedForDamageAssignment.get(blocker);
 
                 final int damage = blocker.getNetCombatDamage();
 
@@ -450,7 +452,7 @@ public class Combat {
     private final boolean assignAttackersDamage(boolean firstStrikeDamage) {
      // Assign damage by Attackers
         this.defendingDamageMap.clear(); // this should really happen in deal damage
-        List<Card> blockers = null;
+        List<Card> orderedBlockers = null;
         final List<Card> attackers = this.getAttackers();
         boolean assignedDamage = false;
         for (final Card attacker : attackers) {
@@ -468,10 +470,10 @@ public class Combat {
             AttackingBand band = this.getBandOfAttacker(attacker);
 
             boolean trampler = attacker.hasKeyword("Trample");
-            blockers = this.orderAttackerDamageAssignment.get(attacker);
+            orderedBlockers = this.blockersOrderedForDamageAssignment.get(attacker);
             assignedDamage = true;
             // If the Attacker is unblocked, or it's a trampler and has 0 blockers, deal damage to defender
-            if (blockers == null || blockers.isEmpty()) {
+            if (orderedBlockers == null || orderedBlockers.isEmpty()) {
                 if (trampler || !band.isBlocked()) {
                     this.addDefendingDamage(damageDealt, attacker);
                 } // No damage happens if blocked but no blockers left
@@ -482,11 +484,11 @@ public class Combat {
                 // It allows the defending player to assign damage instead of the attacking player
                 if (defender instanceof Player && defender.hasKeyword("You assign combat damage of each creature attacking you.")) {
                     assigningPlayer = (Player)defender;
-                } else if ( AttackingBand.isValidBand(blockers, true)){
+                } else if ( AttackingBand.isValidBand(orderedBlockers, true)){
-                    assigningPlayer = blockers.get(0).getController();
+                    assigningPlayer = orderedBlockers.get(0).getController();
                 }
 
-                Map<Card, Integer> map = assigningPlayer.getController().assignCombatDamage(attacker, blockers, damageDealt, defender, this.getAttackingPlayer() != assigningPlayer);
+                Map<Card, Integer> map = assigningPlayer.getController().assignCombatDamage(attacker, orderedBlockers, damageDealt, defender, this.getAttackingPlayer() != assigningPlayer);
                 for (Entry<Card, Integer> dt : map.entrySet()) {
                     if( dt.getKey() == null) {
                         if (dt.getValue() > 0) 
@@ -502,6 +504,24 @@ public class Combat {
         return assignedDamage;
     }
 
+    // Damage to whatever was protected there. 
+    private final void addDefendingDamage(final int n, final Card source) {
+        final GameEntity ge = this.getDefenderByAttacker(source);
+    
+        if (ge instanceof Card) {
+            final Card planeswalker = (Card) ge;
+            planeswalker.addAssignedDamage(n, source);
+    
+            return;
+        }
+    
+        if (!this.defendingDamageMap.containsKey(source)) {
+            this.defendingDamageMap.put(source, n);
+        } else {
+            this.defendingDamageMap.put(source, this.defendingDamageMap.get(source) + n);
+        }
+    }
+
     public final boolean assignCombatDamage(boolean firstStrikeDamage) {
         boolean assignedDamage = assignAttackersDamage(firstStrikeDamage);
         assignedDamage |= assignBlockersDamage(firstStrikeDamage);

src/main/java/forge/game/phase/CombatUtil.java

@@ -401,46 +401,6 @@ public class CombatUtil {
         return null;
     }
 
-    public static void orderMultipleBlockers(final Combat combat) {
-        // If there are multiple blockers, the Attacker declares the Assignment Order
-        final Player player = combat.getAttackingPlayer();
-        for (final AttackingBand band : combat.getAttackingBands()) {
-            List<Card> attackers = band.getAttackers();
-            if (attackers.isEmpty()) {
-                continue;
-            }
-
-            List<Card> blockers = combat.getBlockers(band);
-            for(Card attacker : attackers) {
-                List<Card> orderedBlockers = null;
-                if (blockers.size() <= 1) {
-                    orderedBlockers = blockers;
-                } else {
-                    // Damage Ordering needs to take cards like Melee into account, is that happening?
-                    orderedBlockers = player.getController().orderBlockers(attacker, blockers);
-                }
-                combat.setAttackerDamageAssignmentOrder(attacker, orderedBlockers);
-            }
-        }
-    }
-
-    public static void orderBlockingMultipleAttackers(final Combat combat) {
-        // If there are multiple blockers, the Attacker declares the Assignment Order
-        for (final Card blocker : combat.getAllBlockers()) {
-            List<Card> attackers = combat.getAttackersBlockedBy(blocker);
-            List<Card> orderedAttacker = null;
-
-            if (attackers.size() <= 1) {
-                orderedAttacker = attackers;
-            } else {
-                // Damage Ordering needs to take cards like Melee into account, is that happening?
-                orderedAttacker = blocker.getController().getController().orderAttackers(blocker, attackers);
-            }
-
-            combat.setBlockerDamageAssignmentOrder(blocker, orderedAttacker);
-        }
-    }
-
     // can the blocker block an attacker with a lure effect?
     /**
      * <p>

src/main/java/forge/game/phase/PhaseHandler.java

@@ -527,8 +527,9 @@ public class PhaseHandler implements java.io.Serializable {
             if ( game.isGameOver() ) // they just like to close window at any moment
                 return;
         } while(p != playerTurn);
-        CombatUtil.orderMultipleBlockers(combat);
+        
-        CombatUtil.orderBlockingMultipleAttackers(combat);
+        combat.orderBlockersForDamageAssignment();
+        combat.orderAttackersForDamageAssignment();
 
         combat.removeAbsentCombatants();