Glasgow | 25 SDC NOV | Katarzyna Kazimierczuk | Sprint 2 | linked list #117
Glasgow | 25 SDC NOV | Katarzyna Kazimierczuk | Sprint 2 | linked list #117katarzynakaz wants to merge 6 commits intoCodeYourFuture:mainfrom
Conversation
katarzynakaz
added
📅 Sprint 2
katarzynakaz
changed the title
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Your current implementation is not an actual linked list -- it is an emulation of a linked list using Python list (which is an array). Can you implement a real linked list ? (https://en.wikipedia.org/wiki/Linked_list)
Also, can you implement the linked list as a class so that it can pass all the tests in linked_list_test.py?
cjyuan
added
Reviewed
katarzynakaz
added
the
Needs Review
| self.tracker_number = 0 | ||
| self.our_list = [] |
There was a problem hiding this comment.
Try implement the linked list without involving an array and tracker_number; they may steer you in the wrong direction.
There was a problem hiding this comment.
our_list is removed as requested, I kept the tracker number to practice assigning ids to elements
There was a problem hiding this comment.
Follow-up with this comment: #117 (comment)
What do you mean by "assigning ids to elements"? Why do you need to assign id to elements?
In your implementation, I don't see how you use tracker_number in the linked list, and I don't see how the tracker property in node is used. The tracker property is always assigned None.
cjyuan
removed
the
Needs Review
katarzynakaz
added
the
Needs Review
|
I don’t see any updates, and I don't see any response to my feedback. Did you forget to push your changes to GitHub? |
cjyuan
removed
the
Needs Review
|
the requested changes are implemented |
|
I still do not see any new commits on this PR branch. Did you made your commits on another branch? |
|
Thank you for the feedback. Not sure what happened as I pushed changes to this branch. The requested changes are now implemented. |
katarzynakaz
added
NotCoursework
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
The code in linked_list_test.py expects both .next and .previous properties of the removed node to be assigned None. Currently your implementation could not pass the tests.
Note: Do you know the why it is a good practice to assign .next and .previous of the removed node to None?
| class Node: | ||
| def __init__(self, tracker, inserted_item_key): | ||
| self.tracker = tracker | ||
| self.inserted_item_key = inserted_item_key |
There was a problem hiding this comment.
Typically a linked list node store some data in addition to the next and previous references.
You could name the property data or value if you needed.
There was a problem hiding this comment.
Thank you, I implemented all the requested changes. Since it now returns the node obj, the tracker is unnecessary and I deleted it. I didn't realize there was a standard naming convention and have renamed it. I understand adding None is good practice because it removes the node's references to the active list.
| self.tracker_number = 0 | ||
| self.our_list = [] |
There was a problem hiding this comment.
Follow-up with this comment: #117 (comment)
What do you mean by "assigning ids to elements"? Why do you need to assign id to elements?
In your implementation, I don't see how you use tracker_number in the linked list, and I don't see how the tracker property in node is used. The tracker property is always assigned None.
cjyuan
removed
the
Needs Review
katarzynakaz
added
the
Needs Review
|
|
||
| return wrapped_item | ||
|
|
||
| def remove(self, id_for_this_particular_item): |
There was a problem hiding this comment.
-
for_this_particulardoes not carry much useful info. -
Why not just name the parameter
node_to_remove?
cjyuan
added
Complete
2 participants
Self checklist
Changelist
Linked list python task is completed.