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

IterableAsList is very ineffective #157

Closed
yegor256 opened this issue Jun 16, 2017 · 6 comments
Closed

IterableAsList is very ineffective #157

yegor256 opened this issue Jun 16, 2017 · 6 comments

Comments

@yegor256
Copy link
Owner

The implementation of IterableAsList is very wrong, performance wise. Let's implement it similar to what IterableAsMap is doing.

@Timmeey
Copy link
Contributor

Timmeey commented Jun 16, 2017

@yegor256 how would you change it, if it should still be able to reflect changes in the underlying Iterable?

IterableAsMap suffers from the same problem. It initializes itself on the first call, but any change after that, will not be reflected anymore

    @Test
    public void iterableAsMapReflectsUnderlyingChanges() {
        Map<String, String> underlyingMap = new HashMap<String, String>();
        Map<String, String> map = new IterableAsMap<String, String>
            (underlyingMap.entrySet());

        map.get("");
        underlyingMap.put("TestKey", "TestValue");
        MatcherAssert.assertThat("Did not contain Key",
            map.get("TestKey"), Matchers.is("TestValue"));
    }

@akryvtsun
Copy link
Contributor

Currently IterableAsMap is immutable. Was this design done intentionally?

@yegor256
Copy link
Owner Author

@Timmeey yes, this is how it should be -- they must be immutable.

@Timmeey
Copy link
Contributor

Timmeey commented Jun 17, 2017

@yegor256 are you arguing that you should not be able to add/remove elements from lists, but only create new lists?

From what i see here, the underlying list is not part of the state of the IterableAsList but rather part of its behaviour.

alex-semenyuk added a commit to alex-semenyuk/cactoos that referenced this issue Jun 19, 2017
@yegor256
Copy link
Owner Author

fixed in #175

@yegor256
Copy link
Owner Author

@Timmeey you were right, see #180

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

No branches or pull requests

3 participants