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

Map's getKeys() / getValues() should be keys / values #1248

Closed
DartBot opened this issue Jan 20, 2012 · 7 comments
Closed

Map's getKeys() / getValues() should be keys / values #1248

DartBot opened this issue Jan 20, 2012 · 7 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries.

Comments

@DartBot
Copy link

DartBot commented Jan 20, 2012

This issue was originally filed by @seaneagan


Map's getKeys() and getValues() return Collections which are backed by the Map, as in Java. This means none of the Map's keys / values are actually copied. Thus, these methods are O(1). Thus, they should satisfy all the criteria in the Dart style guide (http://www.dartlang.org/articles/style-guide/) for getters:

Not take any arguments.
Return a value.
Be side-effect free.
Be fast.

The obvious names for these getters are "keys" and "values".

@dgrove
Copy link
Contributor

dgrove commented Jan 23, 2012

Added Area-Library, Triaged labels.

@dgrove
Copy link
Contributor

dgrove commented Jan 23, 2012

Issue #1265 has been merged into this issue.

@DartBot
Copy link
Author

DartBot commented Jan 23, 2012

This comment was originally written by ladicek@gmail.com


While I'd love to have keys/values getters instead of getKeys()/getValues() methods, I have to point out that this:

Map's getKeys() and getValues() return Collections which are backed by the Map, as in Java. This means none of the Map's keys / values are actually copied.

actually doesn't seem to be true. The documentation doesn't contain any mention of the keys/values collections being backed by the Map, and they are not. See corelib/src/implementation/hash_map_set.dart -- both getKeys() and getValues() in HashMapImplementation are creating a new List and copying the keys/values to it, making it also O(N).

@DartBot
Copy link
Author

DartBot commented Jan 23, 2012

This comment was originally written by @seaneagan


Hmmm, since the API clearly takes its inspiration from Java, I just assumed.

Here's why these methods need to return Collections backed by the Map:

  1. Avoids copying internal values
  2. Avoids the need to call the methods (which copy internal values, and create a new Collection), each and every time up to date keys / values are needed.
  3. Abstraction. You should be able to think of the returned Collections as abstract Collections, without needing to know the Map from which they came, in order to get up to date information.
  4. Dart's copy constructors already provide a much more orthogonal way to copy Collections, and can be composed with "keys" and "values" on the rare occasion that a "snapshot" is needed:

new List.from(map.keys) // or
new Set.from(map.keys)
// or if Collection had a default implementation (which it should)
new Collection.from(map.keys)

The same arguments apply to making Collection#filter and Collection#map (issue #945) lazy.

@DartBot
Copy link
Author

DartBot commented Jan 23, 2012

This comment was originally written by ladicek@gmail.com


I agree that the collections of keys and values provided by the Map should be only a view on top of the Map, not a snapshot -- but that's probably worth another issue.

@DartBot
Copy link
Author

DartBot commented Jan 23, 2012

This comment was originally written by @seaneagan


Agreed, see issue #1305 !

@floitschG
Copy link
Contributor

This has been fixed with libv2.


Added Fixed label.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries.
Projects
None yet
Development

No branches or pull requests

3 participants