Sheffield | 25-SDC-Nov | Hassan Osman | Sprint 2 | Implement a linked list in Python

[HassanOHOsman]

Self checklist

• I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
• My changes meet the requirements of the task
• I have tested my changes
• My changes follow the style guide

In this pull request, I implemented a linked list with push_head, pop_tail, and remove operations, all in O(1) time.

[cjyuan]

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?

[cjyuan]

• Could consider delegating the node removing task to remove() -- less code to maintain.

• Could also consider this style of code writing:

        # Handle all errors and special cases first
        if self.tail is None:
            raise IndexError("Unable to remove from empty linked list")

        # From this point onward, we don't have to worry about special cases.

        # No else needed; one less indentation level.
        tail_node = self.tail
        previous = self.tail.previous
        self.tail = previous
        if self.tail is not None:
            self.tail.next = None
        else:
            self.head = None

[HassanOHOsman]

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?

Strangely, all three tests passed on my end. Here's a screenshot for you reference.

Having said that, I will check it again and see what's gone wrong. Thank you CJ :)

[cjyuan]

The original test is not quite complete. I added a few more myself, and here is one of them.

    def test_remove_middle(self):
        l = LinkedList()
        l.push_head("a")
        b = l.push_head("b")
        l.push_head("c")

        l.remove(b)

        self.assertIsNone(b.next)
        self.assertIsNone(b.previous)

        self.assertEqual(l.pop_tail(), "a")
        self.assertEqual(l.pop_tail(), "c")

[HassanOHOsman]

The original test is not quite complete. I added a few more myself, and here is one of them.

    def test_remove_middle(self):
        l = LinkedList()
        l.push_head("a")
        b = l.push_head("b")
        l.push_head("c")

        l.remove(b)

        self.assertIsNone(b.next)
        self.assertIsNone(b.previous)

        self.assertEqual(l.pop_tail(), "a")
        self.assertEqual(l.pop_tail(), "c")

Thanks for sharing, I will update my test file to include what've just shared and then work to pass it.

[HassanOHOsman]

Note: Do you know the why it is a good practice to assign .next and .previous of the removed node to None?

Is it to ensure that the removed node is dormant/permanently isolated, so that nobody accidentally in the future re-link it back to the list?

[cjyuan]

Is it to ensure that the removed node is dormant/permanently isolated, so that nobody accidentally in the future re-link it back to the list?

That one of the reasons. Another one is related to garbage collection.

[cjyuan]

Currently it is still possible that some removed nodes do not have their .previous properly set to None.

Can you fix the bug?

[HassanOHOsman]

updated my logic now so we first capture the previous and next nodes to our current node before any changes we make to the linked array structure (re-linking).

[cjyuan]

Currently it is still possible that some removed nodes do not have their .previous properly set to None.

This is part of the cons of "having more code to maintain" -- focus on where node can be removed from the linked list in your implementation.