Conversation
src/main/java/graphs/GraphEdge.java
Outdated
| return 1; | ||
| } | ||
| // is it necessary to specify "this." ? | ||
| if(edge.getWeight() > this.getWeight()) { |
There was a problem hiding this comment.
use Double's compareTo instead
There was a problem hiding this comment.
so the compare to method just cares about less than, greater than, or equal to zero. it doesnt need to handle the margin of difference. since im just returning 0, -1 or 1, I could change it to Double compareTo(), but it looks like the Comparable interface says the method should return an int, if i change that would the Comparable interface not be implemented correctly and thus Collection.sort() wouldnt work?
There was a problem hiding this comment.
I was thinking it would be this:
Double.compare(this.getWeight(), edge.getWeight())
I think that's the method we're looking for.
| edgesForGraph11.add(edgeBetweenDandC.setWeighted(true).setWeight(1)); | ||
| edgesForGraph11.add(edgeBetweenEandD.setWeighted(true).setWeight(1)); | ||
| edgesForGraph11.add(edgeBetweenDandF.setWeighted(true).setWeight(1)); | ||
| edgesForGraph11.add(edgeBetweenAandD.setWeight(1)); |
There was a problem hiding this comment.
why not set the weight on the edge in a constructor? or even better, declare this graph unweighted?
There was a problem hiding this comment.
this graph is weighted, because theres an option to go to two different edges from node Z, either to node D or to node G, and ultimately, going to D would be the shorter path, but i am tricking the algo to take the local optimum which is to node G because the other edge (Z->D) has weight 2, which adds one extra edge to the final path.
src/main/java/graphs/GraphClass.java
Outdated
| edges.add(new GraphEdge(edge.getRightNode(), edge.getLeftNode(), edge.getWeight())); | ||
| } | ||
|
|
||
| public GraphClass duplicateUndirectedEdges() { |
There was a problem hiding this comment.
do we need this? is there any other way to add an edge besides the constructor(s) and addEdge?
There was a problem hiding this comment.
what i am thinking now, is change the Graph constructors to take the isDirected flag, and then set that first in the constructor, and then say if(isDirected()) this.edges = new List(edges) else for(edge : edges) addEdge(edge)
this will either just put the edges into the graphs edge list, or if it's undirected, add each edge individually with the public addEdge() method which would duplicate the edge for both directions. then I can remove the public duplicateEdges() method, and just handle the edges in the constructor, and allow any new edges to be duplicated if it's not directed.
shalka
left a comment
shalka
left a comment
There was a problem hiding this comment.
things are looking really good!
|
|
||
| // "Note: this class has a natural ordering that is inconsistent with equals." | ||
| public int compareTo(Edge edge) { | ||
| return Double.compare(this.getWeight(), edge.getWeight()); |
|
|
||
| public GraphClass(GraphClass graph) { | ||
| public Graph(Graph graph) { | ||
| edges = graph.getEdges(); |
There was a problem hiding this comment.
Is the intent of this copy constructor is to provide a deep copy of the specified graph? Or is it alright that we point to the same set of edges and nodes that the original graph does?
There was a problem hiding this comment.
I dont really know why i implemented this, i guess just in case. here the new graph instance will have it's edges pointing to the same edges object of the first graph? so if i added an edge to the second graph, would the edges of the first graph update as well?
| throw new IllegalArgumentException("null start node or target node"); | ||
| } | ||
| if(! (graph.getNodes().contains(start) && graph.getNodes().contains(finish))) { | ||
| return new LinkedList(); |
There was a problem hiding this comment.
it seems to me like this would be exceptional. the caller provided a graph that doesn't contain the start or finish node. empty list indicates that there is no path between start and finish, but implicitly they are both contained in the graph. my vote is a throw here (though if you feel strongly about this, i think its alright)
2 participants
abstracts weighted / directed flags to graph.
edges now are always directed, weighted with a default weight of 1.
a few the tests failed at first because the algos are deterministic depending on the order the edges are added to the List that is used to instantiate that graph.