4
\$\begingroup\$

Intro

I am currently working on this project (PathFinding.java). This time, I need to get the following class reviewed:

Code

io.github.coderodde.pathfinding.finders.BeamSearchFinder.java:

package io.github.coderodde.pathfinding.finders;
import static io.github.coderodde.pathfinding.finders.Finder.searchSleep;
import io.github.coderodde.pathfinding.heuristics.HeuristicFunction;
import io.github.coderodde.pathfinding.logic.GridCellNeighbourIterable;
import io.github.coderodde.pathfinding.logic.PathfindingSettings;
import io.github.coderodde.pathfinding.logic.SearchState;
import io.github.coderodde.pathfinding.model.GridModel;
import io.github.coderodde.pathfinding.utils.Cell;
import io.github.coderodde.pathfinding.utils.CellType;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
/**
 *
 * @author Rodion "rodde" Efremov
 * @version 1.0.0 (Sep 5, 2025)
 * @since 1.0.0 (Sep 5, 2025)
 */
public final class BeamSearchFinder implements Finder {
 @Override
 public List<Cell> findPath(GridModel model,
 GridCellNeighbourIterable neighbourIterable, 
 PathfindingSettings pathfindingSettings, 
 SearchState searchState) {
 Map<Cell, Cell> parents = new HashMap<>();
 Deque<Cell> queue = new ArrayDeque<>();
 
 Cell source = model.getSourceGridCell();
 Cell target = model.getTargetGridCell();
 
 parents.put(source, null);
 queue.addLast(source);
 
 while (!queue.isEmpty()) {
 if (searchState.haltRequested()) {
 return List.of();
 }
 
 if (searchState.pauseRequested()) {
 searchSleep(pathfindingSettings);
 continue;
 }
 
 if (queue.size() > pathfindingSettings.getBeamWidth()) {
 List<Cell> layer = new ArrayList<>(queue);
 HeuristicFunction hf = 
 pathfindingSettings.getHeuristicFunction();
 
 layer.sort((a, b) -> {
 return Double.compare(hf.estimate(a, target),
 hf.estimate(b, target));
 });
 
 for (Cell cell : queue) {
 model.setCellType(cell, CellType.FREE);
 }
 
 queue.clear();
 queue.addAll(
 layer.subList(
 0,
 Math.min(layer.size(),
 pathfindingSettings.getBeamWidth())));
 
 for (Cell cell : layer) {
 model.setCellType(cell, CellType.OPENED);
 }
 }
 
 Cell current = queue.removeFirst();
 
 if (current.equals(target)) {
 return tracebackPath(target, parents);
 }
 
 if (!current.equals(source)) {
 model.setCellType(current, CellType.VISITED);
 }
 
 neighbourIterable.setStartingCell(current);
 
 for (Cell neighbour : neighbourIterable) {
 if (searchState.haltRequested()) {
 return List.of();
 }
 
 while (searchState.pauseRequested()) {
 searchSleep(pathfindingSettings);
 }
 
 searchSleep(pathfindingSettings);
 
 if (parents.containsKey(neighbour)) {
 continue;
 }
 
 if (!neighbour.equals(target)) {
 model.setCellType(neighbour, CellType.OPENED);
 }
 
 parents.put(neighbour, current);
 queue.addLast(neighbour);
 }
 }
 
 return List.of();
 }
}

Possible output

Beam search found an optimal path

Warning

If you decide to clone or fork the above repository, note that it will require some tweaking with Maven: in order to run the app, go to the root directory of the repository and run mvn clean javafx:run. Also, after the first search run, the UI may fail.

Critique request

I am eager to hear any constructive commentary. However, my first concern is whether I got the algorithm right. Especially, I am wondering whether the following part

if (queue.size() > pathfindingSettings.getBeamWidth()) {
 List<Cell> layer = new ArrayList<>(queue);
 HeuristicFunction hf = 
 pathfindingSettings.getHeuristicFunction();
 
 layer.sort((a, b) -> {
 return Double.compare(hf.estimate(a, target),
 hf.estimate(b, target));
 });
 
 for (Cell cell : queue) {
 model.setCellType(cell, CellType.FREE);
 }
 
 queue.clear();
 queue.addAll(
 layer.subList(0,
 Math.min(layer.size(),
 pathfindingSettings.getBeamWidth())));
 
 for (Cell cell : layer) {
 model.setCellType(cell, CellType.OPENED);
 }
}
 

can be optimized efficiency-wise.

asked Sep 5 at 14:56
\$\endgroup\$
1
  • \$\begingroup\$ (I'd appreciate an example where the path found by beam search is not the shortest one.) \$\endgroup\$ Commented 2 days ago

1 Answer 1

4
\$\begingroup\$

Naming:
Map<Cell, Cell> parents fine
Deque<Cell> queue 2nd rate: reminds me of what I can do with it, instead of why what's in it is there - toDo? open? opened?

Seeing FIFO does not get used with queue/open, make it a List<Cell> and avoid moving to layer and fro.

Regarding the snippet, one obviously small improvement would be to set all of layer.subList(beamWidth, layer.size()) to FREE, and all of the newly populated queue to OPENED.
(Is it even intended to set all of layer to OPENED? Wouldn't that undo all the setting to FREE?)

To add to user293216's idea to use an ordered Set to avoid ordering over&again:
Implement a size-limited MultiPriorityQueue.
This should keep the number of comparisons low and highlight the relation to Dijkstra's Shortest Path: size limit vs. none, evaluating all neighbours vs. always the highest priority node.

I don't see CellType used (assuming use in visualisation).

answered yesterday
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.