London | 26-SDC-Mar | Ebrahim Beiati-Asl | Sprint 2 | implement_skip_list#155
London | 26-SDC-Mar | Ebrahim Beiati-Asl | Sprint 2 | implement_skip_list#155ebrahimbeiati wants to merge 1 commit into
Conversation
ebrahimbeiati
added
the
Needs Review
|
|
||
| if pos < len(self.data) and self.data[pos] == value: | ||
| return # value already exists, do not insert duplicates | ||
| self.data.insert(pos, value) |
There was a problem hiding this comment.
This insertion operation is O(n), which does not meet the requirement.
cjyuan
added
Reviewed
ebrahimbeiati
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
What you implemented does not look like a "Skip List".
May I suggest search for Skip List pseudo code, and then convert the pseudo code to Python? There may be more than one way to implement Skip List, so look for something you can understand.
This video gives a good illustration of how Skip List works, but it does not include any pseudo code: https://www.youtube.com/watch?v=UGaOXaXAM5M
| def _get_node(self, index): | ||
| """Get the node at the given index.""" | ||
| current = self.head | ||
| for _ in range(index): | ||
| if current is None: | ||
| return None | ||
| current = current.next | ||
| return current |
There was a problem hiding this comment.
This function performs a linear traversal on the linked list from head.
There was a problem hiding this comment.
When index is close to n, this function has to traverse about n nodes to reach current.
The complexity of a function calling this function will be at least O(n).
| for i in range(len(self.skips) - 1): | ||
| a = self.skips[i] | ||
| b = self.skips[i + 1] | ||
|
|
||
| node_a = self._get_node(a) | ||
| node_b = self._get_node(b) |
There was a problem hiding this comment.
This loop, together with the linear traversal performed by _get_node(pos), suggests a complexity of worse than O(n).
cjyuan
removed
the
Needs Review
ebrahimbeiati
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Your implementation does not look like a typical Skip List. I don't think you can make it a Skip List by making some changes.
I would suggest try to understand how Skip List works first, then study the pseudo code of a Skip List, and then reimplement it from the beginning.
| def _get_node(self, index): | ||
| """Get the node at the given index.""" | ||
| current = self.head | ||
| for _ in range(index): | ||
| if current is None: | ||
| return None | ||
| current = current.next | ||
| return current |
There was a problem hiding this comment.
When index is close to n, this function has to traverse about n nodes to reach current.
The complexity of a function calling this function will be at least O(n).
cjyuan
removed
the
Needs Review
ebrahimbeiati
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
ebrahimbeiati
force-pushed
the
skip_list_pr
branch
from
ff49dd6 to
ae2105f
Compare
ebrahimbeiati
added
the
Needs Review
2 participants
Learners, PR Template
Self checklist