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<K, V> should extend Collection<K> #326

Closed
DartBot opened this issue Nov 3, 2011 · 8 comments
Closed

Map<K, V> should extend Collection<K> #326

DartBot opened this issue Nov 3, 2011 · 8 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue

Comments

@DartBot
Copy link

DartBot commented Nov 3, 2011

This issue was originally filed by @seaneagan


Having Map<K, V> extend Collection<K> would obviate the need for...

  1. Map.forEach(K, V):

map.forEach((key, value) => foo(key, value);
// becomes
map.forEach((key) => foo(key, map[key]);

  1. Map.getKeys:

map.getKeys().{every,some,filter,etc.}(...)
// becomes
map.{every,some,filter,etc.}(...)

  1. Map.{length,isEmpty}

These just become Collection.{length,isEmpty}

This would be familiar to JS programmers since JS objects are akin to dart Map<String, Dynamic>'s, and JS has:

for(key in obj) ...

Also, it would make sense to add:

bool contains(E);

to Collection<E>, which in conjunction with this proposal would replace Map.containsKey.

Another alternative would be to have Map<K, V> extend Collection<Pair<K, V>> where Pair represents key-value pairs, but that seems more complex / less desirable.

@DartBot
Copy link
Author

DartBot commented Nov 4, 2011

This comment was originally written by drfibonacci@google.com


Added Area-Library, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Nov 4, 2011

This comment was originally written by ladicek@gmail.com


While this would make some things easier, I don't think that this is the right thing to do. Is map really a collection? I certainly don't think that it is a collection of its keys, nor do I think that it is a collection of key-value pairs (although this makes some sense).

If it is worth something, I vote against this proposal. I don't think that the savings are worth the (possible) confusion.

@DartBot
Copy link
Author

DartBot commented Nov 4, 2011

This comment was originally written by @seaneagan


According to http://en.wikipedia.org/wiki/Collection_(computing):

"a collection is a grouping of some variable number of data items (possibly zero) that have some shared significance to the problem being solved and need to be operated upon together in some controlled fashion"

It further mentions an example of a collection being the "associative array" (http://en.wikipedia.org/wiki/Associative_array) which is "also called a map or a dictionary".

The Collection interface provides common methods to "operate upon" the "data items" of collections. The "data items" for maps are "associations" or "ordered pairs". This argues for Map<K,V> to extend something like Collection<Pair<K,V>> rather than Collection<K>. However, the Collection interface really only consists of convenience methods for Iterables, i.e. all Iterables could be Collections if there were an easy way to "mixin" the implementation of the Collection methods. Thus, it makes sense for the generic type to correspond to whatever is most useful to iterate over. Since Map defines the [] operator to get the value of a key, there really is no need to iterate over key-value pairs, iterating over the keys is simpler, more convenient, and probably more efficient since no Pair objects need to be created.

Probably the best evidence that Map wants to be a Collection is that it defines "length", "isEmpty", and a variant of "forEach", all of which are in Collection. It doesn't define "some", "every", and "filter" probably just to avoid bloating the interface with slightly modified variants of the Collection methods, and this makes it difficult to remember which types implement them.

@DartBot
Copy link
Author

DartBot commented Nov 5, 2011

This comment was originally written by ladicek@gmail.com


You might also argue that map is in fact two collections at once: a collection of keys and a collection of values. That is particularly interesting when considering a "bimap" in which you can get values by keys and also keys by values.

I'd say that probably the best evidence that Map shouldn't be a Collection is the fact that there are too many ways of doing it and all of them makes some sense (either from theoretical or practical point of view). You can have your collection of keys just by calling .getKeys(), that isn't too hard (and it probably could be a getter, that would make it even more easier).

@DartBot
Copy link
Author

DartBot commented Nov 15, 2011

This comment was originally written by Yegor.Jba...@gmail.com


#­1 Implies that keys are unique within the map. Then wouldn't it be more correct to say that Map<K, V> should extend Set<K>?

If Map<K, V> extends Collection<Pair<K, V>>, it makes it easier to extend Map to Multimap.

@DartBot
Copy link
Author

DartBot commented Nov 16, 2011

This comment was originally written by @seaneagan


@4: Having Map<K, V> extend Collection<K> actually supports "BiMap" as a subtype of Map quite well. BiMap would just need to specialize the return type of "getValues()" (which could be shortened to "values") to BiMap<V, K>.

@5: Having Map<K, V> extend Set<K> or Set<Pair<K, V>> would be awesome. If MultiMap<K, V> were added, it should be in between Set<Pair<K, V>> and Map<K, V>:

interface MultiMap<K, V> extends Set<Pair<K, V>> {
  MultiMap<V, K> get inverse();
  Collection<K> get keys();
  Collection<V> get values();
}

interface Map<K, V> extends MultiMap<K, V> {
  Set<K> get keys(); // keys are unique
  //...
}

interface BiMap<K, V> extends Map<K, V> {
  BiMap<V, K> get inverse(); // inverse is also a BiMap (and thus Map)
  Set<V> get values(); // values are unique
}

@lrhn
Copy link
Member

lrhn commented Sep 4, 2012

Map is not a collection. Its keys might be a Set. I'd prefer to have a Set view of the keys, backed by the Map. Example:
  class Map<K,V> {
    ...
    /** Returns a live view of the keys of this [Map].*/
    Set<K> get keys;
    /// ditto values
    Collection<V> get values;
  }

Also: Favor composition over inheritance!
A Map has a set of keys, it is not a set.

A view as Set of Pair is also possible.


Added WontFix label.

@DartBot
Copy link
Author

DartBot commented Sep 4, 2012

This comment was originally written by @seaneagan


Something like:

Set<Pair<K, V>> get pairs;

should be sufficient.

For the keys/values getters, see issue #1248.

@DartBot DartBot added Type-Defect area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Sep 4, 2012
@kevmoo kevmoo added closed-not-planned Closed as we don't intend to take action on the reported issue and removed resolution-wont_fix labels Mar 1, 2016
nex3 pushed a commit that referenced this issue Aug 31, 2016
catch parse exceptions from args
copybara-service bot pushed a commit that referenced this issue Jan 31, 2023
…g, mockito, package_config, shelf, string_scanner, test, webdev

Revisions updated by `dart tools/rev_sdk_deps.dart`.

async (https://github.com/dart-lang/async/compare/f700e9a..f700e9a):
  f700e9a  2023-01-27  Devon Carew  blast_repo fixes (#231)

characters (https://github.com/dart-lang/characters/compare/4526aa8..4526aa8):
  4526aa8  2023-01-30  Lasse R.H. Nielsen  Update tables to Unicode 15.0. (#71)

collection (https://github.com/dart-lang/collection/compare/a566328..a566328):
  a566328  2023-01-26  Devon Carew  add a publish script; prep to publish (#267)

dartdoc (https://github.com/dart-lang/dartdoc/compare/bc7bdc4..bc7bdc4):
  bc7bdc44  2023-01-30  dependabot[bot]  Bump js from 0.6.5 to 0.6.7 (#3310)

json_rpc_2 (https://github.com/dart-lang/json_rpc_2/compare/e73c4ad..e73c4ad):
  e73c4ad  2023-01-26  Devon Carew  blast_repo fixes (#89)

logging (https://github.com/dart-lang/logging/compare/399100a..399100a):
  399100a  2023-01-26  Devon Carew  add a publish script; prep to publish 1.1.1 (#128)

mockito (https://github.com/dart-lang/mockito/compare/d2a8df1..d2a8df1):
  d2a8df1  2023-01-30  Kevin Moore  Latest build_web_compilers, move to pkg:lints, fix breaks (#605)
  13340b5  2023-01-30  dependabot[bot]  Bump dart-lang/setup-dart from 1.3 to 1.4 (#600)

package_config (https://github.com/dart-lang/package_config/compare/3fe81c4..3fe81c4):
  3fe81c4  2023-01-30  Kevin Moore  Support latest pkg:build_web_compilers, lints. Update min SDK (#129)

shelf (https://github.com/dart-lang/shelf/compare/8fca9d9..8fca9d9):
  8fca9d9  2023-01-26  Devon Carew  blast_repo fixes (#326)

string_scanner (https://github.com/dart-lang/string_scanner/compare/29e471e..29e471e):
  29e471e  2023-01-30  dependabot[bot]  Bump dart-lang/setup-dart from 1.3 to 1.4 (#53)

test (https://github.com/dart-lang/test/compare/cec47c1..cec47c1):
  cec47c1c  2023-01-27  Nate Bosch  Add missing pub requirements (#1878)
  c99d455e  2023-01-27  Nate Bosch  Prepare to publish (#1877)
  0e7ec6a7  2023-01-27  Nate Bosch  Rename `Check` to `Subject` (#1875)
  78382731  2023-01-27  Nate Bosch  Add String.matches condition (#1874)
  26e0e87b  2023-01-27  Nate Bosch  Add Iterable.containsInOrder condition (#1873)
  c9232d6b  2023-01-27  Nate Bosch  Rename `that` to `which` (#1872)
  457166b3  2023-01-26  Nate Bosch  Add missing dependency on package:lints (#1876)
  193f2a0b  2023-01-26  Nate Bosch  Retry instead of extend timeout for flaky Node tests (#1871)
  7ad9b2c3  2023-01-26  Nate Bosch  Overhaul async matchers (#1868)
  ca254546  2023-01-26  Nate Bosch  Add a withQueue utility (#1870)
  6ae2e5e9  2023-01-26  Nate Bosch  Refactor tests to a new isRejectedBy utility (#1867)
  5aeba66d  2023-01-26  Nate Bosch  Use pubspec_overrides.yaml files (#1869)

webdev (https://github.com/dart-lang/webdev/compare/ce9c581..ce9c581):
  ce9c581  2023-01-29  Anna Gringauze  Validate only needed summaries in expression_compiler_service (#1920)

Change-Id: I3ddb0ddeb3b989f6f9e78cd8aa6327aba5899018
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280078
Commit-Queue: Devon Carew <devoncarew@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
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. closed-not-planned Closed as we don't intend to take action on the reported issue
Projects
None yet
Development

No branches or pull requests

3 participants