Conversation
…d tests, all passing.
…n and written test
|
|
||
|
|
||
| class Graph(object): | ||
| """Graph class.""" |
There was a problem hiding this comment.
This graph has weights. The doc string should inform me of it.
src/graph.py
Outdated
| """Return a list of edges in graph.""" | ||
| edges = [] | ||
| for key in self._graph: | ||
| for i in self._graph[key]: |
src/graph.py
Outdated
|
|
||
| def add_node(self, val): | ||
| """Add a node with value of val to graph.""" | ||
| self._graph[val] = {} |
There was a problem hiding this comment.
So if I add a node with the same name as a pre-existing node, I'm going to end up overwriting it and emptying its contents
src/graph.py
Outdated
| self._graph[val1][val2] = weight | ||
| elif val2 in self._graph and val1 not in self._graph: | ||
| self._graph[val1] = {} | ||
| self._graph[val1][val2] = weight |
There was a problem hiding this comment.
You do this line no matter what the condition, so why not just do this once outside of the conditional statements?
src/graph.py
Outdated
| if val2 not in self._graph[val1]: | ||
| self._graph[val1][val2] = weight | ||
| elif val1 in self._graph and val2 not in self._graph: | ||
| self._graph[val2] = {} |
There was a problem hiding this comment.
You're adding a new node here. You have a method whose job it is to add new nodes. Don't forget that
src/graph.py
Outdated
| if val in self._graph: | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
This could be one-lined as
return val in self._graph
src/graph.py
Outdated
| if child == val and key not in neighbors: | ||
| neighbors.append(key) | ||
| else: | ||
| return neighbors |
There was a problem hiding this comment.
My lord this is a lot of code for a simple task. First, the neighbors of val are only going to be the nodes that val points to. This is not a two-way relationship. That's why this is a directed graph. Second, because you're effectively just harvesting all the keys that val points to, this whole method could probably just be:
try:
return list(self._graph[val].keys())
except KeyError:
raise ValueError('This node doesnt exist')
src/graph.py
Outdated
| else: | ||
| return neighbors | ||
| except KeyError: | ||
| raise ValueError('This node dosent exit') |
src/graph.py
Outdated
| else: | ||
| return False | ||
| else: | ||
| raise ValueError('These edges do not exist.') |
There was a problem hiding this comment.
try:
return val2 in self._graph[val1]
except KeyError:
raise ValueError('%s is not a node in this graph' % val1)
src/graph.py
Outdated
| val = path.pop() | ||
| if val not in depth_traversal: | ||
| depth_traversal.append(val) | ||
| path = path + list(self._graph[val].keys()) |
There was a problem hiding this comment.
This isn't wrong. Just wanted to note you could also have written
path = path.extend(list(self._graph[val].keys()))or
path += list(self._graph[val].keys())2 participants
No description provided.