HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Prims algorithm implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
algorithmimplementationprims

Problem

Review this code regarding optimization, cleanup, and best practices.

```
final class EdgePrims {
private final T source, target;
private final int distance;

public EdgePrims(T node1, T node2, int distance) {
this.source = node1;
this.target = node2;
this.distance = distance;
}

public T getSource() {
return source;
}

public T getTarget() {
return target;
}

public int getDistance() {
return distance;
}

@Override
public String toString() {
return " first vertex " + source + " to vertex " + target + " with distance: " + distance;
}
}

final class GraphPrims implements Iterable {

private final Map> graph;

public GraphPrims() {
graph = new HashMap>();
}

public void addEgde(T vertex1, T vertex2, int distance) {
if (vertex1 == null) {
throw new NullPointerException("The vertex 1 cannot be null");
}
if (vertex2 == null) {
throw new NullPointerException("The vertex 2 cannot be null");
}

if (!graph.containsKey(vertex1)) {
graph.put(vertex1, new HashMap());
}
if (!graph.containsKey(vertex2)) {
graph.put(vertex2, new HashMap());
}
graph.get(vertex1).put(vertex2, distance);
graph.get(vertex2).put(vertex1, distance);
}

public Set getVertices() {
return Collections.unmodifiableSet(graph.keySet()); // QQ: should this be replaced by DEEP COPy ?
}

public Map getEdges(T source) {
if (source == null) {
throw new NullPointerException("The source cannot be null.");
}
return Collections.unmodifiableMap(graph.get(source));
}

public void removeEdges(T vertex1, T vertex2) {
if (vertex1 == null) {
throw new NullPointerException("The vertex 1 cannot be null");
}
if (vertex2 == null) {
throw new NullPointerEx

Solution

-
public EdgePrims(T node1, T node2, int distance) - What are node1 and node2? From reading the source one can see they are source and target node - so they should be named accordingly. The names of the parameters of a function are an important piece of documentation and should convey their meaning.

-
You have created a link between your data structures (GraphPrims, EdgePrims) and an algorithm which seems weird as they have only in common that Prim's is a graph algorithm - meaning you need a graph to run it on but the graph doesn't need the algorithm. I'm pretty sure I could implement Dijkstra's algorithm and run it on a graph of yours.

The problem is that this creates a barrier in your mind for the re-usability of the classes. Also it reads odd if I write

class Dijkstras 
{
    public static  List> getShortestPath(GraphPrims graph, EdgePrims from, EdgePrims to)
    { ... }
}


-
Why do you initialize edgesAvailable with a default initial capacity of 10? According to the Java documentation the default initial capacity is 11. Why is 10 any better?

Code Snippets

class Dijkstras 
{
    public static <T> List<EdgePrims<T>> getShortestPath(GraphPrims<T> graph, EdgePrims<T> from, EdgePrims<T> to)
    { ... }
}

Context

StackExchange Code Review Q#36488, answer score: 6

Revisions (0)

No revisions yet.