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.

Sign up for GitHub

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

Add new Dart 2 Map methods to BiMap #423

Merged
merged 5 commits into from
Mar 23, 2018
Merged

Add new Dart 2 Map methods to BiMap #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ba4b605
Add Dart 2 core methods for BiMap
srawlins Mar 9, 2018
fb4bb2f
analyzer
srawlins Mar 9, 2018
0e158a3
Feedback
srawlins Mar 12, 2018
f2a5dc9
Merge branch 'master' into bimap
srawlins Mar 20, 2018
bdd467b
Feedback
srawlins Mar 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#### Master

* New: BiMap now includes a real implementation of `addEntries`, `get
entries`, `map`, `removeWhere`, `update`, and `updateAll`.
* New: DelegatingIterable now includes a real implementation of
`followedBy`, and accepts the `orElse` parameter on `singleWhere`.
* New: DelegatingList now includes real implementations of `operator +`,
Expand Down
58 changes: 22 additions & 36 deletions lib/src/collection/bimap.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,13 @@ class HashBiMap<K, V> implements BiMap<K, V> {
int get length => _map.length;
Iterable<V> get values => _inverse.keys;

BiMap<V, K> get inverse => _cached ??= new HashBiMap._from(_inverse, _map);

@override
// TODO: Dart 2.0 requires this method to be implemented.
// ignore: override_on_non_overriding_method
void addEntries(Iterable<Object> entries) {
// Change Iterable<Object> to Iterable<MapEntry<K, V>> when
// the MapEntry class has been added.
throw new UnimplementedError("addEntries");
void addEntries(Iterable<MapEntry<K, V>> entries) {
for (var entry in entries) {
_add(entry.key, entry.value, false);
}
}

@override
Expand All @@ -87,29 +87,11 @@ class HashBiMap<K, V> implements BiMap<K, V> {
}

@override
// TODO: Dart 2.0 requires this method to be implemented.
// ignore: override_on_non_overriding_getter
Iterable<Null> get entries {
// Change Iterable<Null> to Iterable<MapEntry<K, V>> when
// the MapEntry class has been added.
throw new UnimplementedError("entries");
}

BiMap<V, K> get inverse {
if (_cached == null) {
_cached = new HashBiMap._from(_inverse, _map);
}
return _cached;
}
Iterable<MapEntry<K, V>> get entries => _map.entries;

@override
// TODO: Dart 2.0 requires this method to be implemented.
// ignore: override_on_non_overriding_method
Map<K2, V2> map<K2, V2>(Object transform(K key, V value)) {
// Change Object to MapEntry<K2, V2> when
// the MapEntry class has been added.
throw new UnimplementedError("map");
}
Map<K2, V2> map<K2, V2>(MapEntry<K2, V2> transform(K key, V value)) =>
_map.map(transform);
Copy link
Member

Choose a reason for hiding this comment

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

Does Map.map document the expected return type? If it's expected to be a BiMap, we need to apply the invariants from _add (i.e. that not only must the keys be unique, but so must the values, since they're the keys of the inverse).

If it's not, then I think this is fine as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't document really.

My thought is it looks just like List.map, Set.map`, ...: you don't expect a List or a Set out the other side; you just expect an Iterable.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. That sounds perfectly reasonable to me.


V putIfAbsent(K key, V ifAbsent()) {
var value = _map[key];
Expand All @@ -124,10 +106,9 @@ class HashBiMap<K, V> implements BiMap<K, V> {
}

@override
// TODO: Dart 2.0 requires this method to be implemented.
// ignore: override_on_non_overriding_method
void removeWhere(bool test(K key, V value)) {
throw new UnimplementedError("removeWhere");
_inverse.removeWhere((v, k) => test(k, v));
_map.removeWhere(test);
}

@override
Expand All @@ -138,17 +119,22 @@ class HashBiMap<K, V> implements BiMap<K, V> {
}

@override
// TODO: Dart 2.0 requires this method to be implemented.
// ignore: override_on_non_overriding_method
V update(K key, V update(V value), {V ifAbsent()}) {
throw new UnimplementedError("update");
var value = _map[key];
if (value != null) {
return _add(key, update(value), true);
} else {
if (ifAbsent == null)
throw new ArgumentError.value(key, 'key', 'Key not in map');
return _add(key, ifAbsent(), false);
}
}

@override
// TODO: Dart 2.0 requires this method to be implemented.
// ignore: override_on_non_overriding_method
void updateAll(V update(K key, V value)) {
throw new UnimplementedError("updateAll");
for (var key in this.keys) {
_add(key, update(key, _map[key]), true);
}
}

void clear() {
Expand Down
93 changes: 93 additions & 0 deletions test/collection/bimap_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,63 @@ main() {
map.remove(k1);
expect(map.containsKey(k1), false);
expect(map.inverse.containsKey(v1), false);

map[k1] = v1;
map[k2] = v2;
map.removeWhere((k, v) => v.isOdd);
expect(map.containsKey(k1), false);
expect(map.containsKey(k2), true);
expect(map.inverse.containsKey(v1), false);
expect(map.inverse.containsKey(v2), true);
});

test('should not contain mappings removed from its inverse', () {
map[k1] = v1;
map.inverse.remove(v1);
expect(map.containsKey(k1), false);
expect(map.inverse.containsKey(v1), false);

map[k1] = v1;
map[k2] = v2;
map.inverse.removeWhere((v, k) => v.isOdd);
expect(map.containsKey(k1), false);
expect(map.containsKey(k2), true);
expect(map.inverse.containsKey(v1), false);
expect(map.inverse.containsKey(v2), true);
});

test('should update both sides', () {
map[k1] = v1;
map.update(k1, (v) => v + 1);
expect(map[k1], equals(v1 + 1));
expect(map.inverse[v1 + 1], equals(k1));

map[k1] = v1;
map.inverse.update(v1, (k) => '_$k');
expect(map['_$k1'], equals(v1));
expect(map.inverse[v1], equals('_$k1'));
});

test('should update absent key values', () {
map[k1] = v1;
map.update(k2, (v) => v + 1, ifAbsent: () => 0);
expect(map[k2], equals(0));
expect(map.inverse[0], equals(k2));

map[k1] = v1;
map.inverse.update(v2, (k) => '_$k', ifAbsent: () => '_X');
expect(map['_X'], equals(v2));
expect(map.inverse[v2], equals('_X'));
});

test('should update all values', () {
map[k1] = v1;
map[k2] = v2;
map.updateAll((k, v) => v + k.length);
expect(map[k1], equals(3));
expect(map[k2], equals(4));
expect(map.inverse[3], equals(k1));
expect(map.inverse[4], equals(k2));
});

test('should be empty after clear', () {
Expand Down Expand Up @@ -230,6 +280,49 @@ main() {
expect(map.inverse.values, unorderedEquals([k1, k2]));
});

test('should add entries', () {
map.addEntries([new MapEntry<String, int>(k1, v1)]);
expect(map[k1], equals(v1));
expect(map.inverse[v1], equals(k1));

map.inverse.addEntries([new MapEntry<int, String>(v2, k2)]);
expect(map[k2], equals(v2));
expect(map.inverse[v2], equals(k2));
});

test('should get entries', () {
map[k1] = v1;
map.inverse[v2] = k2;

var mapEntries = map.entries;
expect(mapEntries, hasLength(2));
// MapEntry objects are not equal to each other; cannot use `contains`. :(
expect(mapEntries.singleWhere((e) => e.key == k1).value, equals(v1));
expect(mapEntries.singleWhere((e) => e.key == k2).value, equals(v2));

var inverseEntries = map.inverse.entries;
expect(inverseEntries, hasLength(2));
expect(inverseEntries.singleWhere((e) => e.key == v1).value, equals(k1));
expect(inverseEntries.singleWhere((e) => e.key == v2).value, equals(k2));
});

test('should map mappings', () {
map[k1] = v1;
map[k2] = v2;

var mapped = map.map((k, v) => new MapEntry(k.toUpperCase(), '$k / $v'));
expect(mapped, contains('K1'));
expect(mapped, contains('K2'));
expect(mapped['K1'], equals('k1 / 1'));
expect(mapped['K2'], equals('k2 / 2'));

var mapped2 = map.inverse.map((v, k) => new MapEntry('$v$v', k.length));
expect(mapped2, contains('11'));
expect(mapped2, contains('22'));
expect(mapped2['11'], equals(2));
expect(mapped2['22'], equals(2));
});

test('should add mappings via putIfAbsent if absent', () {
map.putIfAbsent(k1, () => v1);
expect(map[k1], v1);
Expand Down