• 1

• 1.3k
• 7
• 17

#Naming

QUEUEA, QUEUEB, DISTANCEA, DISTANCEB, PARENTSA, PARENTSB

Should be:

queueA, queueB, distanceA, distanceB, parentsA, parentsB

baseGetNeighbors is a strange name. I understand it's the 'base' method for getChildArticles/getParentArticles, but what is wrong with getNeighbors?

Also the reuse and the articulation between those three methods seem a bit naive to me, because tho base method only has three instructions, and only one is strictly identical: the other two use ternary operator... There is little profit in extracting this.

This is more straightforward, as it has no branching:

private static final String CHARSET_NAME = "UTF-8";
private static final Charset UTF8 = Charset.forName(CHARSET_NAME);
private static List<String> getChildArticles(String currentTitle) throws IOException {
 String jsonDataUrl = String.format(FORWARD_URL_FORMAT, URLEncoder.encode(currentTitle, CHARSET_NAME ));
 String jsonText = IOUtils.toString(new URL(jsonDataUrl), UTF8);
 return extractForwardLinkTitles(jsonText);
}
private static List<String> getParentArticles(String current) throws IOException {
 String jsonDataUrl = String.format(BACKWARD_URL_FORMAT, URLEncoder.encode(currentTitle, CHARSET_NAME));
 String jsonText = IOUtils.toString(new URL(jsonDataUrl), UTF8);
 return extractBackwardLinkTitles(jsonText);
}

Unconventional logging

You require a nullable PrintStream as input, for logging. This is a bad practice because you force the user to worry about it. Worse, you request it in a business method, which could be called in multiple places by the user, so you propagate the problem to those many places.

I you don't want a big re-write, one simple solution is to provide the PrintStream in a setter method. This way the user worries about it once (on creation), not at every usage.

Pathfinder` is a Helper in disguise

You have a bunch of static, and non-static method.

Try this: declare all methods static. No compile error? Then your entire class is not an Object, it's a merely Helper.

Why is it so? Your only two public methods are:

• findShortestPath
• The default Constructor

The former merely calls static methods on method-scope variables, it is non-mutating. The latter does nothing since the Object has no field. So constructing is a useless interaction forced down on your user. You have two choices:

• Make the whole class a true Helper (all methods static, private Constructor, final class). This gives minimal code change and is better than the existing, but I do not recommend it. Objects are useful!
• Make the class a true Object with a useful state.

I'll explore what goods the second option brings.

1. Remember the annoying logging? Make it part of the State, provide a Setter.
2. Provide iteration control. MAke iteration state part of Object state. Then you can provide iterate(timeout), iterate(iterationsLimit), iterateOnce(), etc.
3. Reuse the results. Say the user wants to graph a piece of wikipedia. He wants the distance between any two nodes. You're ditching all results after each computation, but after you do search(nodeA, nodeB), you hsould be able to do search(nodeA, nodeC) AND be able to reuse queueA, parentsA, distanceA, etc. If nodeC is already known you just return it. This is out of scope for now obviously, but it would make sense, and having a true Object model is enabling this possibility.

Separation of concerns

Your (single) class does:

• Exctract URLS from web pages
• Perform BBFS
• URL encoding/decoding

That is too much. Split it in several sub-classes. This will allow you later to swap the m with more pewerful functionalities (caching result, A* star - although I don't know with which heuristic ^^ - etc.).

Algorithm pitfall

Be careful about how you find the touchNode. There could be several touchNodes, and the first one encountered is not necessarily the best.

It is in your case, because all distances are equal to 1. In general in bidirectional search it is not the case, you could find a touchNode1 which links up both BFS with a cost of 2.5 + 3.5, and miss a touchNode2 which links us with a cost of 3.5 + 0.2 (which is better, but evaluated later because its first link is more expensive, so it was later in the QueueA).

You're fine, just keep this in mind in future implementations (e.g. if you want to make you bidirectional pathfinder generic).