NW | 26-SDC-Mar | TzeMing Ho | Sprint 2 | implement_linked_list#168
NW | 26-SDC-Mar | TzeMing Ho | Sprint 2 | implement_linked_list#168TzeMingHo wants to merge 2 commits into
Conversation
| self.tail = new_tail | ||
| return val | ||
|
|
||
| def remove(self, Node): |
There was a problem hiding this comment.
Python naming convention for parameters/variables is lower snake case
| new_next = Node.next | ||
|
|
||
| if self.head == Node and self.tail == Node: | ||
| Node.value = None |
There was a problem hiding this comment.
Why clear the value of the removed node?
If it is needed, why not do the same when the removed node is not the only node in the linked list?
| new_next.previous = None | ||
| self.head = new_next | ||
| elif self.tail == Node: | ||
| self.pop_tail() |
There was a problem hiding this comment.
Could consider writing the "remove tail" code in this function, and then use remove() in pop_tail().
- Have all the remove logic in one place.
| Node.previous = None | ||
| Node.next = None |
There was a problem hiding this comment.
Could probably factor out these two lines of code.
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
Code could probably use some internal documentation.
| return None | ||
|
|
||
| return self.remove(self.tail) | ||
|
|
||
| def remove(self, node): | ||
| if self.head == None and self.tail == None: | ||
| return |
There was a problem hiding this comment.
Could use the same syntax to return None.
|
Forgot to suggest researching this
|
Learners, PR Template
Self checklist
Changelist