chore: address comments

zepfred · zepfred · commit 7b603274af2c · 2025-11-04T10:27:54.000-03:00
diff --git a/core/src/main/java/ai/timefold/solver/core/api/domain/valuerange/ValueRange.java b/core/src/main/java/ai/timefold/solver/core/api/domain/valuerange/ValueRange.java
@@ -8,7 +8,6 @@
 
 import ai.timefold.solver.core.api.domain.variable.PlanningListVariable;
 import ai.timefold.solver.core.api.domain.variable.PlanningVariable;
-import ai.timefold.solver.core.impl.domain.valuerange.sort.ValueRangeSorter;
 
 import org.jspecify.annotations.NullMarked;
 import org.jspecify.annotations.Nullable;
@@ -46,14 +45,6 @@ public interface ValueRange<T> {
      */
     boolean contains(@Nullable T value);
 
-    /**
-     * The sorting operation copies the current value range and sorts it using the provided sorter.
-     *
-     * @param sorter never null, the value range sorter
-     * @return A new instance of the value range, with the data sorted.
-     */
-    ValueRange<T> sort(ValueRangeSorter<T> sorter);
-
     /**
      * Select in random order, but without shuffling the elements.
      * Each element might be selected multiple times.
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/AbstractCountableValueRange.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/AbstractCountableValueRange.java
@@ -5,6 +5,7 @@
 import ai.timefold.solver.core.api.domain.valuerange.CountableValueRange;
 import ai.timefold.solver.core.api.domain.valuerange.ValueRange;
 import ai.timefold.solver.core.api.domain.valuerange.ValueRangeFactory;
+import ai.timefold.solver.core.impl.domain.valuerange.sort.SortableValueRange;
 import ai.timefold.solver.core.impl.domain.valuerange.sort.ValueRangeSorter;
 
 import org.jspecify.annotations.NullMarked;
@@ -17,7 +18,7 @@
  * @see ValueRangeFactory
  */
 @NullMarked
-public abstract class AbstractCountableValueRange<T> implements CountableValueRange<T> {
+public abstract class AbstractCountableValueRange<T> implements CountableValueRange<T>, SortableValueRange<T> {
 
     /**
      * Certain optimizations can be applied if {@link Object#equals(Object)} can be relied upon
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/AbstractUncountableValueRange.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/AbstractUncountableValueRange.java
@@ -3,6 +3,7 @@
 import ai.timefold.solver.core.api.domain.valuerange.CountableValueRange;
 import ai.timefold.solver.core.api.domain.valuerange.ValueRange;
 import ai.timefold.solver.core.api.domain.valuerange.ValueRangeFactory;
+import ai.timefold.solver.core.impl.domain.valuerange.sort.SortableValueRange;
 import ai.timefold.solver.core.impl.domain.valuerange.sort.ValueRangeSorter;
 
 /**
@@ -15,7 +16,7 @@
  *             Use {@link CountableValueRange} instead, and configure a step.
  */
 @Deprecated(forRemoval = true, since = "1.1.0")
-public abstract class AbstractUncountableValueRange<T> implements ValueRange<T> {
+public abstract class AbstractUncountableValueRange<T> implements ValueRange<T>, SortableValueRange<T> {
 
     @Override
     public ValueRange<T> sort(ValueRangeSorter<T> sorter) {
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/sort/SelectionSorterAdapter.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/sort/SelectionSorterAdapter.java
@@ -1,12 +1,9 @@
 package ai.timefold.solver.core.impl.domain.valuerange.sort;
 
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 import java.util.SortedSet;
 
-import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionSetSorter;
 import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionSorter;
 
 import org.jspecify.annotations.NullMarked;
@@ -21,18 +18,11 @@ public static <Solution_, T> ValueRangeSorter<T> of(Solution_ solution, Selectio
 
     @Override
     public List<T> sort(List<T> selectionList) {
-        var newList = new ArrayList<>(selectionList);
-        selectionSorter.sort(solution, newList);
-        return Collections.unmodifiableList(newList);
+        return selectionSorter.sort(solution, selectionList);
     }
 
     @Override
-    @SuppressWarnings({ "rawtypes", "unchecked" })
     public SortedSet<T> sort(Set<T> selectionSet) {
-        if (!(selectionSorter instanceof SelectionSetSorter selectionSetSorter)) {
-            throw new IllegalStateException(
-                    "Impossible state: the sorting operation cannot be performed because the sorter does not support sorting collection sets.");
-        }
-        return selectionSetSorter.sort(solution, selectionSet);
+        return selectionSorter.sort(solution, selectionSet);
     }
 }
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/sort/SortableValueRange.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/sort/SortableValueRange.java
@@ -0,0 +1,18 @@
+package ai.timefold.solver.core.impl.domain.valuerange.sort;
+
+import ai.timefold.solver.core.api.domain.valuerange.ValueRange;
+
+import org.jspecify.annotations.NullMarked;
+
+@NullMarked
+@FunctionalInterface
+public interface SortableValueRange<T> {
+
+    /**
+     * The sorting operation copies the current value range and sorts it using the provided sorter.
+     *
+     * @param sorter never null, the value range sorter
+     * @return A new instance of the value range, with the data sorted.
+     */
+    ValueRange<T> sort(ValueRangeSorter<T> sorter);
+}
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/ComparatorFactorySelectionSorter.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/ComparatorFactorySelectionSorter.java
@@ -1,5 +1,6 @@
 package ai.timefold.solver.core.impl.heuristic.selector.common.decorator;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
@@ -21,8 +22,7 @@
  * @param <T> the selection type
  */
 @NullMarked
-public final class ComparatorFactorySelectionSorter<Solution_, T>
-        implements SelectionSorter<Solution_, T>, SelectionSetSorter<Solution_, T> {
+public final class ComparatorFactorySelectionSorter<Solution_, T> implements SelectionSorter<Solution_, T> {
 
     private final ComparatorFactory<Solution_, T> selectionComparatorFactory;
     private final SelectionSorterOrder selectionSorterOrder;
@@ -41,9 +41,11 @@ private Comparator<T> getAppliedComparator(Comparator<T> comparator) {
     }
 
     @Override
-    public void sort(Solution_ solution, List<T> selectionList) {
+    public List<T> sort(Solution_ solution, List<T> selectionList) {
         var appliedComparator = getAppliedComparator(selectionComparatorFactory.createComparator(solution));
-        selectionList.sort(appliedComparator);
+        var sortedList = new ArrayList<>(selectionList);
+        sortedList.sort(appliedComparator);
+        return Collections.unmodifiableList(sortedList);
     }
 
     @Override
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/ComparatorSelectionSorter.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/ComparatorSelectionSorter.java
@@ -1,5 +1,6 @@
 package ai.timefold.solver.core.impl.heuristic.selector.common.decorator;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
@@ -20,8 +21,7 @@
  * @param <T> the selection type
  */
 @NullMarked
-public final class ComparatorSelectionSorter<Solution_, T>
-        implements SelectionSorter<Solution_, T>, SelectionSetSorter<Solution_, T> {
+public final class ComparatorSelectionSorter<Solution_, T> implements SelectionSorter<Solution_, T> {
 
     private final Comparator<T> appliedComparator;
 
@@ -40,8 +40,10 @@ public ComparatorSelectionSorter(Comparator<T> comparator, SelectionSorterOrder
     }
 
     @Override
-    public void sort(Solution_ solution, List<T> selectionList) {
-        selectionList.sort(appliedComparator);
+    public List<T> sort(Solution_ solution, List<T> selectionList) {
+        var sortedList = new ArrayList<>(selectionList);
+        sortedList.sort(appliedComparator);
+        return Collections.unmodifiableList(sortedList);
     }
 
     @Override
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/SelectionSetSorter.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/SelectionSetSorter.java
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/SelectionSorter.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/SelectionSorter.java
@@ -1,6 +1,8 @@
 package ai.timefold.solver.core.impl.heuristic.selector.common.decorator;
 
 import java.util.List;
+import java.util.Set;
+import java.util.SortedSet;
 
 import ai.timefold.solver.core.api.domain.entity.PlanningEntity;
 import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
@@ -21,16 +23,23 @@
  * @param <T> the selection type
  */
 @NullMarked
-@FunctionalInterface
 public interface SelectionSorter<Solution_, T> {
 
     /**
-     * Apply an in-place sorting operation.
-     *
+     * Creates a copy of the provided list and sort the data.
+     * 
      * @param solution never null, the current solution
      * @param selectionList never null, a {@link List}
      *        of {@link PlanningEntity}, planningValue, {@link Move} or {@link Selector} that will be sorted.
      */
-    void sort(Solution_ solution, List<T> selectionList);
+    List<T> sort(Solution_ solution, List<T> selectionList);
+
+    /**
+     * Creates a copy of the provided set and sort the data.
+     *
+     * @param solution never null, the current solution
+     * @param selectionSet never null, a {@link Set} of values that will be used as input for sorting.
+     */
+    SortedSet<T> sort(Solution_ solution, Set<T> selectionSet);
 
 }
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/SortingEntitySelector.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/SortingEntitySelector.java
@@ -60,7 +60,8 @@ public void constructCache(SolverScope<Solution_> solverScope) {
             return;
         }
         super.constructCache(solverScope);
-        sorter.sort(solverScope.getScoreDirector().getWorkingSolution(), cachedEntityList);
+        // We need to update the cachedEntityList since the sorter will copy the data and return a sorted list
+        cachedEntityList = sorter.sort(solverScope.getScoreDirector().getWorkingSolution(), cachedEntityList);
         logger.trace("    Sorted cachedEntityList: size ({}), entitySelector ({}).",
                 cachedEntityList.size(), this);
     }
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/decorator/SortingMoveSelector.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/decorator/SortingMoveSelector.java
@@ -25,7 +25,8 @@ public SortingMoveSelector(MoveSelector<Solution_> childMoveSelector, SelectionC
     @Override
     public void constructCache(SolverScope<Solution_> solverScope) {
         super.constructCache(solverScope);
-        sorter.sort(solverScope.getScoreDirector().getWorkingSolution(), cachedMoveList);
+        // We need to update the cachedMoveList since the sorter will copy the data and return a sorted list
+        cachedMoveList = sorter.sort(solverScope.getScoreDirector().getWorkingSolution(), cachedMoveList);
         logger.trace("    Sorted cachedMoveList: size ({}), moveSelector ({}).",
                 cachedMoveList.size(), this);
     }
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/value/decorator/FromEntitySortingValueSelector.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/value/decorator/FromEntitySortingValueSelector.java
@@ -98,7 +98,8 @@ public Iterator<Object> iterator(Object entity) {
         childValueSelector.iterator(entity).forEachRemaining(cachedValueList::add);
         logger.trace("    Created cachedValueList: size ({}), valueSelector ({}).",
                 cachedValueList.size(), this);
-        sorter.sort(scoreDirector.getWorkingSolution(), cachedValueList);
+        // We need to update the cachedValueList since the sorter will copy the data and return a sorted list
+        cachedValueList = sorter.sort(scoreDirector.getWorkingSolution(), cachedValueList);
         logger.trace("    Sorted cachedValueList: size ({}), valueSelector ({}).",
                 cachedValueList.size(), this);
         return cachedValueList.iterator();
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/value/decorator/SortingValueSelector.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/value/decorator/SortingValueSelector.java
@@ -27,7 +27,8 @@ public SortingValueSelector(IterableValueSelector<Solution_> childValueSelector,
     @Override
     public void constructCache(SolverScope<Solution_> solverScope) {
         super.constructCache(solverScope);
-        sorter.sort(solverScope.getScoreDirector().getWorkingSolution(), cachedValueList);
+        // We need to update the cachedValueList since the sorter will copy the data and return a sorted list
+        cachedValueList = sorter.sort(solverScope.getScoreDirector().getWorkingSolution(), cachedValueList);
         logger.trace("    Sorted cachedValueList: size ({}), valueSelector ({}).",
                 cachedValueList.size(), this);
     }
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/score/director/ValueRangeManager.java b/core/src/main/java/ai/timefold/solver/core/impl/score/director/ValueRangeManager.java
@@ -22,6 +22,7 @@
 import ai.timefold.solver.core.impl.domain.valuerange.buildin.primdouble.DoubleValueRange;
 import ai.timefold.solver.core.impl.domain.valuerange.descriptor.ValueRangeDescriptor;
 import ai.timefold.solver.core.impl.domain.valuerange.sort.SelectionSorterAdapter;
+import ai.timefold.solver.core.impl.domain.valuerange.sort.SortableValueRange;
 import ai.timefold.solver.core.impl.domain.variable.descriptor.BasicVariableDescriptor;
 import ai.timefold.solver.core.impl.domain.variable.descriptor.GenuineVariableDescriptor;
 import ai.timefold.solver.core.impl.domain.variable.descriptor.ListVariableDescriptor;
@@ -373,6 +374,7 @@ public <T> CountableValueRange<T> getFromSolution(ValueRangeDescriptor<Solution_
         return getFromSolution(valueRangeDescriptor, solution, null);
     }
 
+    @SuppressWarnings({ "rawtypes", "unchecked" })
     public <T> CountableValueRange<T> getFromSolution(ValueRangeDescriptor<Solution_> valueRangeDescriptor, Solution_ solution,
             @Nullable SelectionSorter<Solution_, T> sorter) {
         var item = fromSolution[valueRangeDescriptor.getOrdinal()];
@@ -395,8 +397,13 @@ public <T> CountableValueRange<T> getFromSolution(ValueRangeDescriptor<Solution_
                 valueRange = countableValueRange;
             }
             if (sorter != null) {
+                if (!(valueRange instanceof SortableValueRange sortableValueRange)) {
+                    throw new IllegalStateException(
+                            "Impossible state: value range (%s) on planning solution (%s) is not sortable."
+                                    .formatted(valueRangeDescriptor, solution));
+                }
                 var sorterAdapter = SelectionSorterAdapter.of(solution, sorter);
-                valueRange = (CountableValueRange<T>) valueRange.sort(sorterAdapter);
+                valueRange = (CountableValueRange<T>) sortableValueRange.sort(sorterAdapter);
             }
             fromSolution[valueRangeDescriptor.getOrdinal()] = new CountableValueRangeItem<>(valueRange, sorter);
         }
@@ -414,7 +421,7 @@ public <T> CountableValueRange<T> getFromEntity(ValueRangeDescriptor<Solution_>
     /**
      * @throws IllegalStateException if called before {@link #reset(Object)} is called
      */
-    @SuppressWarnings("unchecked")
+    @SuppressWarnings({ "unchecked", "rawtypes" })
     public <T> CountableValueRange<T> getFromEntity(ValueRangeDescriptor<Solution_> valueRangeDescriptor, Object entity,
             @Nullable SelectionSorter<Solution_, T> sorter) {
         if (cachedWorkingSolution == null) {
@@ -445,8 +452,13 @@ public <T> CountableValueRange<T> getFromEntity(ValueRangeDescriptor<Solution_>
                 valueRange = countableValueRange;
             }
             if (sorter != null) {
+                if (!(valueRange instanceof SortableValueRange sortableValueRange)) {
+                    throw new IllegalStateException(
+                            "Impossible state: value range (%s) on planning entity (%s) is not sortable."
+                                    .formatted(valueRangeDescriptor, entity));
+                }
                 var sorterAdapter = SelectionSorterAdapter.of(cachedWorkingSolution, sorter);
-                valueRange = (CountableValueRange<T>) valueRange.sort(sorterAdapter);
+                valueRange = (CountableValueRange<T>) sortableValueRange.sort(sorterAdapter);
             }
             valueRangeList[valueRangeDescriptor.getOrdinal()] = new CountableValueRangeItem<>(valueRange, sorter);
         }
diff --git a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/common/CodeAssertableSorter.java b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/common/CodeAssertableSorter.java
@@ -0,0 +1,29 @@
+package ai.timefold.solver.core.impl.heuristic.selector.common;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
+
+import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionSorter;
+import ai.timefold.solver.core.testutil.CodeAssertable;
+
+public class CodeAssertableSorter<T extends CodeAssertable> implements SelectionSorter<Object, T> {
+
+    @Override
+    public List<T> sort(Object solution, List<T> selectionList) {
+        var sortedList = new ArrayList<>(selectionList);
+        Collections.sort(sortedList, Comparator.comparing(CodeAssertable::getCode));
+        return sortedList;
+    }
+
+    @Override
+    public SortedSet<T> sort(Object solution, Set<T> selectionSet) {
+        var sortedSet = new TreeSet<T>(Comparator.comparing(CodeAssertable::getCode));
+        sortedSet.addAll(selectionSet);
+        return sortedSet;
+    }
+}
diff --git a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/common/TestdataObjectSorter.java b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/common/TestdataObjectSorter.java
@@ -0,0 +1,29 @@
+package ai.timefold.solver.core.impl.heuristic.selector.common;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
+
+import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionSorter;
+import ai.timefold.solver.core.testdomain.TestdataObject;
+
+public class TestdataObjectSorter<T extends TestdataObject> implements SelectionSorter<Object, T> {
+
+    @Override
+    public List<T> sort(Object solution, List<T> selectionList) {
+        var sortedList = new ArrayList<>(selectionList);
+        Collections.sort(sortedList, Comparator.comparing(TestdataObject::getCode));
+        return sortedList;
+    }
+
+    @Override
+    public SortedSet<T> sort(Object solution, Set<T> selectionSet) {
+        var sortedSet = new TreeSet<T>(Comparator.comparing(TestdataObject::getCode));
+        sortedSet.addAll(selectionSet);
+        return sortedSet;
+    }
+}
diff --git a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/ComparatorFactorySelectionSorterTest.java b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/ComparatorFactorySelectionSorterTest.java
diff --git a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/ComparatorSelectionSorterTest.java b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/common/decorator/ComparatorSelectionSorterTest.java
diff --git a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/SortingEntitySelectorTest.java b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/SortingEntitySelectorTest.java
diff --git a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/decorator/SortingMoveSelectorTest.java b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/decorator/SortingMoveSelectorTest.java
diff --git a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/value/decorator/SortingValueSelectorTest.java b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/value/decorator/SortingValueSelectorTest.java

Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,8 @@ public void constructCache(SolverScope<Solution_> solverScope) {`
`60`	`60`	`return;`
`61`	`61`	`}`
`62`	`62`	`super.constructCache(solverScope);`
`63`		`- sorter.sort(solverScope.getScoreDirector().getWorkingSolution(), cachedEntityList);`
	`63`	`+ // We need to update the cachedEntityList since the sorter will copy the data and return a sorted list`
	`64`	`+ cachedEntityList = sorter.sort(solverScope.getScoreDirector().getWorkingSolution(), cachedEntityList);`
`64`	`65`	`logger.trace(" Sorted cachedEntityList: size ({}), entitySelector ({}).",`
`65`	`66`	`cachedEntityList.size(), this);`
`66`	`67`	`}`