Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Fix issue 355 added get frequency getter to iterable #357

Closed
35 changes: 35 additions & 0 deletions lib/src/iterable_extensions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,41 @@ extension IterableExtension<T> on Iterable<T> {
yield slice;
}
}

/// The count of occurrences of each element.
///
/// The map contains an entry for each unique element of this iterable,
/// as determined by `==`.
/// The value for each key is the number of times that element
/// appears in the iterable.
///
/// If there are elements that are equal (`==`),
/// but not identical (`identical`),
/// it is unspecified which of the elements is used as the key in the map.
/// For example, if there are multiple lists with the same content,
/// the map will only keep one of them as a key.
Copy link
Contributor

@lrhn lrhn Oct 1, 2024

Choose a reason for hiding this comment

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

Not a good example, though, since list equality is identity. Same below ...

///
/// Example:
/// ```dart
/// ['a', 'b', 'c', 'b', 'c', 'c'].frequencies;
/// Returns: {'a': 1, 'b': 2, 'c': 3}.
/// ```
///
/// Note: This method uses `==` to compare elements.
/// For collections # `List`, `Set`, or `Map`, deep equality is not checked.
Copy link
Contributor

@lrhn lrhn Oct 1, 2024

Choose a reason for hiding this comment

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

... where not even shallow equality is checked, two lists are equal only if they are the same instance.

If that is a use-case you want to support, maybe we should to add a parameter for the equality, to allow you to ask for const ListEquality() for the elements:

Map<T, int> frequencies([Equality<T>? equality]) {
  var frequenceyMap = equality == null ? <T, int>{} : 
     equality is IdentityEquality ? LinkedHashMap<T, int>.identity() : 
     LinkedHashMap<T, int>(equality.equals, equality.hash);
  // ...
}

or have a parallel ... frequenciesBy(Equality<T> equality) ....

Then you can do nested list equality as .frequenciesBy(const ListEquality(ListEquality())).

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 this is "shallow" as it relates to this, not as it relates to the elements of this that are also collection typed.

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK. In that case the wording is at least ambiguous.
Consider:

/// For elements that are collections, like `List`, `Set` or `Map`, the `==` comparison only
/// checks the identity of the collection, not its contents.

/// If you need deep equality (e.g., nested lists),
/// consider using a custom equality mechanism.

Map<T, int> get frequencies {
final frequencyMap = <T, int>{};
for (var item in this) {
frequencyMap.update(item, _increment, ifAbsent: _one);
}
return frequencyMap;
}

static int _increment(int value) => value + 1;
static int _one() => 1;
}

/// Extensions that apply to iterables with a nullable element type.
Expand Down
129 changes: 129 additions & 0 deletions test/extensions_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:collection';
import 'dart:math' show Random, pow;

import 'package:collection/collection.dart';
Expand Down Expand Up @@ -1352,6 +1353,122 @@ void main() {
expect(l3.toList(), [4, 5]);
});
});
group('get frequencies tests', () {
Copy link
Contributor

@lrhn lrhn Oct 1, 2024

Choose a reason for hiding this comment

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

Just 'frequenices', it's cleaner.

The names of groups and tests are concatenated with a space between them, so they can rely on context. Things like "should be correct" is a given for tests, so it doesn't add value. Consider:

    group('frequencies', () { 
      test('of integers', () { ... });
      test('of strings', () { ... });
      test('of empty iterable', () { ... });
      test('of empty single element iterable', () { ... });
      group('(when equality is not identity)', () {
        test('of records', () { ... });
        /// ...
      });
   });

These names are only important when a test fails.
At that point, they should give enough context to make the error message meaningful.

Take "frequencies of single element iterable" with an error (if the result was empty) of

  Expected: {42: 1}
    Actual: {}
     Which: has different length and is missing map key <42>

With that headline, it's fairly clear where and what the problem is.

test('should return correct frequency map for List of integers', () {
var list = [1, 2, 2, 3, 3, 3];
var frequencyMap = list.frequencies;
expect(frequencyMap, {1: 1, 2: 2, 3: 3});
});

test('should return correct frequency map for List of strings', () {
var list = ['a', 'b', 'b', 'c', 'c', 'c'];
var frequencyMap = list.frequencies;
expect(frequencyMap, {'a': 1, 'b': 2, 'c': 3});
});

test('should handle empty List', () {
var list = [];
var frequencyMap = list.frequencies;
expect(frequencyMap, {});
Copy link
Contributor

@lrhn lrhn Oct 1, 2024

Choose a reason for hiding this comment

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

Consider isEmpty as matcher. I think it works for maps too, and it should a more precise an error message like "is not empty".

});

test('should handle single element List', () {
var list = [42];
var frequencyMap = list.frequencies;
expect(frequencyMap, {42: 1});
});

test('should return correct frequency map for Set of integers', () {
// ignore: equal_elements_in_set
var set = {1, 2, 2, 3, 3, 3};
var frequencyMap = set.frequencies;
expect(frequencyMap, {1: 1, 2: 1, 3: 1});
});

test('should return correct frequency map for Set of strings', () {
// ignore: equal_elements_in_set
var set = {'a', 'b', 'b', 'c', 'c', 'c'};
var frequencyMap = set.frequencies;
expect(frequencyMap, {'a': 1, 'b': 1, 'c': 1});
});

test('should handle empty Set', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would drop the tests for sets and queues unless there is a reason to believe that the implementation treats them differently (which it doesn't).

We're not testing iteration, that is assumed to work, so all that matters is the sequence of values we get by iterating, and a List is fine for that.

(If we start trying to detect lists or sets, to do more efficient iteration or maybe assuming that sets cannot have duplicates so we don't need to count - which isn't true because sets can have non-standard equality - then there is reason to test with, and without, those types specifically.)

var set = <int>{};
var frequencyMap = set.frequencies;
expect(frequencyMap, {});
});

test('should handle single element Set', () {
var set = {42};
var frequencyMap = set.frequencies;
expect(frequencyMap, {42: 1});
});

test('should return correct frequency map for Queue of integers', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably overkill to go throguh all kinds of iterables.
I'd rather have more diversity in the elements.

For example having elements that are equal, but not identical, or having elements that are records.
It's the building of the map that is tricky, iterating the iterable will probably work.

Copy link
Author

Choose a reason for hiding this comment

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

added get frequencies tests extended group

var queue = Queue<int>();
queue.addAll([1, 2, 2, 3, 3, 3]);
var frequencyMap = queue.frequencies;
expect(frequencyMap, {1: 1, 2: 2, 3: 3});
});

test('should return correct frequency map for Queue of strings', () {
var queue = Queue<String>();
queue.addAll(['a', 'b', 'b', 'c', 'c', 'c']);
var frequencyMap = queue.frequencies;
expect(frequencyMap, {'a': 1, 'b': 2, 'c': 3});
});

test('should handle empty Queue', () {
var queue = Queue<int>();
var frequencyMap = queue.frequencies;
expect(frequencyMap, {});
});

test('should handle single element Queue', () {
var queue = Queue<int>();
queue.add(42);
var frequencyMap = queue.frequencies;
expect(frequencyMap, {42: 1});
});
});

group('get frequencies tests extended', () {
test('list of equal but not identical strings', () {
var list = ['apple', String.fromCharCodes('apple'.codeUnits)];
Copy link
Contributor

@lrhn lrhn Oct 1, 2024

Choose a reason for hiding this comment

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

String is not a good choice for this test because those will be identical when compiled for web.

I'd drop this one, and just use the MyObject test below.
(Or keep this, but document that identity may differ between platforms.)

var frequencyMap = list.frequencies;
expect(frequencyMap, {'apple': 2});
});

test('list of records', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

(So for naming, I'd drop the "list of" here, what is important is that it's records being compared, and it being a list isn't important to the test.
Maybe just test('of records', () { ... }); with the changes above.)

var list = [(1, 'a'), (1, 'a'), (2, 'b'), (1, 'a')];
var frequencyMap = list.frequencies;
expect(frequencyMap, {(1, 'a'): 3, (2, 'b'): 1});
});

test('list with elements that are objects', () {
var list = [const MyObject(1), const MyObject(1), const MyObject(2)];
Copy link
Contributor

@lrhn lrhn Oct 1, 2024

Choose a reason for hiding this comment

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

Don't use const here, because that makes the objects actually identical, so it doesn't check equality being different from identitiy.
Or have one const and one not.
Or add a second field to MyObject, where only one, say the first one is used for equality: [const MyObject(1, "a"), const MyObject(1, "b") ...
would be equal but not identical.

var frequencyMap = list.frequencies;
expect(frequencyMap, {const MyObject(1): 2, const MyObject(2): 1});
});

test('list with equal numbers but different types', () {
Copy link
Contributor

@lrhn lrhn Oct 1, 2024

Choose a reason for hiding this comment

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

Good test. There is no way around web considering them all identical here, but different classes being equal is an important test for native numbers.

(I was about to suggest testing with double.nan, but ... don't. You can't look it up in the map afterwards, and the default implementation of Map.entries actually looks up the value, so you can't assume to be able to even inspect the returned map.)

var list = [1, 1.0, 1, 1.0];
var frequencyMap = list.frequencies;
expect(frequencyMap, {1: 4,});
});

test('list with mixed data types', () {
var list = [1, 'one', true, 1, 'one', false];
var frequencyMap = list.frequencies;
expect(frequencyMap, {1: 2, 'one': 2, true: 1, false: 1});
});

test('list with null values', () {
var list = [null, null, 1, 'null'];
var frequencyMap = list.frequencies;
expect(frequencyMap, {null: 2, 1: 1, 'null': 1});
});
});
});

group('Comparator', () {
Expand Down Expand Up @@ -2046,6 +2163,18 @@ void main() {
});
}

class MyObject {
final int id;
const MyObject(this.id);

@override
bool operator ==(Object other) =>
identical(this, other) || other is MyObject && id == other.id;

@override
int get hashCode => id.hashCode;
}

/// Creates a plain iterable not implementing any other class.
Iterable<T> iterable<T>(Iterable<T> values) sync* {
yield* values;
Expand Down
Loading