Clone of an undirected graph

Perform a graph clone. Verifying complexity to be O(E). Looking for code review, optimizations and best practices.

```
class NodeOfGraph {
private final T item;

NodeOfGraph(T item) {
this.item = item;
}

public T getItem() {
return item;
}
}

public class GraphWithCloneFunctionality implements Iterable> {

/*
* A map from nodes in the graph to list of outgoing edges.
*/
private final Map, Map, Double>> graph;

public GraphWithCloneFunctionality() {
graph = new HashMap, Map, Double>>();
}

public GraphWithCloneFunctionality(Map, Map, Double>> graph) {
if (graph == null) {
throw new NullPointerException("The graph should not be null");
}
this.graph = graph;
}

/**
* 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(NodeOfGraph node) {
if (node == null) {
throw new NullPointerException("The input node cannot be null.");
}
graph.put(node, new HashMap, Double>());
return true;
}

/**
* Given the two nodes it would add an arc from source
* to destination node.
*
* @param node1 the node1 node.
* @param node2 the node2 node.
* @param length if length if string
* @throws NullPointerException if node1 or nod2 is null.
* @throws NoSuchElementException if either node1 or node2 does not exists.
*/
public void addEdge (NodeOfGraph node1, NodeOfGraph node2, double length) {
if (node1 == null || node2 == null) {
throw new NullPointerException("node1 and node2, both should be non-null.");
}
if (!graph.containsKey(node1) || !graph.containsKey

Clone is an ugly API. Josh Bloch has a lot to say about it.

My opinion on it, is that you should use it as a last resort, and, when you do use it, it should be a short-cut to a public copy-constructor.

In other words, your clone method should look like:

public GraphWithCloneFunctionality clone() {
    return new GraphWithCloneFunctionality(this);
}

This way, you have the benefit of the Copy-Constructor, and the functionality of the clone as well, if needed.

After having said all that, I can't see any other significant issues in your code's functionality. It looks about right, and nice and clean.

If you create a copy-constructor, it would look something like:

public GraphWithCloneFunctionality(GraphWithCloneFunctionality tocopy) {
    this(copyGraph(tocopy.graph));
}

The copyGraph function would copy the graph, believe it or not.

Talking about graph-copying, this constructor should take a defensive copy of the graph as well. You don't want to have leaks of graph functionality to outside your class...

public GraphWithCloneFunctionality(Map, Map, Double>> graph) {
    if (graph == null) {
        throw new NullPointerException("The graph should not be null");
    }
    this.graph = graph;
}

The above this.graph = graph is not safe. Use this.graph = copyGraph(graph);

StackExchange Code Review Q#46510, answer score: 6