Skip to content
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

Implement Stack.takeWhile() #1639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Saurabh-Daware
Copy link
Contributor

No description provided.

@Saurabh-Daware
Copy link
Contributor Author

Requesting review @motlin. Thanks!

@motlin
Copy link
Contributor

motlin commented Jul 4, 2024

The implementation is probably good, but the implementations of StackIterable.takeWhile() have no test coverage.

@Saurabh-Daware
Copy link
Contributor Author

I tried writing these tests in StackIteratable.java

@Test
    public void takeWhile(){
        StackIterable<Integer> stack = this.newStackWith(1, 2, 3, 4, 5, 6, 7);
        assertEquals( this.newStackWith(), stack.takeWhile(Predicates.alwaysFalse()));
        assertEquals(this.newStackWith(1), stack.takeWhile(each -> each <= 1));
        assertEquals(this.newStackWith(1, 2), stack.takeWhile(each -> each <= 2));
        assertEquals(this.newStackWith(1, 2, 3, 4, 5, 6), stack.takeWhile(each -> each <= stack.size() - 1));
        assertEquals(this.newStackWith(1, 2, 3, 4, 5, 6, 7), stack.takeWhile(each -> each <= stack.size()));
        assertEquals(this.newStackWith(1, 2, 3, 4, 5, 6, 7), stack.takeWhile(Predicates.alwaysTrue()));
    }

but they return

[ERROR] Failures: 
[ERROR]   ImmutableArrayStackTest>StackIterableTestCase.takeWhile:226 expected:<[1]> but was:<[]>
[ERROR]   ArrayStackTest>StackIterableTestCase.takeWhile:226 expected:<[1]> but was:<[]>
[ERROR]   SynchronizedStackTest>StackIterableTestCase.takeWhile:226 expected:<[1]> but was:<[]>
[ERROR]   UnmodifiableStackTest>StackIterableTestCase.takeWhile:226 expected:<[1]> but was:<[]>

What might be the problem?

@Saurabh-Daware Saurabh-Daware force-pushed the stack-takewhile branch 4 times, most recently from 3d70e30 to 8f4f458 Compare July 5, 2024 21:53
@Saurabh-Daware
Copy link
Contributor Author

[ERROR] Failures: 
[ERROR]   ImmutableArrayStackTest>StackIterableTestCase.takeWhile:226 expected:<[1]> but was:<[]>
[ERROR]   ArrayStackTest>StackIterableTestCase.takeWhile:226 expected:<[1]> but was:<[]>
[ERROR]   SynchronizedStackTest>StackIterableTestCase.takeWhile:226 expected:<[1]> but was:<[]>
[ERROR]   UnmodifiableStackTest>StackIterableTestCase.takeWhile:226 expected:<[1]> but was:<[]>

@motlin When Stack.takeWhile() was implemented using LazyIterator, returned Stack was always null. Do we have an issue with LazyIterator.takeWhile()?

@motlin
Copy link
Contributor

motlin commented Jul 30, 2024

LGTM now, probably just needs the two commits squashed into one.

@Saurabh-Daware
Copy link
Contributor Author

Done.


public ReverseIterable<T> asReversed()
{
return ReverseIterable.adapt(this.toList());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't add asReversed here. It's not part of the public API (no @OverRide) here. as* methods are supposed to be O(1) and this implementation is O(2). Anyway, it's not necessary. We can use asLazy() instead.

assertEquals(this.newStackWith(1, 2), stack.takeWhile(Predicates.lessThanOrEqualTo(2)));
assertEquals(this.newStackWith(1, 2, 3, 4, 5, 6), stack.takeWhile(Predicates.lessThanOrEqualTo(stack.size() - 1)));
assertEquals(this.newStackWith(1, 2, 3, 4, 5, 6, 7), stack.takeWhile(Predicates.lessThanOrEqualTo(stack.size())));
assertEquals(this.newStackWith(1, 2, 3, 4, 5, 6, 7), stack.takeWhile(Predicates.alwaysTrue()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there aren't any tests of empty stacks, so ImmutableEmptyStack won't have coverage.

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.

2 participants