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

Add extension methods to package:collection. #135

Merged
merged 2 commits into from
Sep 30, 2020
Merged

Add extension methods to package:collection. #135

merged 2 commits into from
Sep 30, 2020

Conversation

lrhn
Copy link
Contributor

@lrhn lrhn commented May 13, 2020

Add a number of useful extension methods to List and Iterable, and to a few other types.

A good part of these extensions are backed by the algorithms of algorithms.dart, which is expanded to support them:

  • Added quickSort.
  • Add extra optional Random argument to shuffle.
  • Generalize some of the functions in algorithms.dart to work on specific properties of the objects (binarySearchBy,
    lowerBoundBy, insertionSortBy, quickSortBy, mergeSortBy).

@lrhn lrhn requested a review from natebosch May 13, 2020 13:36
@lrhn
Copy link
Contributor Author

lrhn commented May 13, 2020

This is a first crack at adding extension methods to package:collection.

They are not tested. Some are probably not useful (I expect Iterable<T extends num>.sum() to be mostly speculative).

Copy link
Contributor

@natebosch natebosch left a comment

Choose a reason for hiding this comment

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

Did something get lost in the upload? There are no extension methods added here. This does look like it might be some work towards adding extensions, but it doesn't match the PR description as is.

analysis_options.yaml Outdated Show resolved Hide resolved
analysis_options.yaml Outdated Show resolved Hide resolved
lib/src/empty_unmodifiable_set.dart Outdated Show resolved Hide resolved
lib/src/algorithms.dart Outdated Show resolved Hide resolved
@lrhn
Copy link
Contributor Author

lrhn commented May 14, 2020

Whoops. Forgot to git-add the new files.

lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved
lib/src/iterable_extensions.dart Show resolved Hide resolved
lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved
lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved
lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved
}

/// Maps each element and its index to a new value.
Iterable<R> mapIndexed<R>(R convert(int index, E element)) sync* {
Copy link
Contributor

Choose a reason for hiding this comment

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

How much value are we buying by specializing this against Iterable.mapIndexed? Do we expect this[index] to be meaningfully different (faster?) than for(var element in this)?

Copy link
Contributor Author

@lrhn lrhn May 15, 2020

Choose a reason for hiding this comment

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

Faster, but possibly not by a lot.
It has a better chance of ending up monomorphic.

lib/src/list_extensions.dart Outdated Show resolved Hide resolved
lib/src/list_extensions.dart Show resolved Hide resolved
lib/src/list_extensions.dart Show resolved Hide resolved
lib/src/list_extensions.dart Outdated Show resolved Hide resolved
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@lrhn lrhn added cla: yes and removed cla: no labels Jun 15, 2020
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@mit-mit
Copy link
Contributor

mit-mit commented Sep 17, 2020

@lrhn looks stale?

@lrhn
Copy link
Contributor Author

lrhn commented Sep 17, 2020

Was waiting for null-safety. I should be able to continue this now.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@lrhn
Copy link
Contributor Author

lrhn commented Sep 28, 2020

PTAL

@natebosch
Copy link
Contributor

I had been waiting to review after the merge conflicts were resolved... Do you want me to do a first pass and then re-review after solving the conflicts?

@lrhn
Copy link
Contributor Author

lrhn commented Sep 28, 2020

Conflicts should be gone now.

lib/src/algorithms.dart Show resolved Hide resolved
lib/src/algorithms.dart Show resolved Hide resolved
lib/src/algorithms.dart Outdated Show resolved Hide resolved
lib/src/algorithms.dart Outdated Show resolved Hide resolved
lib/src/algorithms.dart Outdated Show resolved Hide resolved
lib/src/iterable_extensions.dart Show resolved Hide resolved
lib/src/iterable_extensions.dart Show resolved Hide resolved
lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved
lib/src/iterable_extensions.dart Show resolved Hide resolved
lib/src/iterable_extensions.dart Show resolved Hide resolved
Copy link
Contributor

@natebosch natebosch left a comment

Choose a reason for hiding this comment

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

Overall I'm slightly worried about adding so many of these speculatively since there is a high cost to remove them, but given that they are extension methods I think the overall cost to keeping them is low.

CHANGELOG.md Outdated Show resolved Hide resolved
test/extensions_test.dart Outdated Show resolved Hide resolved
Add a number of useful extension methods to List and Iterable, and to a few other types.
Also add a quick-sort implementation that these can extension methods can defer to,
one which allows sorting only a range of the list.
Copy link
Contributor Author

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

Addressed comments.

lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved

/// The elements of this iterable with duplicates removed.
Iterable<T> unique() sync* {
var seen = <T>{};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably on the web, not on the VM.
On the other hand, most people use {}, so if this is the only code using HashSet, it will increase the compiled size of the web code. Dropping the method anyway.

var result = <K, Set<T>>{};
for (var element in this) {
var key = keyOf(element);
result[key] = (result[key] ?? {})..add(element);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I was trying to optimize, just ended up here and didn't recognize it could be written even shorter.

/// var parts = [1, 2, 3, 4, 5, 6, 7, 8, 9].split(isPrime);
/// print(parts); // ([1], [2], [3, 4], [5, 6], [7, 8, 9])
/// ```
Iterable<List<T>> splitBefore(bool test(T element)) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

/// The sum of the elements.
///
/// The sum is zero if the iterable is empty.
T sum() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, and was on the fence about whether naming it for the operation or the result. If it's the operation, then it should be a method.
It's a little annoying that the best verb for the operation is also the best noun for the result. (Ditto for average. Truly, any noun can be verbed.)

Let's make them getters.

lib/src/iterable_extensions.dart Show resolved Hide resolved
lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved
lib/src/iterable_extensions.dart Show resolved Hide resolved
lib/src/iterable_extensions.dart Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@lrhn lrhn merged commit 7d44763 into master Sep 30, 2020
@natebosch natebosch deleted the extensions branch September 30, 2020 16:49
mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 18, 2024
…#135)

Add a number of useful extension methods to `List` and `Iterable`, and to a few other types.

A good part of these extensions are backed by the algorithms of `algorithms.dart`, which is expanded to support them:

* Added `quickSort`.
* Add extra optional `Random` argument to `shuffle`.
* Generalize some of the functions in algorithms.dart to work on specific properties of the objects (`binarySearchBy`,
`lowerBoundBy`, `insertionSortBy`, `quickSortBy`, `mergeSortBy`).

The new methods are not exported from the library yet, they are intended to be used through the extension.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants