Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring - Part 1 #63

Closed

Conversation

wenzhaojia2000
Copy link

@wenzhaojia2000 wenzhaojia2000 commented Dec 7, 2022

Copy link

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

👏 Nice work, could go even further by adding guard clauses, I've added in a couple examples here


forget("Nash", "John")
forget(group, "Nash", "John")

if __name__ == "__main__":
assert len(group) == 4, "Group should have 4 members"

Choose a reason for hiding this comment

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

Really good, you could also only define the group within the if __name__ == "__main__" block, so that you don't have a group defined if you import this file

Comment on lines +15 to +16
# didn't know you can have objects as keys but i guess there's no reason
# not to

Choose a reason for hiding this comment

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

yep, often you'd have a string, but doesn't have to be

self.connections[person] = relation

def forget(self, person):
"""Removes any connections to a person"""
pass
del self.connections[person]

Choose a reason for hiding this comment

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

You could also add a guard clause here so that the user gets a better error message to help them understand what is wrong

Suggested change
del self.connections[person]
if person not in self.connections:
raise ValueError(f"I don't know about {person.name}")
del self.connections[person]

@@ -32,17 +32,27 @@ def add_person(self, name, age, job):

def number_of_connections(self, name):
"""Find the number of connections that a person in the group has"""
pass
return len(self.connections[name])

Choose a reason for hiding this comment

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

Could also have another guard clause here

Suggested change
return len(self.connections[name])
if not self.contains(name):
raise ValueError(f"I don't know about {name}.")
return len(self.connections[name])

Comment on lines +41 to +46
try:
self.connections[name1] += [(name2, relation)]
except:
self.connections[name1] = [(name2, relation)]
if reciprocal:
self.connect(name2, name1, relation, False)

Choose a reason for hiding this comment

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

Ah interesting, I thinik our example we the pairs of connections but this works too. Again could have some guard clauses to make sure that the names only existing people are being used and they they already know

Suggested change
try:
self.connections[name1] += [(name2, relation)]
except:
self.connections[name1] = [(name2, relation)]
if reciprocal:
self.connect(name2, name1, relation, False)
if not self.contains(name1):
raise ValueError(f"I don't know who {name1} is.")
if not self.contains(name2):
raise ValueError(f"I don't know who {name2} is.")
try:
self.connections[name1] += [(name2, relation)]
except:
self.connections[name1] = [(name2, relation)]
if reciprocal:
self.connect(name2, name1, relation, False)

@stefpiatek stefpiatek closed this Dec 9, 2022
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.

2 participants