[NCC] Address comments for Lethal Scheme

Mage.Sets/src/mage/cards/l/LethalScheme.java

@@ -3,7 +3,6 @@ package mage.cards.l;
 import java.util.*;
 import java.util.stream.Collectors;
 
-import javafx.util.Pair;
 import mage.MageObjectReference;
 import mage.abilities.Ability;
 import mage.abilities.effects.OneShotEffect;
@@ -16,14 +15,11 @@ import mage.choices.Choice;
 import mage.choices.ChoiceImpl;
 import mage.constants.CardType;
 import mage.constants.Outcome;
-import mage.constants.WatcherScope;
 import mage.game.Game;
-import mage.game.events.GameEvent;
 import mage.game.permanent.Permanent;
-import mage.game.stack.Spell;
 import mage.players.Player;
 import mage.target.common.TargetCreatureOrPlaneswalker;
-import mage.watchers.Watcher;
+import mage.watchers.common.EachCreatureThatConvokedSourceWatcher;
 
 /**
  *
@@ -41,7 +37,7 @@ public final class LethalScheme extends CardImpl {
         this.getSpellAbility().addEffect(new DestroyTargetEffect());
         this.getSpellAbility().addTarget(new TargetCreatureOrPlaneswalker());
         // Each creature that convoked Lethal Scheme connives.
-        this.getSpellAbility().addWatcher(new LethalSchemeWatcher());
+        this.getSpellAbility().addWatcher(new EachCreatureThatConvokedSourceWatcher());
         this.getSpellAbility().addEffect(new LethalSchemeEffect());
     }
 
@@ -74,17 +70,23 @@ class LethalSchemeEffect extends OneShotEffect {
 
     @Override
     public boolean apply(Game game, Ability source) {
-        LethalSchemeWatcher watcher = game.getState().getWatcher(LethalSchemeWatcher.class);
+        EachCreatureThatConvokedSourceWatcher watcher = game.getState().getWatcher(EachCreatureThatConvokedSourceWatcher.class);
-        if (watcher != null) {
+        if (watcher == null) {
+            return false;
+        }
+
         MageObjectReference mor = new MageObjectReference(source.getSourceId(), game);
         Set<MageObjectReference> creatures = watcher.getConvokingCreatures(mor);
-            if (creatures != null) {
+        if (creatures == null) {
-                Set<Pair<UUID,Permanent>> playerPermanentsPairs =
+            return false;
+        }
+
+        Set<AbstractMap.SimpleEntry<UUID, Permanent>> playerPermanentsPairs =
             creatures
                 .stream()
                 .map(creatureMOR -> creatureMOR.getPermanentOrLKIBattlefield(game))
                 .filter(Objects::nonNull)
-                        .map(permanent -> new Pair<>(permanent.getControllerId(),permanent))
+                .map(permanent -> new AbstractMap.SimpleEntry<>(permanent.getControllerId(),permanent))
                 .collect(Collectors.toSet());
 
         Map<Player, Set<Permanent>> permanentsPerPlayer = new HashMap<>();
@@ -107,7 +109,7 @@ class LethalSchemeEffect extends OneShotEffect {
             .stream()
             .map(game::getPlayer)
             .filter(Objects::nonNull)
-                    .filter(player -> permanentsPerPlayer.containsKey(player))
+            .filter(permanentsPerPlayer::containsKey)
             .collect(Collectors.toList())) {
 
             Set<Permanent> permanents = permanentsPerPlayer.get(player);
@@ -116,9 +118,11 @@ class LethalSchemeEffect extends OneShotEffect {
                 Choice choiceForThisLoop = new ChoiceImpl(true);
                 choiceForThisLoop.setMessage("Choose next connive to resolve.");
 
-                        permanents.stream()
+                permanents.forEach(permanent -> choiceForThisLoop.getChoices().add(permanent.getIdName()));
-                                .forEach(permanent -> choiceForThisLoop.getChoices().add(permanent.getIdName()));
 
+                if (player == null) {
+                    break;
+                }
                 player.choose(Outcome.Neutral, choiceForThisLoop, game);
 
                 String choice = choiceForThisLoop.getChoice();
@@ -128,49 +132,6 @@ class LethalSchemeEffect extends OneShotEffect {
                 permanents.remove(choicePermanent);
             }
         }
-            }
         return true;
     }
-        return false;
-    }
-}
-
-// Based on "Venerated Loxodon"
-class LethalSchemeWatcher extends Watcher {
-
-    private final Map<MageObjectReference, Set<MageObjectReference>> convokingCreatures = new HashMap<>();
-
-    public LethalSchemeWatcher() {
-        super(WatcherScope.GAME);
-    }
-
-    @Override
-    public void watch(GameEvent event, Game game) {
-        if (event.getType() == GameEvent.EventType.CONVOKED) {
-            Spell spell = game.getSpell(event.getSourceId());
-            Permanent tappedCreature = game.getPermanentOrLKIBattlefield(event.getTargetId());
-            if (spell != null && tappedCreature != null) {
-                MageObjectReference convokedSpell = new MageObjectReference(spell.getSourceId(), game);
-                Set<MageObjectReference> creatures;
-                if (convokingCreatures.containsKey(convokedSpell)) {
-                    creatures = convokingCreatures.get(convokedSpell);
-                } else {
-                    creatures = new HashSet<>();
-                    convokingCreatures.put(convokedSpell, creatures);
-                }
-                creatures.add(new MageObjectReference(tappedCreature, game));
-            }
-        }
-    }
-
-    public Set<MageObjectReference> getConvokingCreatures(MageObjectReference mor) {
-        return convokingCreatures.get(mor);
-    }
-
-    @Override
-    public void reset() {
-        super.reset();
-        convokingCreatures.clear();
-    }
-
 }

Mage.Sets/src/mage/cards/v/VeneratedLoxodon.java

@@ -1,8 +1,5 @@
 package mage.cards.v;
 
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
 import mage.MageInt;
@@ -16,13 +13,10 @@ import mage.cards.CardSetInfo;
 import mage.constants.CardType;
 import mage.constants.Outcome;
 import mage.constants.SubType;
-import mage.constants.WatcherScope;
 import mage.counters.CounterType;
 import mage.game.Game;
-import mage.game.events.GameEvent;
 import mage.game.permanent.Permanent;
-import mage.game.stack.Spell;
+import mage.watchers.common.EachCreatureThatConvokedSourceWatcher;
-import mage.watchers.Watcher;
 
 /**
  *
@@ -42,7 +36,7 @@ public final class VeneratedLoxodon extends CardImpl {
         this.addAbility(new ConvokeAbility());
 
         // When Venerated Loxodon enters the battlefield, put a +1/+1 counter on each creature that convoked it.
-        this.addAbility(new EntersBattlefieldTriggeredAbility(new VeneratedLoxodonEffect(), false), new VeneratedLoxodonWatcher());
+        this.addAbility(new EntersBattlefieldTriggeredAbility(new VeneratedLoxodonEffect(), false), new EachCreatureThatConvokedSourceWatcher());
     }
 
     private VeneratedLoxodon(final VeneratedLoxodon card) {
@@ -73,7 +67,7 @@ class VeneratedLoxodonEffect extends OneShotEffect {
 
     @Override
     public boolean apply(Game game, Ability source) {
-        VeneratedLoxodonWatcher watcher = game.getState().getWatcher(VeneratedLoxodonWatcher.class);
+        EachCreatureThatConvokedSourceWatcher watcher = game.getState().getWatcher(EachCreatureThatConvokedSourceWatcher.class);
         if (watcher != null) {
             MageObjectReference mor = new MageObjectReference(source.getSourceId(), source.getSourceObjectZoneChangeCounter() - 1, game); // -1 because of spell on the stack
             Set<MageObjectReference> creatures = watcher.getConvokingCreatures(mor);
@@ -91,41 +85,3 @@ class VeneratedLoxodonEffect extends OneShotEffect {
     }
 }
 
-class VeneratedLoxodonWatcher extends Watcher {
-
-    private final Map<MageObjectReference, Set<MageObjectReference>> convokingCreatures = new HashMap<>();
-
-    public VeneratedLoxodonWatcher() {
-        super(WatcherScope.GAME);
-    }
-
-    @Override
-    public void watch(GameEvent event, Game game) {
-        if (event.getType() == GameEvent.EventType.CONVOKED) {
-            Spell spell = game.getSpell(event.getSourceId());
-            Permanent tappedCreature = game.getPermanentOrLKIBattlefield(event.getTargetId());
-            if (spell != null && tappedCreature != null) {
-                MageObjectReference convokedSpell = new MageObjectReference(spell.getSourceId(), game);
-                Set<MageObjectReference> creatures;
-                if (convokingCreatures.containsKey(convokedSpell)) {
-                    creatures = convokingCreatures.get(convokedSpell);
-                } else {
-                    creatures = new HashSet<>();
-                    convokingCreatures.put(convokedSpell, creatures);
-                }
-                creatures.add(new MageObjectReference(tappedCreature, game));
-            }
-        }
-    }
-
-    public Set<MageObjectReference> getConvokingCreatures(MageObjectReference mor) {
-        return convokingCreatures.get(mor);
-    }
-
-    @Override
-    public void reset() {
-        super.reset();
-        convokingCreatures.clear();
-    }
-
-}

Mage.Tests/src/test/java/org/mage/test/cards/single/ncc/LethalSchemeTest.java

@@ -19,14 +19,14 @@ public class LethalSchemeTest extends CardTestPlayerBase {
      * Convoke
      * Destroy target creature or planeswalker. Each creature that convoked Lethal Scheme connives.
      */
-    private String scheme = "Lethal Scheme";
+    private static final String scheme = "Lethal Scheme";
 
-    private String vanguard = "Elite Vanguard"; // vanilla 2/1
+    private static final String vanguard = "Elite Vanguard"; // vanilla 2/1
-    private String bear = "Grizzly Bears"; // vanilla 2/2
+    private static final String bear = "Grizzly Bears"; // vanilla 2/2
-    private String ogre = "Gray Ogre"; // vanilla 2/2
+    private static final String ogre = "Gray Ogre"; // vanilla 2/2
-    private String mino = "Felhide Minotaur"; // vanilla 2/3
+    private static final String mino = "Felhide Minotaur"; // vanilla 2/3
 
-    private String blade = "Doom Blade"; // instant {1}{B} destroy target non-black creature.
+    private static final String blade = "Doom Blade"; // instant {1}{B} destroy target non-black creature.
     /*
      * Act of Aggression {3}{R/P}{R/P}
      * Instant
@@ -34,11 +34,11 @@ public class LethalSchemeTest extends CardTestPlayerBase {
      * Gain control of target creature an opponent controls until end of turn.
      * Untap that creature. It gains haste until end of turn.
      */
-    private String aggression = "Act of Aggression";
+    private static final String aggression = "Act of Aggression";
 
-    private String swamp = "Swamp";
+    private static final String swamp = "Swamp";
-    private String island = "Island";
+    private static final String island = "Island";
-    private String mountain = "Mountain";
+    private static final String mountain = "Mountain";
 
     @Test
     public void LethalSchemeNoConvoke() {
@@ -58,7 +58,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
         setStrictChooseMode(true);
         setStopAt(1, PhaseStep.BEGIN_COMBAT);
         execute();
-        assertAllCommandsUsed();
 
         assertPermanentCount(playerB, 0);
         assertGraveyardCount(playerA, 1);
@@ -97,7 +96,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
 
         setStopAt(1, PhaseStep.END_TURN);
         execute();
-        assertAllCommandsUsed();
 
         assertPermanentCount(playerB, 0);
         assertHandCount(playerA, 1);
@@ -144,7 +142,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
 
         setStopAt(1, PhaseStep.END_TURN);
         execute();
-        assertAllCommandsUsed();
 
         assertPermanentCount(playerB, 0);
 
@@ -191,7 +188,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
 
         setStopAt(1, PhaseStep.END_TURN);
         execute();
-        assertAllCommandsUsed();
 
         assertPermanentCount(playerB, 0);
 
@@ -237,7 +233,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
 
         setStopAt(1, PhaseStep.END_TURN);
         execute();
-        assertAllCommandsUsed();
 
         assertPermanentCount(playerB, 0);
 
@@ -291,7 +286,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
 
         setStopAt(1, PhaseStep.END_TURN);
         execute();
-        assertAllCommandsUsed();
 
         assertPermanentCount(playerB, 0);
 
@@ -349,7 +343,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
 
         setStopAt(1, PhaseStep.END_TURN);
         execute();
-        assertAllCommandsUsed();
 
         assertPermanentCount(playerB, 0);
 
@@ -426,7 +419,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
 
         setStopAt(1, PhaseStep.POSTCOMBAT_MAIN);
         execute();
-        assertAllCommandsUsed();
 
         assertGraveyardCount(playerA, 2); // Lethal Scheme + Doom Blade
         assertGraveyardCount(playerB, 3); // Act of Aggression + Elite Vanguard + Island
@@ -486,7 +478,6 @@ public class LethalSchemeTest extends CardTestPlayerBase {
 
         setStopAt(1, PhaseStep.POSTCOMBAT_MAIN);
         execute();
-        assertAllCommandsUsed();
 
         assertGraveyardCount(playerA, 3); // Grizzly Bears + Lethal Scheme + Gray Ogre
 

Mage/src/main/java/mage/watchers/common/EachCreatureThatConvokedSourceWatcher.java

@@ -0,0 +1,59 @@
+package mage.watchers.common;
+
+import mage.MageObjectReference;
+import mage.constants.WatcherScope;
+import mage.game.Game;
+import mage.game.events.GameEvent;
+import mage.game.permanent.Permanent;
+import mage.game.stack.Spell;
+import mage.watchers.Watcher;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * @author LevelX2
+ */
+public class EachCreatureThatConvokedSourceWatcher extends Watcher {
+
+    private final Map<MageObjectReference, Set<MageObjectReference>> convokingCreatures = new HashMap<>();
+
+    public EachCreatureThatConvokedSourceWatcher() {
+        super(WatcherScope.GAME);
+    }
+
+    @Override
+    public void watch(GameEvent event, Game game) {
+        if (event.getType() != GameEvent.EventType.CONVOKED) {
+            return;
+        }
+
+        Spell spell = game.getSpell(event.getSourceId());
+        Permanent tappedCreature = game.getPermanentOrLKIBattlefield(event.getTargetId());
+        if (spell == null || tappedCreature == null) {
+            return;
+        }
+
+        MageObjectReference convokedSpell = new MageObjectReference(spell.getSourceId(), game);
+        Set<MageObjectReference> creatures;
+        if (convokingCreatures.containsKey(convokedSpell)) {
+            creatures = convokingCreatures.get(convokedSpell);
+        } else {
+            creatures = new HashSet<>();
+            convokingCreatures.put(convokedSpell, creatures);
+        }
+        creatures.add(new MageObjectReference(tappedCreature, game));
+    }
+
+    public Set<MageObjectReference> getConvokingCreatures(MageObjectReference mor) {
+        return convokingCreatures.get(mor);
+    }
+
+    @Override
+    public void reset() {
+        super.reset();
+        convokingCreatures.clear();
+    }
+}