algorithms-sedgewick-python icon indicating copy to clipboard operation
algorithms-sedgewick-python copied to clipboard

Wrong solution for stack copy

Open Arquestro opened this issue 4 years ago • 3 comments
trafficstars

In the test example of 1.3.12 exercise, the first(or head) element of the stack should be 4, not 1. The copy of the stack is done in reverse, breaking it's structure: https://github.com/ChangeMyUsername/algorithms-sedgewick-python/blob/15007b02081f5e398aeb316f5935ce9c97e59ca8/chapter_1/module_1_3.py#L154

I suppose it should be done through another temporary stack to ensure structure integrity:


    # 1.3.12 practice
    @classmethod
    def copy(cls, old_stack: Stack) -> None:
        """
        Copy existing stack and return new stack.
        Args:
            cls: Stack class
            stack (Stack): existing stack
        Returns:
            Stack: new stack
        >>> s = Stack()
        >>> s.push(1)
        >>> s.push(2)
        >>> s.push(3)
        >>> s_copy = Stack.copy(s)
        >>> s_copy._first.val
        3
        >>> s_copy.size()
        3
        >>> for item in s_copy:
        ...     print(item)
        ...
        3
        2
        1
        """
        s_tmp = cls()
        s_copy = cls()
        if isinstance(old_stack, Stack) and not old_stack.is_empty():
            for item in old_stack:
                s_tmp.push(item)
            for item in s_tmp:
                s_copy.push(item)
        return s_copy

O(N) time is expected. But O(N) space, I don't think we can avoid that.

And I guess the constructor here should be changed too, because it has the same behaviour: https://github.com/ChangeMyUsername/algorithms-sedgewick-python/blob/15007b02081f5e398aeb316f5935ce9c97e59ca8/chapter_1/module_1_3.py#L37

Maybe even make the option to make a copy in 'init' if there is a 'Stack' in 'args'.

Arquestro avatar Sep 30 '21 12:09 Arquestro

Sorry for the late reply (getting lazy these days...), issues you mentioned have been updated, you can check and see if any further issues.

ChangeMyUsername avatar Dec 13 '21 16:12 ChangeMyUsername

It's all right, no stress :) Just want to mention, that constructor behaviour is the same as old copy, it should be changed to self.copy instead of pushing, imo.

Arquestro avatar Dec 14 '21 09:12 Arquestro

These fixes are not optimized, should be a better approve for this solution

ChangeMyUsername avatar Dec 15 '21 13:12 ChangeMyUsername