Graph Implementation in Java 8

Here is my code which implements a simple directed graph in Java 8. Here are some of the things I am unsure about:

-
I am fairly new to Java 8 concepts. Have I used the new API correctly (and not increased the running time of the operations)?

-
In the current implementation, there is no way to iterate over the list of the edges. Is this a shortcoming?

-
Right now, adding new operations like DFS or weighted shortest path, I will have to change the Graph.java class. How can I improve the design such that adding graph algorithms, does not require changing the graph data class (or is there no need to do this)?

Graph.java

```
public class Graph {

private Map> adjacencyList;

public Graph() {
adjacencyList = new HashMap<>();
}

/**
* Adds a vertex to the graph.
* @param vertex vertex to add
*/
public boolean addVertex(T vertex) {
if (adjacencyList.containsKey(vertex)) {
return false;
}
adjacencyList.put(vertex, new Node<>(vertex));
return true;
}

/**
* Adds a directed edge between two vertices in the graph.
* @param vertex1 vertex where the (directed) edge begins
* @param vertex2 vertex where the (directed) edge ends
*/
public boolean addEdge(T vertex1, T vertex2) {
return addEdge(vertex1, vertex2, 0);
}

/**
* Adds a weighted directed edge between two vertices in the graph.
* @param vertex1 vertex where the (directed) edge begins
* @param vertex2 vertex where the (directed) edge ends
* @param weight weight of the edge
*/
public boolean addEdge(T vertex1, T vertex2, int weight) {
if (!containsVertex(vertex1) || !containsVertex(vertex2)) {
throw new RuntimeException("Vertex does not exist");
}

// add the edge
Node node1 = getNode(vertex1);
Node node2 = getNode(vertex2);
return node1.addEdge(node2, weig

These are mostly going to be general comments.

private Map> adjacencyList;

This field can (and should) be made final. This applies to fields in your other classes as well.

I also don't see why you initialize it in the constructor. You can just initialize it at declaration and drop the constructor entirely, since it will then be the default constructor.

I also find it slightly confusing that the name suggests that it is a list when really it is a Map. This is a mismatch.

public boolean addVertex(T vertex) {
    if (adjacencyList.containsKey(vertex)) {
        return false;
    }
    adjacencyList.put(vertex, new Node<>(vertex));
    return true;
}

You have a method containsVertex that you also use in your other methods. You should use it here as well. This also applies to other places in your code.

if (!containsVertex(vertex1) || !containsVertex(vertex2)) {
    throw new RuntimeException("Vertex does not exist");
}

You could use a custom exception here, e.g., VertexNotFoundException or something similar. This makes exception handling easier for the caller.

Also, this kind of code is duplicated. You could create a private checkVertexExists method (or some other name) to which you pass a vertex and that immediately throws if the vertex doesn't exist.

// add the edge
// get node to be removed
// remove all incoming edges to node
// remove the node

These are all noise comments and are meaningless. Don't comment the obvious. Let your code comment itself. There are more comments of this kind, this is just a small selection.

// run bfs on the graph
runBFS(startVertex);

Again, this comment is meaningless. But also, runBFS is a relatively cryptic method name. What if I don't know that acronym? I would use a more descriptive name here.

if (end == null) {
    return null;
}
else {
    Collections.reverse(path);
}
return path;

You don't need that else block at all since the if case immediately returns. Just put the reverse call prior to the return path and drop the else.

private void runBFS(T startVertex) {
    // ...
}

Apart from renaming it, which I mentioned above, this method is too long and does too much. Split it up into parts. In fact, you may find that it might be worth extracting this into another class entirely.

StackExchange Code Review Q#67970, answer score: 4