algorithms-sedgewick-python
algorithms-sedgewick-python copied to clipboard
Wrong solution for stack copy
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'.
Sorry for the late reply (getting lazy these days...), issues you mentioned have been updated, you can check and see if any further issues.
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.
These fixes are not optimized, should be a better approve for this solution