chore: return null when the solution is invalid in calculate score - #2569
chore: return null when the solution is invalid in calculate score#2569Christopher-Chianelli wants to merge 1 commit into
Conversation
| logger.trace("{} Move index ({}), score ({}), move ({}).", | ||
| logIndentation, moveScope.getMoveIndex(), moveScope.getScore().raw(), moveScope.getMove()); | ||
| logIndentation, moveScope.getMoveIndex(), | ||
| (score != null) ? moveScope.getScore().raw() : "invalid", moveScope.getMove()); |
There was a problem hiding this comment.
(moveScope.getScore() != null) ? moveScope.getScore().raw() : "invalid", moveScope.getMove());
| return isConsistentSolutionAccepted(moveScope); | ||
| } | ||
|
|
||
| public abstract boolean isConsistentSolutionAccepted(LocalSearchMoveScope<Solution_> moveScope); |
There was a problem hiding this comment.
The public modifier may not be ideal, as this method can still be called instead of isAccepted. Can we change it to protected to prevent this?
There was a problem hiding this comment.
When I tried package-private, was getting compile errors since some implementations are in different packages. I can try protected though.
| if (lastVariableUpdateSuccessful) { | ||
| return innerCalculateScore(); | ||
| } else { | ||
| var invalidScore = InnerScore.invalid(getScoreDefinition().getZeroScore()); |
There was a problem hiding this comment.
Chris, I'm not convinced that the new solution is better than the old approach. I believe returning a non-null value is preferable. What is the primary reason for changing this behavior?
There was a problem hiding this comment.
@triceo wanted it, so any places where it needs to be checked become obvious. We might find a different way though.
zepfred
left a comment
zepfred
left a comment
There was a problem hiding this comment.
Chris, I have a few questions regarding the proposed approach.
2 participants
Note:
DefaultPhaseCommandContext.executeTemporarythrows if the score is null/invalid (viaObjects.requireNonNull(score, () -> "The move (%s) failed to calculate a score.".formatted(move));). Should that be change (i.e. what should happen when a user executes a temporary move that results in an inconsistent solution in a PhaseCommand).