查找代码复习,优化和最佳实践。还需要帮助弄清楚复杂性,在我的最佳尝试中,这是O(E!),其中E是边的数量。
class GraphFindAllPaths<T> implements Iterable<T> {
/* A map from nodes in the graph to sets of outgoing edges. Each
* set of edges is represented by a map from edges to doubles.
*/
private final Map<T, Map<T, Double>> graph = new HashMap<T, Map<T, Double>>();
/**
* Adds a new node to the graph. If the node already exists then its a
* no-op.
*
* @param node Adds to a graph. If node is null then this is a no-op.
* @return true if node is added, false otherwise.
*/
public boolean addNode(T node) {
if (node == null) {
throw new NullPointerException("The input node cannot be null.");
}
if (graph.containsKey(node)) return false;
graph.put(node, new HashMap<T, Double>());
return true;
}
/**
* Given the source and destination node it would add an arc from source
* to destination node. If an arc already exists then the value would be
* updated the new value.
*
* @param source the source node.
* @param destination the destination node.
* @param length if length if
* @throws NullPointerException if source or destination is null.
* @throws NoSuchElementException if either source of destination does not exists.
*/
public void addEdge (T source, T destination, double length) {
if (source == null || destination == null) {
throw new NullPointerException("Source and Destination, both should be non-null.");
}
if (!graph.containsKey(source) || !graph.containsKey(destination)) {
throw new NoSuchElementException("Source and Destination, both should be part of graph");
}
/* A node would always be added so no point returning true or false */
graph.get(source).put(destination, length);
}
/**
* Removes an edge from the graph.
*
* @param source If the source node.
* @param destination If the destination node.
* @throws NullPointerException if either source or destination specified is null
* @throws NoSuchElementException if graph does not contain either source or destination
*/
public void removeEdge (T source, T destination) {
if (source == null || destination == null) {
throw new NullPointerException("Source and Destination, both should be non-null.");
}
if (!graph.containsKey(source) || !graph.containsKey(destination)) {
throw new NoSuchElementException("Source and Destination, both should be part of graph");
}
graph.get(source).remove(destination);
}
/**
* Given a node, returns the edges going outward that node,
* as an immutable map.
*
* @param node The node whose edges should be queried.
* @return An immutable view of the edges leaving that node.
* @throws NullPointerException If input node is null.
* @throws NoSuchElementException If node is not in graph.
*/
public Map<T, Double> edgesFrom(T node) {
if (node == null) {
throw new NullPointerException("The node should not be null.");
}
Map<T, Double> edges = graph.get(node);
if (edges == null) {
throw new NoSuchElementException("Source node does not exist.");
}
return Collections.unmodifiableMap(edges);
}
/**
* Returns the iterator that travels the nodes of a graph.
*
* @return an iterator that travels the nodes of a graph.
*/
@Override public Iterator<T> iterator() {
return graph.keySet().iterator();
}
}
/**
* Given a connected directed graph, find all paths between any two input points.
*/
public class FindAllPaths<T> {
private final GraphFindAllPaths<T> graph;
/**
* Takes in a graph. This graph should not be changed by the client
*/
public FindAllPaths(GraphFindAllPaths<T> graph) {
if (graph == null) {
throw new NullPointerException("The input graph cannot be null.");
}
this.graph = graph;
}
private void validate (T source, T destination) {
if (source == null) {
throw new NullPointerException("The source: " + source + " cannot be null.");
}
if (destination == null) {
throw new NullPointerException("The destination: " + destination + " cannot be null.");
}
if (source.equals(destination)) {
throw new IllegalArgumentException("The source and destination: " + source + " cannot be the same.");
}
}
/**
* Returns the list of paths, where path itself is a list of nodes.
*
* @param source the source node
* @param destination the destination node
* @return List of all paths
*/
public List<List<T>> getAllPaths(T source, T destination) {
validate(source, destination);
List<List<T>> paths = new ArrayList<List<T>>();
recursive(source, destination, paths, new LinkedHashSet<T>());
return paths;
}
// so far this dude ignore's cycles.
private void recursive (T current, T destination, List<List<T>> paths, LinkedHashSet<T> path) {
path.add(current);
if (current == destination) {
paths.add(new ArrayList<T>(path));
path.remove(current);
return;
}
final Set<T> edges = graph.edgesFrom(current).keySet();
for (T t : edges) {
if (!path.contains(t)) {
recursive (t, destination, paths, path);
}
}
path.remove(current);
}
public static void main(String[] args) {
GraphFindAllPaths<String> graphFindAllPaths = new GraphFindAllPaths<String>();
graphFindAllPaths.addNode("A");
graphFindAllPaths.addNode("B");
graphFindAllPaths.addNode("C");
graphFindAllPaths.addNode("D");
graphFindAllPaths.addEdge("A", "B", 10);
graphFindAllPaths.addEdge("A", "C", 10);
graphFindAllPaths.addEdge("B", "D", 10);
graphFindAllPaths.addEdge("C", "D", 10);
graphFindAllPaths.addEdge("B", "C", 10);
graphFindAllPaths.addEdge("C", "B", 10);
FindAllPaths<String> findAllPaths = new FindAllPaths<String>(graphFindAllPaths);
List<List<String>> paths = new ArrayList<List<String>>();
// ABD
List<String> path1 = new ArrayList<String>();
path1.add("A"); path1.add("B"); path1.add("D");
// ABCD
List<String> path2 = new ArrayList<String>();
path2.add("A"); path2.add("B"); path2.add("C"); path2.add("D");
//ACD
List<String> path3 = new ArrayList<String>();
path3.add("A"); path3.add("C"); path3.add("D");
//ABCD
List<String> path4 = new ArrayList<String>();
path4.add("A"); path4.add("C"); path4.add("B"); path4.add("D");
paths.add(path1);
paths.add(path2);
paths.add(path3);
paths.add(path4);
findAllPaths.getAllPaths("A", "D");
assertEquals(paths, findAllPaths.getAllPaths("A", "D"));
}
}
#1 楼
这是美丽,易于阅读且文档齐全的代码。阅读此代码很高兴,而Java方面没有什么改进。该代码中显示的最大弱点不是您对Java的熟练程度(远远超过了我的能力),而是您对英语的了解。
在JavaDoc
addEdge
中,您谈论的是弧而不是边。可以是“源和目标都应为非空”或“源和目标都应为非空”。该方法还具有非常令人困惑的JavaDoc“如果长度为if,则为@param长度” –我不是确定这是什么意思。
但是让我们继续讲技术方面。
recursive
方法中有一个不错的错误:您正在通过T
比较==
实例:if (current == destination) {
但是,这默认为比较指针。它仅在这里起作用,因为常量字符串
"A"
,"B"
等由Java实现池合并。除非比较身份是您真正想要的,否则请进行比较:current.equals(destination)
。关于
recursive
将忽略循环的评论是错误的–它只会构造每个节点最多访问一次的路径。这是处理循环的唯一正确方法,否则循环图中将存在无数的路径。来自
validate
的错误消息没有任何意义。if (source == null) {
throw new NullPointerException("The source: " + source + " cannot be null.");
因此,我们将收到错误消息“源:null不能为null”。这没有帮助,我建议:
if (source == null) {
throw new NullPointerException("The \"source\" parameter cannot be null.");
在您的测试代码中,您经历了太多麻烦而无法产生预期的
paths
。我非常喜欢Arrays.asList(…)
:List<List<String>> paths = Arrays.asList(
Arrays.asList("A", "B", "D"),
Arrays.asList("A", "B", "C", "D"),
Arrays.asList("A", "C", "D"),
Arrays.asList("A", "C", "B", "D")
);
该代码简洁且具有自我说明性。除了将路径指定为代码外,还将路径放入注释中,当注释在
ABCD
路径上方的ACBD
处出现时,这是错误的; 。路径的顺序应该被认为是无关紧要的(您实际上对路径集合感兴趣),并且未指定,因为使用HashMap
不能保证密钥的保留顺序。实际上,您正在遍历keySet()
,它将确定路径的顺序!更好的相等性测试可能是:assertEquals(new HashSet<List<String>>(paths), new HashSet<List<String>>(result));
评论
\ $ \ begingroup \ $
路径是LinkedHashSet
\ $ \ endgroup \ $
– JavaDeveloper
2014年3月29日18:40
\ $ \ begingroup \ $
@JavaDeveloper对此感到抱歉,我现在删除了该段。不知道我在想什么
\ $ \ endgroup \ $
–阿蒙
2014年3月29日18:50
\ $ \ begingroup \ $
很高兴我编码,因为您会给我反馈,进一步肯定了我的信心
\ $ \ endgroup \ $
– JavaDeveloper
2014年3月30日在5:53
\ $ \ begingroup \ $
在平面设计评审提案FYI中引用。
\ $ \ endgroup \ $
–rolfl
2014年6月16日下午16:46
#2 楼
只是一些小事:这里的注释不正确,应该是
ACBD
://ABCD
List<String> path4 = new ArrayList<String>();
path4.add("A"); path4.add("C"); path4.add("B"); path4.add("D");
您可以使用Guava的
newArrayList
(或编写类似的方法)从测试代码中删除一些重复项:List<String> path4 = newArrayList("A", "C", "B", "D");
此处无需打个电话:
findAllPaths.getAllPaths("A", "D");
assertEquals(paths, findAllPaths.getAllPaths("A", "D"));
为此:
if (graph.containsKey(node)) return false;
根据Java编程语言的代码约定,
if语句始终使用大括号{}。
容易出错。我发现上面的单行代码很难阅读,因为如果逐行扫描代码,很容易会漏掉一行的结尾处有一个
return
语句。我将
validate
重命名为validateNotSame
,paths
重命名为expectedPaths
,以便更清晰。
/>
GraphFindAllPaths
可能只是Graph
。摘自Clean Code,第25页:
类和对象应具有名词或名词短语名称,例如
Customer
,WikiPage
,Account
和AddressParser
。 [...]类名不应是动词。例如,考虑使用AllPathFinder
而不是FindAllPaths
。您可能想要在此处创建防御性副本:
public FindAllPaths(GraphFindAllPaths<T> graph) {
if (graph == null) {
throw new NullPointerException("The input graph cannot be null.");
}
this.graph = graph;
}
它会禁止恶意客户端修改类的内部数据,并可能为您节省一个几个小时的调试。 (有效的Java,第2版,第39项:在需要时进行防御性复制)
未使用或未测试
Iterable
功能和removeEdge
方法。您也许可以删除它们。测试方法通常在
Test
文件(例如,FindAllPathsTest)中,并由JUnit运行。它们通常位于单独的源文件夹中,该文件夹不使junit.jar
(和其他测试依赖项)与生产代码一起打包。 Maven的目录结构是众所周知的项目结构,请考虑使用它(以及Maven,Gradle或任何其他构建工具)。评论
\ $ \ begingroup \ $
我确实同意您的命名约定(例如Graph),并为单元测试提供了单独的文件,但是对我个人而言,将它们全部合并到一个文件中对我来说更方便。虽然我同意他们的观点
\ $ \ endgroup \ $
– JavaDeveloper
2014年3月29日在18:41
\ $ \ begingroup \ $
您可能想在此处创建一个防御性副本-我也同意这一点,但是创建图的深层副本并不简单。
\ $ \ endgroup \ $
– JavaDeveloper
2014年3月30日下午5:55
#3 楼
这确实是一个有趣的问题。首先,我想提到恕我直言,此任务没有多项式解决方案,因为它不是优化问题。所需的结果不是某物的最大/最小,而是所有可能路径的枚举。我认为所有可能的路径都可能导致n!完整图中的不同路径,其中n是节点数。因此,找到解决方案取决于输入的大小。如果输入上有大图,则使用此算法将花费大量时间和内存,并且可能无法完成。我不知道此解决方案的确切用法,但我想它是一种地图/导航应用程序。如果您不介意,我会标记一些有用的观点并分享我的考虑事项:我看不到您在哪里使用double值,该值在每个边上都有。我想它将用于计算路径成本并返回按此条件排序的路径。如果是这样,考虑每次使用一些流行的算法来解决最短路径问题时,仅一次找到一小部分路径
如果输入中有稀疏图,则将导致每个节点使用Map在许多重物中,只具有一些价值。这将不必要地增加内存消耗。我不知道用例,将使用哪种类型的节点(T),但我建议使用从int到T的单个映射,并使用整数作为节点号。然后,您可以使用double [] []矩阵保存边缘值,并使用整数作为矩阵中的索引。示例:
0->“ A”
1->“ B”
2->“ C”
然后,将像这样显示边“ A” --4.2->“ C”:matrix [0] [2] = 4.2。同样,这将导致矩阵中许多未使用的单元。甚至更好的解决方案将是使用单个阵列/列表保存每个节点的邻居阵列/列表。实际上,您正在遍历邻居,因此不需要HashMap,只需持有一个键-值对列表就足够了。我的意思是:
List<List<Double>> nodes
代表图,而node.get(i)是另一个包含对的列表像(nodeNumber,edgeCost)。使用前面的示例:如果使用ArrayList,请考虑提供较小的初始容量,因为我认为默认值为16。 希望您能找到解决该问题的可行方法:)。
评论
\ $ \ begingroup \ $
我非常不同意你的第二点。在这种情况下,使用Map
- >想法似乎是使用Maps的一种特殊方法,但缺点是它也不是稀疏的(实际上与矩阵表示形式完全相同)。与矩阵表示相比,使用地图的优势在于,获取出站边集不必是O(n)操作。
\ $ \ endgroup \ $
–阿蒙
2014年3月29日10:29
\ $ \ begingroup \ $
如果您使用LinkedList,则List
- >不会有空单元格,但不会使用自扩展ArrayList。我同意您的观点,即使用map将使O(1)访问边缘,但是正如我所见,该算法会遍历所有邻居,并且不会通过键访问它们。我对使用HashMap的担心是它的默认加载因子为0.75,而最好的情况下,您将仅使用分配的内存的0.75%。达到负载系数时,它将自行调整大小(可能加倍)。实际上,您对使用地图有个很好的了解。这里的生产力取决于Map和List的实现。
\ $ \ endgroup \ $
–egelev
2014年3月29日上午11:05
\ $ \ begingroup \ $
如果使用List
- >,则double属于哪个边的唯一编码是double在列表中的位置。无论是否存在这种边缘,都必须表示每个可能的边缘。
\ $ \ endgroup \ $
– user2357112支持Monica
2014年3月29日在11:36
评论
您可能应该指定是否假设没有循环。这不是一个小细节。它处理周期
您的解决方案几乎是完美的。但是,您应该在递归方法中使用current.equals(destination)。