-
Notifications
You must be signed in to change notification settings - Fork 35
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
Refactoring - Part 1 #63
Conversation
There was a problem hiding this 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" |
There was a problem hiding this comment.
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
# didn't know you can have objects as keys but i guess there's no reason | ||
# not to |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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
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]) |
There was a problem hiding this comment.
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
return len(self.connections[name]) | |
if not self.contains(name): | |
raise ValueError(f"I don't know about {name}.") | |
return len(self.connections[name]) |
try: | ||
self.connections[name1] += [(name2, relation)] | ||
except: | ||
self.connections[name1] = [(name2, relation)] | ||
if reciprocal: | ||
self.connect(name2, name1, relation, False) |
There was a problem hiding this comment.
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
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) |
Answers UCL-COMP0233-2022-2023/RSE-Classwork#51
Answers UCL-COMP0233-2022-2023/RSE-Classwork#52
Answers UCL-COMP0233-2022-2023/RSE-Classwork#53