Skip to content

Implement growable stack and max-depth tracking (#8)#10

Open
HossamSaberr wants to merge 3 commits intopharo-containers:masterfrom
HossamSaberr:master
Open

Implement growable stack and max-depth tracking (#8)#10
HossamSaberr wants to merge 3 commits intopharo-containers:masterfrom
HossamSaberr:master

Conversation

@HossamSaberr
Copy link
Copy Markdown

This closes #8.

Overview

This PR fix the issue by introducing a growable stack implementation .

Changes

  1. CTGrowableStack: Re-introduced the OrderedCollection-based implementation. This provides a dynamic stack that doesn't require a fixed initial capacity, fulfilling the request for the old implementation style.
  2. CTTrackedGrowableStack: Added a subclass that introduces a maxDepth instance variable. It overrides push: to track the high-water mark of the stack's size during its lifecycle.
  3. Testing:
    • Created CTGrowableStackTest for basic LIFO verification.
    • Created CTTrackedGrowableStackTest to verify that maxDepth correctly records the peak size even after elements are popped.

and all tests are green!

image

@jordanmontt
Copy link
Copy Markdown
Member

I like this PR!

Could you add more tests to test the internals?
For example, we need to test explicitly that the CTGrowableStack actually grows its internal array (like ordered collection). If the initial size if 4 and we add 5 elements the new size should be 8 (because we double the size, like in ordered collection).

The same for CTTrackedGrowableStack we need to tests the internals as well of the external API

:)

@HossamSaberr
Copy link
Copy Markdown
Author

HossamSaberr commented Mar 24, 2026

I have added the internal tests, I wrote tests that verify the internal OrderedCollection's capacity successfully doubles from 4 to 8 when the 5th element is added
To achieve this without altering the public API, I used instVarNamed: in the test to strictly verify the internal array's behavior

All Green!

Copy link
Copy Markdown

@virenvarma007 virenvarma007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this PR.
You addressed Jordan’s request with explicit internal tests for capacity growth from 4 to 8 on the 5th push in both CTGrowableStack and CTTrackedGrowableStack, plus good max-depth coverage.
I ran all tests locally and they passed, including one extra personal local test.

Minor non-blocking nit: remove the unused initialCapacity temp in CTGrowableStackTest>>testInternalGrowth.

Looks good to approve.

@HossamSaberr
Copy link
Copy Markdown
Author

Thanks for the review and the approval! I just pushed a quick commit to remove that unused initialCapacity variable.

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.

May be we could keep the old implementation as CTGrowableStack

3 participants