Skip to content

Improve Stack Implementation by making it Array based & adding missing methods with proper Tests #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 16, 2025

Conversation

Alokzh
Copy link
Contributor

@Alokzh Alokzh commented Jul 13, 2025

Summary

Improved CTStack replacing inefficient OrderedCollection based approach with high-performance Array based approach along with adding missing methods & Tests

Changes

  • Core: Array-based stack with fixed capacity and topIndex tracking
  • API: Added missing methods for proper Stack implementation
  • Tests: Added comprehensive Tests for complete implementation in CTStackTest class
  • CI: Updated the complete workflow
  • Readme: Enahanced Readme to include explanation of Stack & Quick start

@jordanmontt jordanmontt merged commit 086c183 into pharo-containers:master Jul 16, 2025
12 checks passed
@Ducasse
Copy link
Collaborator

Ducasse commented Jul 16, 2025

Hi

I have a couple of questions:

  • did you measure the speed gain?
  • what happen if the user puts more values on the stack than the array size?

@Alokzh
Copy link
Contributor Author

Alokzh commented Jul 17, 2025

Hi @Ducasse , Thnx for pointing out & You're correct

I made these changes because I thought that Stack should be of fixed size & was worried that OrderedCollection resizing would be slow but in reality other languages also allow Stack to dynamically change their size ( Ex: C++ ). Sorry I should have checked that earlier
As of now when Stack gets full it will throw an Error i.e self isFull ifTrue: [ self error: 'Stack is full' ].

Stephane I also noticed the original implementation used addFirst: and removeFirst:, I have a doubt I mean Are addFirst: & removeFirst in OrderedCollection O(1) or O(n) in Pharo ?
Reason: Adding or Removing the elements at first position we need to shift all other elements which would be O(n) ?

If they are O(n) , should I use addLast: & removeLast: which are O(1) ?

@jordanmontt
Copy link
Contributor

what happen if the user puts more values on the stack than the array size?

Yes, I also think that the stack should increase without limit. So we need to hand that case. We can do the same as in OrderedCollection. We allocate a new array the double size and we copy the elements. There is the method replaceFrom:to:with:startingAt: that copies the elements and it is a primitive, so it should be fast (the same as in Ordered Collection).

Are addFirst: & removeFirst in OrderedCollection O(1) or O(n) in Pharo ?

For addFirst: is difficult to tell. In this case I prefer to use the execution time as a metric instead of the time complexity for this cases. For example, there is the methodmakeRoomAtFirst can be be executed if a condition is met.
It either can allocate a new array leaving empty places at the beginning, or move all the elements some positions to the right. There are some operations that are done by primitives, meaning it is optimized code. So, estimating the time complexity is quite hard.

So, it will be better to prepare some examples using this new implementation vs the previous one to measure the speed gains. Could you first handle the case that the stack gets full and then you reallocate the double array etc... And then we measue the performance of the two implementations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants