Skip to content

Weights#9

Merged
endere merged 35 commits intomasterfrom
weights
Jun 30, 2017
Merged

Weights#9
endere merged 35 commits intomasterfrom
weights

Conversation

@endere
Copy link
Copy Markdown
Collaborator

@endere endere commented Jun 18, 2017

No description provided.

Copy link
Copy Markdown

@clair3st clair3st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work on the implementation, I've provided comments on how some of your code could be improve. Also where are your tests?

branch: queue

Deque in deque.py, test in test_deque.py
branch: deque
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add your graph stuff to the readme

author="David Lim, Ronel Rustia, Erik Enderlein",
author_email="armydavidlim@gmail.com",
license="MIT",
py_modules=['linked_list', 'stack', 'doubly_linked'],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more modules than this

@@ -0,0 +1,178 @@
"""Graphy mcgraphface"""
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My PEP 8 linter is not happy with this entire file, you need to follow pep 8 styles. Docstrings are needed for all functions, and a space between function definitions!

def neighbors(self, val):
"""Return all nodes connected to given node."""
try:
to_return = []
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really unpythonic. Why don't you just return the value of the key given. ie return self.graph_dict[val] in the try except block

raise KeyError("No such node exists.")
def adjacent(self, val1, val2):
"""Return True if edge exists, else return false."""
try:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also is very unpythonic. In python we can check for membership using the in keyword which will return a boolean. Lines 78-82 could be cut down to 1.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we had to be a bit more interesting, since we're looking at a list of lists, the first value of which we are comparing against val2. The earlier iteration of the function used the in keyword, but that proved to not work in this scenario. We did, however, trim it down to a one liner with a lambda function. So there's that.


def nodes(self):
"""Return a list of all keys in dictionary."""
to_return = []
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an inbuilt dictionary method which would allow you to do this one line.




if __name__ == '__main__':
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete all this for submission

@endere endere merged commit 9c75173 into master Jun 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants