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
wants to merge 12 commits into from
Closed

Fix issue 355 added get frequency getter to iterable #357

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ddcad5b
issue-335 countFrequency method added
pvlKryu Aug 2, 2024
c2a7133
issue-335 countFrequency tests added
pvlKryu Aug 2, 2024
0c833d8
issue-335 countFrequency comments edited
pvlKryu Aug 2, 2024
b444e37
Update lib/src/iterable_extensions.dart
pvlKryu Sep 2, 2024
1fe3d16
issue-335 converted countFrequency to getter
pvlKryu Sep 29, 2024
c5e18a2
Merge branch 'fix-issue-355-countFrequency-method' of https://github.…
pvlKryu Sep 29, 2024
8acd858
issue-335 avoided two lookups per item
pvlKryu Sep 29, 2024
facd926
Revert "issue-335 converted countFrequency to getter"
pvlKryu Sep 29, 2024
f4bbe43
issue-335 converted countFrequency to getter
pvlKryu Sep 29, 2024
9c4cf47
issue-335 returned the desired string length back
pvlKryu Sep 29, 2024
da963a5
issue-335 corrected the description in the comment
pvlKryu Oct 1, 2024
2373566
issue-335 added 'get frequencies tests extended' group
pvlKryu Oct 1, 2024
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
16 changes: 16 additions & 0 deletions lib/src/iterable_extensions.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,22 @@ extension IterableExtension<T> on Iterable<T> {
yield slice;
}
}

/// Returns a map where the keys are the unique elements of the iterable
Copy link
Contributor

Choose a reason for hiding this comment

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

Single line first paragraph. Format as sentences.
Don't use Returns for a getter. (Don't start any DartDoc with it, there is always a better word).

I'd say:

  /// The count of occurrences of each element.
  ///
  /// The map has an entry for each distinct element of this iterable,
  /// as determined by `==`, where the value is the number of eelements
  /// in this iterable which are equal to the key. 
  /// If there are elements that are equal, but not identical, its unspecified
  /// which of the elements is used as the key.
  ///
  /// For example `['a', 'b', 'c', 'b', 'c', 'c'].frequencies` 
  /// is a map with entries like `{'a': , 'b': 2, 'c': 3}`.

Copy link
Author

Choose a reason for hiding this comment

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

corrected

/// and the values are the counts of those elements.
///
/// for example, ['a', 'b', 'b', 'c', 'c', 'c'].frequencies;
/// returns {'a': 1, 'b': 2, 'c': 3}.
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
79 changes: 79 additions & 0 deletions test/extensions_test.dart
View file
Open in desktop
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,84 @@ void main() {
expect(l3.toList(), [4, 5]);
});
});
group('get frequencies tests', () {
Copy link
Contributor

@lrhn lrhn Oct 1, 2024 •
edited
Loading

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 •
edited
Loading

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('Comparator', () {
Expand Down
Loading