Skip to content

Week09 bwan1#67

Open
bwan1 wants to merge 3 commits intoUCL-COMP0233-2022-2023:week09from
bwan1:week09-bwan1
Open

Week09 bwan1#67
bwan1 wants to merge 3 commits intoUCL-COMP0233-2022-2023:week09from
bwan1:week09-bwan1

Conversation

@bwan1
Copy link

@bwan1 bwan1 commented Dec 8, 2022

@bwan1
Copy link
Author

bwan1 commented Dec 8, 2022

@bwan1
Copy link
Author

bwan1 commented Dec 8, 2022

@stefpiatek stefpiatek changed the base branch from main to week09 December 9, 2022 09:59
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, some suggestions of guard clauses and limiting the amount that is imported

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.

🎉
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

def forget(self, person):
"""Removes any connections to a person"""
pass
if person in self.connections:

Choose a reason for hiding this comment

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

I like that you've made sure that they exist, in our example we've thrown a specific exception here but this is also another sensible way to deal with it

Comment on lines +37 to +38
else:
return 0

Choose a reason for hiding this comment

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

This is certainly one way to deal with it, the other would be to throw an exception

def number_of_connections(self, name):
"""Find the number of connections that a person in the group has"""
pass
if name in self.connections:
Copy link

@stefpiatek stefpiatek Dec 9, 2022

Choose a reason for hiding this comment

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

Oh interesting, you've used a different data structure than our example I think, we've used this in our example

Suggested change
if name in self.connections:
if self.contains(name):

Optional reciprocal: If true, will add the relationship from name2 to name 1 as well
"""
pass
if self.number_of_connections(name1) != 0:

Choose a reason for hiding this comment

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

Also could have a guard clause here to check that we know about these people exist in our group

Suggested change
if self.number_of_connections(name1) != 0:
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.")
if self.number_of_connections(name1) != 0:

def forget(self, name1, name2):
"""Remove the connection between two people."""
pass
if self.number_of_connections(name1) != 0:

Choose a reason for hiding this comment

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

could also add a guard clause here too

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