Comments, javadoc and refactoring of the recent optimizations

Author: murfffi

src/main/java/zebra4j/AbstractPuzzleGenerator.java

@@ -87,13 +87,19 @@ public P generate() {
 	protected abstract P toPuzzle(List<Fact> facts);
 
 	/**
-	 * @param puzzle
-	 * @return number of unique solutions of the given puzzle
+	 * If the puzzle has a unique solution
+	 * 
+	 * @param puzzle required
+	 * @return false, if the puzzle has no or multiple solutions
 	 */
 	protected boolean uniqueSolution(P puzzle) {
 		return countSolutions(puzzle) == 1;
 	}
 
+	/**
+	 * @param puzzle required
+	 * @return number of unique solutions of the given puzzle
+	 */
 	protected abstract int countSolutions(P puzzle);
 
 	/**
@@ -109,6 +115,17 @@ protected boolean rejectFact(Fact fact) {
 		return false;
 	}
 
+	/**
+	 * Reduce the initial list of all compatible facts to the facts to use in the
+	 * generated puzzle
+	 * 
+	 * <p>
+	 * Implementation typically override either this method or
+	 * {@link #uniqueSolution} with a more efficient solution.
+	 * 
+	 * @param facts initial list of facts with which the puzzle has a unique
+	 *              solution
+	 */
 	protected void removeFacts(List<Fact> facts) {
 		for (int i = 0; i < facts.size(); ++i) {
 			List<Fact> factsCopy = new ArrayList<>(facts);

src/main/java/zebra4j/PuzzleSolver.java

@@ -36,6 +36,7 @@
 import org.chocosolver.solver.Settings;
 import org.chocosolver.solver.Solution;
 import org.chocosolver.solver.Solver;
+import org.chocosolver.solver.variables.IntVar;
 
 import lombok.AllArgsConstructor;
 import lombok.Getter;
@@ -59,10 +60,15 @@ public class PuzzleSolver {
 	private Settings chocoSettings;
 
 	/**
-	 * @param zebraModel
-	 * @return a list of choco-solver solutions; useful to count solutions
+	 * Solve the puzzle returning a lazy stream to all solutions in a raw form,
+	 * close to the underlying Choco solver model
+	 * 
+	 * @return a list of raw solutions - map from {@link Attribute} to person ID;
+	 *         useful when just counting solutions
 	 */
-	Stream<Map<Attribute, Integer>> solveChoco(ZebraModel zebraModel) {
+	Stream<Map<Attribute, Integer>> solveChoco() {
+		ZebraModel zebraModel = toModel();
+		log.debug("Solving puzzle {}", puzzle);
 		for (Fact fact : puzzle.getFacts()) {
 			fact.postTo(zebraModel);
 		}
@@ -78,27 +84,38 @@ public boolean tryAdvance(Consumer<? super Map<Attribute, Integer>> action) {
 					return false;
 				}
 				solution.record();
-				action.accept(zebraModel.getVariableMap().entrySet().stream()
+				// We extract the values of only the relevant variables. Returning a copy of the
+				// complete solution is not efficient because the solution contains much more
+				// internal variables and data.
+				// For question puzzles, this can be further improved by extracting only the
+				// attributes for towards and about, but the optimization is currently not worth
+				// the added complexity.
+				Set<Entry<Attribute, IntVar>> variables = zebraModel.getVariableMap().entrySet();
+				action.accept(variables.stream()
 						.collect(Collectors.toMap(e -> e.getKey(), e -> solution.getIntVal(e.getValue()))));
 				return true;
 			}
 		};
 		return StreamSupport.stream(spliterator, false);
 	}
 
+	/**
+	 * Solves the puzzles eagerly.
+	 * 
+	 * @return a list of all distinct solution.
+	 */
 	public List<PuzzleSolution> solve() {
-		log.debug("Solving puzzle {}", puzzle);
 		List<PuzzleSolution> zebraSolutions = solveToStream().distinct().collect(Collectors.toList());
 		log.trace("Found {} distinct solutions", zebraSolutions.size());
 		return zebraSolutions;
 	}
 
+	/**
+	 * @return a lazy stream of all solutions with possible duplicates
+	 */
 	public Stream<PuzzleSolution> solveToStream() {
-		ZebraModel zebraModel = toModel();
-		Stream<Map<Attribute, Integer>> solutions = solveChoco(zebraModel);
-		// 16% of runtime is here
-		Stream<PuzzleSolution> stream = solutions.map(choco -> fromChocoSolution(choco));
-		return stream;
+		// 16% of the typical CPU cost of this method is in the map.
+		return solveChoco().map(this::fromChocoSolution);
 	}
 
 	private PuzzleSolution fromChocoSolution(Map<Attribute, Integer> choco) {
@@ -114,7 +131,7 @@ private PuzzleSolution fromChocoSolution(Map<Attribute, Integer> choco) {
 		return builder.build();
 	}
 
-	ZebraModel toModel() {
+	private ZebraModel toModel() {
 		ZebraModel model = new ZebraModel(chocoSettings);
 		for (Entry<AttributeType, Set<Attribute>> entry : puzzle.getAttributeSets().entrySet()) {
 			entry.getKey().addToModel(model, entry.getValue(), puzzle.numPeople());

src/main/java/zebra4j/Question.java

@@ -40,6 +40,10 @@
  * The question is towards the person, identified by the attribute in
  * {@link #getTowards()} and is about the attribute of type {@link #getAbout()}.
  */
+/**
+ * @author mnozhchev
+ *
+ */
 @Value
 public class Question {
 	public static Question NAME_OF_CRIMINAL = new Question(Criminal.YES, PersonName.TYPE);
@@ -112,10 +116,17 @@ public Optional<Attribute> answer(PuzzleSolution solution) {
 		return solution.findPerson(towards).flatMap(person -> Optional.ofNullable(person.findAttribute(about)));
 	}
 
-	public Optional<Attribute> answer(Map<Attribute, Integer> chocoSolution, Puzzle puzzle) {
-		int personId = chocoSolution.get(towards);
+	/**
+	 * Answers this question about the given solution, if applicable
+	 * 
+	 * @param rawSolution solution, as in {@link PuzzleSolver#solveChoco()}
+	 * @param puzzle      the puzzle that the solution solves, used for metadata
+	 * @return same as {@link #answer(PuzzleSolution)}
+	 */
+	public Optional<Attribute> answer(Map<Attribute, Integer> rawSolution, Puzzle puzzle) {
+		int personId = rawSolution.get(towards);
 		Set<Attribute> attributes = puzzle.getAttributeSets().get(about);
-		return attributes.stream().filter(attr -> chocoSolution.get(attr) == personId).findAny();
+		return attributes.stream().filter(attr -> rawSolution.get(attr) == personId).findAny();
 	}
 
 	public boolean appliesTo(Puzzle puzzle) {

src/main/java/zebra4j/QuestionPuzzleSolver.java

@@ -56,8 +56,7 @@ public Set<Attribute> solve() {
 	}
 
 	public Stream<Attribute> solveToStream() {
-		ZebraModel model = solver.toModel();
-		return solver.solveChoco(model)
+		return solver.solveChoco()
 				.flatMap(solution -> toStream(qPuzzle.getQuestion().answer(solution, qPuzzle.getPuzzle())));
 	}
 

src/test/java/zebra4j/PerfIT.java

@@ -23,8 +23,6 @@
 
 import static org.junit.Assert.assertEquals;
 
-import java.util.Locale;
-
 import org.junit.Test;
 
 import zebra4j.Cli.QSample;
@@ -34,7 +32,6 @@ public class PerfIT {
 	@Test
 	public void testQuestion() throws Exception {
 		QSample sample = Cli.sampleQuestionPuzzle(1, 5);
-		Cli.printQuestionPuzzle(sample, Locale.getDefault(), System.out);
 		assertEquals(3, sample.getPuzzle().getPuzzle().getFacts().size());
 	}