patternjavaMinor
Prims algorithm implementation
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
```
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
-
-
You have created a link between your data structures (
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
-
Why do you initialize
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.