PLANNER-611 Move.doMove() must return undo move to allow CompositeMov…

…e to return a correct undo move

optaplanner-core/src/main/java/org/optaplanner/core/impl/constructionheuristic/decider/ConstructionHeuristicDecider.java

@@ -122,9 +122,8 @@ public void decideNextStep(ConstructionHeuristicStepScope stepScope, Placement p
     private void doMove(ConstructionHeuristicMoveScope moveScope) {
         ScoreDirector scoreDirector = moveScope.getScoreDirector();
         Move move = moveScope.getMove();
-        Move undoMove = move.createUndoMove(scoreDirector);
+        Move undoMove = move.doMove(scoreDirector);
         moveScope.setUndoMove(undoMove);
-        move.doMove(scoreDirector);
         processMove(moveScope);
         undoMove.doMove(scoreDirector);
         if (assertExpectedUndoMoveScore) {

optaplanner-core/src/main/java/org/optaplanner/core/impl/exhaustivesearch/decider/ExhaustiveSearchDecider.java

@@ -146,9 +146,8 @@ public void expandNode(ExhaustiveSearchStepScope<Solution_> stepScope) {
     private void doMove(ExhaustiveSearchStepScope<Solution_> stepScope, ExhaustiveSearchNode moveNode) {
         InnerScoreDirector<Solution_> scoreDirector = stepScope.getScoreDirector();
         Move<Solution_> move = moveNode.getMove();
-        Move<Solution_> undoMove = move.createUndoMove(scoreDirector);
+        Move<Solution_> undoMove = move.doMove(scoreDirector);
         moveNode.setUndoMove(undoMove);
-        move.doMove(scoreDirector);
         processMove(stepScope, moveNode);
         undoMove.doMove(scoreDirector);
         if (assertExpectedUndoMoveScore) {

optaplanner-core/src/main/java/org/optaplanner/core/impl/heuristic/move/AbstractMove.java

@@ -32,11 +32,21 @@ public String getSimpleMoveTypeDescription() {
     }
 
     @Override
-    public final void doMove(ScoreDirector<Solution_> scoreDirector) {
+    public final AbstractMove<Solution_> doMove(ScoreDirector<Solution_> scoreDirector) {
+        AbstractMove<Solution_> undoMove = createUndoMove(scoreDirector);
         doMoveOnGenuineVariables(scoreDirector);
         scoreDirector.triggerVariableListeners();
+        return undoMove;
     }
 
+    /**
+     * Called before the move is done, so the move can be evaluated and then be undone
+     * without resulting into a permanent change in the solution.
+     * @param scoreDirector the {@link ScoreDirector} not yet modified by the move.
+     * @return an undoMove which does the exact opposite of this move.
+     */
+    protected abstract AbstractMove<Solution_> createUndoMove(ScoreDirector<Solution_> scoreDirector);
+
     /**
      * Like {@link #doMove(ScoreDirector)} but without the {@link ScoreDirector#triggerVariableListeners()} call
      * (because {@link #doMove(ScoreDirector)} already does that).

optaplanner-core/src/main/java/org/optaplanner/core/impl/heuristic/move/CompositeMove.java

@@ -96,24 +96,17 @@ public boolean isMoveDoable(ScoreDirector<Solution_> scoreDirector) {
     }
 
     @Override
-    public CompositeMove<Solution_> createUndoMove(ScoreDirector<Solution_> scoreDirector) {
+    public CompositeMove<Solution_> doMove(ScoreDirector<Solution_> scoreDirector) {
         Move<Solution_>[] undoMoves = new Move[moves.length];
         for (int i = 0; i < moves.length; i++) {
-            // Note: this undoMove creation doesn't have the effect yet of a previous move in the moveList
-            Move<Solution_> undoMove = moves[i].createUndoMove(scoreDirector);
-            undoMoves[moves.length - 1 - i] = undoMove;
-        }
-        return new CompositeMove<>(undoMoves);
-    }
-
-    @Override
-    public void doMove(ScoreDirector<Solution_> scoreDirector) {
-        for (Move<Solution_> move : moves) {
             // Calls scoreDirector.triggerVariableListeners() between moves
             // because a later move can depend on the shadow variables changed by an earlier move
-            move.doMove(scoreDirector);
+            Move<Solution_> undoMove = moves[i].doMove(scoreDirector);
+            // Undo in reverse order and each undoMove is created after previous moves have been done
+            undoMoves[moves.length - 1 - i] = undoMove;
         }
         // No need to call scoreDirector.triggerVariableListeners() because Move.doMove() already does it for every move.
+        return new CompositeMove<>(undoMoves);
     }
 
     // ************************************************************************

optaplanner-core/src/main/java/org/optaplanner/core/impl/heuristic/move/Move.java

@@ -66,14 +66,6 @@ public interface Move<Solution_> {
      */
     boolean isMoveDoable(ScoreDirector<Solution_> scoreDirector);
 
-    /**
-     * Called before the move is done, so the move can be evaluated and then be undone
-     * without resulting into a permanent change in the solution.
-     * @param scoreDirector the {@link ScoreDirector} not yet modified by the move.
-     * @return an undoMove which does the exact opposite of this move.
-     */
-    Move<Solution_> createUndoMove(ScoreDirector<Solution_> scoreDirector);
-
     /**
      * Does the move (which indirectly affects the {@link ScoreDirector#getWorkingSolution()}).
      * When the {@link PlanningSolution working solution} is modified, the {@link ScoreDirector} must be correctly notified
@@ -82,9 +74,13 @@ public interface Move<Solution_> {
      * otherwise later calculated {@link Score}s will be corrupted.
      * <p>
      * This method must end with calling {@link ScoreDirector#triggerVariableListeners()} to ensure all shadow variables are updated.
-     * @param scoreDirector never null, the {@link ScoreDirector} that needs to get notified of the changes.
+     * <p>
+     * This method must return an undo move, so the move can be evaluated and then be undone
+     * without resulting into a permanent change in the solution.
+     * @param scoreDirector never null, the {@link ScoreDirector} that needs to get notified of the changes
+     * @return an undoMove which does the exact opposite of this move
      */
-    void doMove(ScoreDirector<Solution_> scoreDirector);
+    Move<Solution_> doMove(ScoreDirector<Solution_> scoreDirector);
 
     // ************************************************************************
     // Introspection methods

optaplanner-core/src/main/java/org/optaplanner/core/impl/localsearch/decider/LocalSearchDecider.java

@@ -143,9 +143,8 @@ public void decideNextStep(LocalSearchStepScope<Solution_> stepScope) {
     private void doMove(LocalSearchMoveScope<Solution_> moveScope) {
         ScoreDirector<Solution_> scoreDirector = moveScope.getScoreDirector();
         Move<Solution_> move = moveScope.getMove();
-        Move<Solution_> undoMove = move.createUndoMove(scoreDirector);
+        Move<Solution_> undoMove = move.doMove(scoreDirector);
         moveScope.setUndoMove(undoMove);
-        move.doMove(scoreDirector);
         processMove(moveScope);
         undoMove.doMove(scoreDirector);
         if (assertExpectedUndoMoveScore) {

optaplanner-core/src/test/java/org/optaplanner/core/impl/heuristic/move/CompositeMoveTest.java

@@ -46,7 +46,7 @@ public void createUndoMove() {
         DummyMove b = new DummyMove("b");
         DummyMove c = new DummyMove("c");
         CompositeMove<TestdataSolution> move = new CompositeMove<>(a, b, c);
-        CompositeMove<TestdataSolution> undoMove = move.createUndoMove(scoreDirector);
+        CompositeMove<TestdataSolution> undoMove = move.doMove(scoreDirector);
         assertAllCodesOfArray(move.getMoves(), "a", "b", "c");
         assertAllCodesOfArray(undoMove.getMoves(), "undo c", "undo b", "undo a");
     }
@@ -122,7 +122,7 @@ public void equals() {
         assertTrue(move.equals(move));
     }
 
-    @Test @Ignore("PLANNER-611") // TODO https://issues.jboss.org/browse/PLANNER-611
+    @Test
     public void interconnectedChildMoves() {
         TestdataSolution solution = new TestdataSolution("s1");
         TestdataValue v1 = new TestdataValue("v1");
@@ -143,8 +143,7 @@ public void interconnectedChildMoves() {
 
         ScoreDirector<TestdataSolution> scoreDirector
                 = mockScoreDirector(variableDescriptor.getEntityDescriptor().getSolutionDescriptor());
-        Move<TestdataSolution> undoMove = move.createUndoMove(scoreDirector);
-        move.doMove(scoreDirector);
+        Move<TestdataSolution> undoMove = move.doMove(scoreDirector);
 
         assertSame(v3, e1.getValue());
         assertSame(v1, e2.getValue());