(Personal note: Good work, this is an excellent resource. Can I bookmark/reuse?)
##What is this for?
protected AbstractPathfinder() {
this.graph = null;
this.weightFunction = null; // Compiler requires this initialization.
}
This is unused if I'm not mistaken, and is dangerous. It needs to go.
##Richer return values
Right now your Pathfinders produce a List<Integer> as a path. If there is no path, the list is empty.
Consider using a dedicated Path object to allow the user to poll the result status:
public interface Path {
public static enum PathState {
SUCCESS, ERROR, NO_PATH, UNFINISHED
}
PathStatus getStatus();
List<Integer> getPath();
List<Point2D.Double> get2dPath(DirectedGraphNodeCoordinates coords); // Maybe?
double getPathCost(); // This is severely lacking
}
##Why expose a Stateful Pathfinder?
The Pathfinder instances have a state, that they keep from one search call to the next. Is that useful? Is the remainder state re-used for subsequent calls to search?
- It could be reused for Dijkstra, keeping the same start cell but aiming for another end cell. But it's not possible, because you've made Dijkstra a special case of AStar...
- It might be used for AStar, but if the goal has changed, so has the heuristic, and the whole computation might become obsolete, I'm nto too sure.
- It's probably very hard to reuse the NBAStarPathfinder for the above reasons.
So, why expose a stateful Pathfinder instance? I would rather limit the Pathfinder to having a DirectedGraph and DirectedGraphWeightFunction like it does (so that it can be used for several paths on the same graph), but not have computation-related states, like each implementation has (CLOSED and OPEN lists etc).
In summary, the pathfinder's search method should not have side-effects. This is not a design problem, but an implementation problem in AStarPathfinder, DijkstraPathfinder, and NBAStarPathfinder.
##Why not provide a more useful stateful Object?
It may still be useful to expose a stateful pathfinder for an other feature: computation control. You rightly decided to leave Multithreading out, but haven't given your users the tools to make it happen themselves.
We all know pathfinding quickly become a resource-hungry beast. Why not provide hooks to control your method's execution?
public abstract class AbstractPathfinder {
[blah, blah, blah, everything was fine here]
// Replace the search: provide a controller instead of straigh execution
public abstract PathfinderController search(int sourceNodeId, int targetNodeId);
[Blah, blah]
}
public interface PathfinderController {
boolean iterateOnce(); // Explores just ONE cell. Return true if succeeded
boolean iterate(long timeout); // Explores until the timeout. Return true if succeeded
Path finish(); // Finish the computation (blocking). Return the Path Object
PathStatus hasFinished(); // Return the current status
}
This way your user can pace the computation as they want. The Pathfinder is unchanged , but the PathfinderController holds the computation state and can deliver the final Path.
##The Trace back methods
Pathfinder's tracebackPath methods feels awkward.
Hint N°1: make it static, it still works. This marks it as a helper function.
Hint N°2: You have two versions, one for each pathfinder implementation. It might not help a RandomPathfinder, or a RightHandWallFollowerPathfinder. So take it out of there, and move it to the right class, as it is an implementation detail.
##Naming conventions
I would rename EuclideanHeuristicFunction as EuclidianDistance because that is more accurate.
I would rename DirectedGraphNodeCoordinates as NodeCoordinates because you don't need to know those coordinates apply to nodes from a graph, or that the graph has directed edges. All around, the DirectedGraph prefix is often superfluous.
DirecteGraph.addArc(tailNodeId, headNodeId) should be DirecteGraph.addArc(headNodeId, tailNodeId), I think
##Commenting is good
All is said. Although I did not follow NBAStar entirely, I must admit ;)
There are very tricky end conditions to bi directional AStar, I'll trust your implementation on this.
Careful though, is the HeuristicFunction estimation supposed to be symmetric : estimateDistanceBetween(nodeId1, nodeId2) == estimateDistanceBetween(node2Id, node1Id) ?
Because it is not specified, and in expandInBackwardDirection() you might obtain invalid heuristic values. But this is really tortured thinking anyway...
##Modularity
Is very good, you provide good interfaces.
However, when faced with i.e. implicit problems, that can have infinite states (most of which should never be explored), the user will have to instantiate most
states and arcs, and hope all the required ones are there...
It might be good to provide a:
public interface Node{
public List<Node> neighbours();
}
But this will go against the use of Integer as node Ids, because you cannot force the user to think with IDs...
This is a (minor) limitation of the current design.
##Overall
Good job, I've tried to be very mean, and I can see myself using this quite easily!