-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
custom-set: implement exercise #1140
Conversation
…thon into implement-custom-set
exercises/custom-set/example.py
Outdated
self.elements = list(elements) | ||
|
||
def empty(self): | ||
return not any(self.elements) |
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.
This should be return not self.elements
- the current version would wrongly return True
if self.elements == [0]
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.
Good catch... fixing.
exercises/custom-set/custom_set.py
Outdated
def subset(self, other): | ||
pass | ||
|
||
def disjoint(self, other): |
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.
I think these should be isdisjoint
and issubset
to match with Python's implementation of set
. I would also then change empty
to isempty
for consistency.
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.
Agreed.
exercises/custom-set/example.py
Outdated
return not any(self.elements) | ||
|
||
def __iter__(self): | ||
return iter(self.elements) |
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.
__iter__
is a kind of useful method to implement here, but I think that __contains__
would be a more logical for testing whether a set contains a particular value. Presumably __iter__
is providing that functionality here, but from a learner point of view, I don't think that this would be clear enough.
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.
I somehow was not aware of the __contains__
method previously, and though that implementing __iter__
was the correct way of supporting the in
operator... I will fix this, thanks!
Resolves #746