Skip to main content
added 23 characters in body
Source Link
coderodde
  • 32.1k
  • 15
  • 78
  • 205

The idea is that childMap.get(\$u\$) will map \$u\$ to all the vertices \$v\$ such that there is a pair \$(u, v)\$ in the edge set of the graph (child nodes of u). Conversely, parentMap.get('\$u\$'\$u\$) will map \$u\$ to all the vertices \$v\$ such that there is a pair \$(v, u)\$ in the edge set (parent nodes of u).

I see that you compute not a shortest path to a target vertex, but rather a shortest path tree. You could encapsulate that tree into a class that supports a query method computing the shortest path from the source vertex to any given input target vertex:

I had this implementation in mind.

The idea is that childMap.get(\$u\$) will map \$u\$ to all the vertices \$v\$ such that there is a pair \$(u, v)\$ in the edge set of the graph (child nodes of u. Conversely, parentMap.get('\$u\$') will map \$u\$ to all the vertices \$v\$ such that there is a pair \$(v, u)\$ in the edge set (parent nodes of u).

I see that you compute not a shortest path to a target vertex, but rather a shortest path tree. You could encapsulate that tree into a class that supports a query method computing the shortest path from the source vertex to any given input:

I had this implementation.

The idea is that childMap.get(\$u\$) will map \$u\$ to all the vertices \$v\$ such that there is a pair \$(u, v)\$ in the edge set of the graph (child nodes of u). Conversely, parentMap.get(\$u\$) will map \$u\$ to all the vertices \$v\$ such that there is a pair \$(v, u)\$ in the edge set (parent nodes of u).

I see that you compute not a shortest path to a target vertex, but rather a shortest path tree. You could encapsulate that tree into a class that supports a query method computing the shortest path from the source vertex to any given input target vertex:

I had this implementation in mind.

Source Link
coderodde
  • 32.1k
  • 15
  • 78
  • 205

On graph node type name

I see that you named the graph node type N, which is OK since in Java we name the type arguments with a single letter according to the first letter of the name of the underlying concept. However, consider renaming it to V (as in Vertex).

I. General comments

Advice I.1

I strongly suggest you use Javadoc facilities for describing your code. If nothing else, you should explain, for instance, the difference between directed and undirected graphs somewhere in your code (preferably as class comments). Also, you could explain briefly what graphs are all about.

Advice I.2

I suggest you put all your classes in a package with a name like com.yourcompany.graph or similar.

II. Graph interface

First of all, let's take a look at your Graph interface:

import java.util.*;

public interface Graph<T> {

    boolean addEdge(T src, Edge edge);

    boolean removeEdge(T source, T destination);

    boolean addVertex(T vertex);

    boolean removeVertex(T vertex);

    void printAdjacencyList();

    List<Edge> getNeighbours(T vertex);

    Map<T, List<Edge>> getAdjacencyList();
}

Advice II.1

import java.util.*;

I really suggest you import explicitly all the classes you use. So, the above becomes:

import java.util.List;
import java.util.Map;

Advice II.2

In my opinion, introducing the Edge class adds an unnecessary level of indirection. I would have instead:

boolean addEdge(V sourceVertex, V targetVertex);

Advice II.3

I would remove printAdjacencyList and provide the methods for accessing the internals of the graph through the java.util.Collections.unmodifiableXXX wrappers so that the caller cannot tamper with the internal state directly.

Advice II.4: Suggested design

I had this mind:

package com.yourcompany.graph;

import java.util.Collection;
import java.util.Set;

/**
 * This interface defines the API for a graph.
 * 
 * @param <V> the actual graph vertex type.
 */
public interface Graph<V> {
    
    boolean addEdge(V sourceVertex, V targetVertex);
    boolean hasEdge(V sourceVertex, V targetVertex);
    boolean removeEdge(V sourceVertex, V targetVertex);
    
    boolean addVertex(V vertex);
    boolean hasVertex(V vertex);
    boolean removeVertex(V vertex);
    
    Set<V> getVertices();
    Collection<V> getParentVerticesOf(V vertex);
    Collection<V> getChildVerticesOf(V vertex);
    
    int numberOfNodes();
    int numberOfEdges();
}

III. Directed graph

Advice III.1

protected final Map<T, List<Edge>> adjacencyList = new HashMap<>();

I would write:

protected final Map<V, Set<V>> childMap = new HashMap<>();
protected final Map<V, Set<V>> parentMap = new HashMap<>();

That way, you can map each vertex to a java.util.HashSet that provides (expected) \$\mathcal{O}(1)\$ time for add, contains and remove. (java.util.List runs contains and remove in \$\mathcal{O}(n)\$ worst-case time.)

The idea is that childMap.get(\$u\$) will map \$u\$ to all the vertices \$v\$ such that there is a pair \$(u, v)\$ in the edge set of the graph (child nodes of u. Conversely, parentMap.get('\$u\$') will map \$u\$ to all the vertices \$v\$ such that there is a pair \$(v, u)\$ in the edge set (parent nodes of u).

Having separate parentMap allows us to have the method getParentVerticesOf, which, in turn, is required in bidirectional point-to-point pathfinding (bidirectional BFS, bidirectional Dijkstra's algorithm, NBA*).

Advice III.2

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

java.util.LinkedList is a poor choice. Consider java.util.ArrayList instead. The problem here is that LinkedList introduces \$\Theta(n)\$ worth space overhead for storing the internal linked list nodes. Also, the internal list nodes do not exhibit reference locality, so CPU cache won't help much. Consider this instead:

@Override
public boolean addVertex(V vertex) {
    checkInputNotNull(vertex);
    
    if (!childMap.containsKey(vertex)) {
        childMap.put(vertex, new HashSet<>());
        parentMap.put(vertex, new HashSet<>());
        vertices.add(vertex); // vertices is also a HashSet!
        return true;
    }
    
    return false;
}

Advice III.3

public boolean removeVertex(T vertex) {
    if (adjacencyList.containsKey(vertex)) {
        adjacencyList.remove(vertex);
        for (List<Edge> entry : adjacencyList.values()) {
            List<Edge> list = entry;
            for (Edge e : list) {
                if (e.getDestination().equals(vertex)) {
                    list.remove(e);
                }
            }
        }
        return true;
    }
    return false;
}

The outer for loop iterates around (+1/-1) \$n\$ times and the outer loop iterates around \$m\$ times, so the running time is \$\Theta(m + n + \sum_{i = 1}^n c_i) = \Theta(m + n)\$ where \$c_i\$ is the number of nodes in the adjacency list of the node \$n_i\$. You can definitely drop this to \$\Theta(1)\$ with the following implementation of the method in question:

public boolean removeVertex(V vertex) {
    checkInputNotNull(vertex);
    
    if (!hasVertex(vertex)) {
        return false;
    }
    
    numberOfEdges -= 
            (childMap.get(vertex).size() + parentMap.get(vertex).size());
    
    childMap.remove(vertex);
    parentMap.remove(vertex);
    vertices.remove(vertex);
    return true;
}

Advice III.4

public boolean removeEdge(T source, T destination) {
    if (adjacencyList.containsKey(source)) {
        for (Edge e : adjacencyList.get(source)) {
            if (e.getDestination().equals(destination)) {
                adjacencyList.get(source).remove(e);
                return true;
            }
        }
    }
    return false;
}

Above, the worst-case running time of the method is \$\mathcal{O}(k^2)\$, where \$k = \$ adjacencyList.get(source).size(). You can do this in constant time:

public boolean removeEdge(V sourceVertex, V targetVertex) {
    checkInputNotNull(sourceVertex);
    checkInputNotNull(targetVertex);
    
    if (!childMap.containsKey(sourceVertex)) {
        return false;
    }
    
    if (!childMap.get(sourceVertex).contains(targetVertex)) {
        return false;
    }
    
    childMap.get(sourceVertex).remove(targetVertex);
    parentMap.get(targetVertex).remove(sourceVertex);
    numberOfEdges--;
    return true;
}

Note III.5

public Map<T, List<Edge>> getAdjacencyList() {
    return Collections.unmodifiableMap(adjacencyList);
}

Good!

IV. Dijkstra shortest path tree

Note IV.1

I see that you compute not a shortest path to a target vertex, but rather a shortest path tree. You could encapsulate that tree into a class that supports a query method computing the shortest path from the source vertex to any given input:

package com.yourcompany.graph;

// imports.

public class ShortestPath<V> {
    
    private final List<V> path = new ArrayList<>();
    
    public ShortestPath(List<V> path) {
        this.path.addAll(path);
    }
    
    public List<V> getVertexList() {
        return Collections.<V>unmodifiableList(path);
    }
    
    public int getPathWeight(WeightFunction<V> weightFunction) {
        int pathWeight = 0;
        
        for (int i = 0; i < path.size() - 1; i++) {
            pathWeight = pathWeight + weightFunction.getWeight(path.get(i),
                                                               path.get(i + 1));
        }
        
        return pathWeight;
    }
}

... and ...

package com.yourcompany.graph;

// imports.

public class ShortestPathTreeBuilder<V> {

    private final Map<V, V> parentMap = new HashMap<>();
    
    public ShortestPathTreeBuilder(Map<V, V> parentMap) {
        this.parentMap.putAll(
                Objects.requireNonNull(
                        parentMap,
                        "The input parent map is null."));
    }
    
    public ShortestPath<V> buildPathTo(V targetVertex) {
        List<V> path = new ArrayList<>();
        V node = targetVertex;
        
        while (node != null) {
            path.add(node);
            node = parentMap.get(node);
        }
        
        Collections.<V>reverse(path);
        return new ShortestPath<>(path);
    }
}

Advice IV.2

I would extract the edge weights from the Edge objects, and code up a simple class for storing the weights for any single ordered pair of vertices:

package com.yourcompany.graph;

public interface WeightFunction<V> {

    void setWeight(V sourceVertex, V targetVertex, int weight);
    int getWeight(V sourceVertex, V targetVertex);
}

Also,

package com.yourcompany.graph.impl;

import com.yourcompany.graph.WeightFunction;
import java.util.HashMap;
import java.util.Map;

public class DirectedGraphWeightFunction<V> 
implements WeightFunction<V> {

    private final Map<V, Map<V, Integer>> map = new HashMap<>();
    
    @Override
    public void setWeight(V sourceVertex, V targetVertex, int weight) {
        if (!map.containsKey(sourceVertex)) {
            map.put(sourceVertex, new HashMap<>());
        }
        
        map.get(sourceVertex).put(targetVertex, weight);
    }
    
    @Override
    public int getWeight(V sourceVertex, V targetVertex) {
        return map.get(sourceVertex).get(targetVertex);
    }
}

Note IV.3

//Keeping track of visited vertices here
List<T> visited = new ArrayList<>();

This will definitely limit your computation speed. The problem is that visited.contains(edge.getDestination()) will run in worst-case \$\mathcal{O}(n)\$ time, where \$n\$ is the number of vertices in the graph. This leads to the fact that your implementation runs in \$\mathcal{O}(mn)\$ worst-case time. Use java.util.HashSet instead and the time complexity will drop to \$\mathcal{(m + n) \log n}\$. (With Fibonacci heap, this drops to \$\mathcal{O}(m + n \log n)\$, but Fibonacci heap hides very large hidden constant factors, so its usage in practice is questionable.)

Summa summarum

I had this implementation.

Hope that helps.