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

Add Iterable.elementAtOrNull extension #217

Merged
merged 6 commits into from Jun 3, 2022

Conversation

windrunner414
Copy link
Contributor

Add a extension function for List getOrNull(int index)
returns a element at the given [index] of this list, if [index] out of bounds, return null

Example:

final list = [1,2,3];
list.getOrNull(1); // 2
list.getOrNull(10); // null

@google-cla
Copy link

google-cla bot commented Sep 17, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Sep 17, 2021
@kevmoo kevmoo requested a review from lrhn September 17, 2021 16:00
@kevmoo
Copy link
Member

kevmoo commented Sep 17, 2021

At a minimum, we should have a test here.

Not sure about the overall usefulness of this extension, though. CC @natebosch

@rakudrama
Copy link
Member

I don't think the method is sufficiently useful.
Having indexes be out of bounds is often a symptom of some logical error elsewhere.

    list.getNoOrNull(i)

has the same null-on-out-of-bounds behaviour as

    list.asMap()[i]

but the latter is shorter.

@windrunner414
Copy link
Contributor Author

I think asMap() has more expensive, if the list is very big.
with nnbd, we must check if the return value is null, so it can't hide the logical error (we know the index may be out of bounds so we use it)
one case is I use list to pass the arguments between javascript and dart in webview, and some positional arguments is optional.args.getOrNull(2) ?? true is more convenience
and the other case is use it with split, like: 'a,b,c'.split(',').getOrNull(3) ?? 'd'

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

I think asMap() has more expensive, if the list is very big.

asMap() returns a ListMapView. It is a view of the list, not a copy of the data, so the cost is independent of the list size.

@windrunner414
Copy link
Contributor Author

I think asMap() has more expensive, if the list is very big.

asMap() returns a ListMapView. It is a view of the list, not a copy of the data, so the cost is independent of the list size.

I see.I mistakenly think that only the constant list will use ListMapView.I find that ListMapView is always used.

@natebosch
Copy link
Member

+1 on this being too specialized to put into a core package.

If we do decide to land this functionality it should be called elementAtOrNull to match the existing method and to avoid using the ambiguous term "get".

https://api.dart.dev/stable/2.14.2/dart-core/Iterable/elementAt.html
https://dart.dev/guides/language/effective-dart/design#avoid-starting-a-method-name-with-get

@windrunner414
Copy link
Contributor Author

list.asMap()[0] maybe a good solution instead of it.Even though it still has some extra costs.
One thing I think is that elementAtOrNull and firstOrNull are similar.Maybe it would be better to extend on iterable.
.length()/.elementAt may not efficient, and asMap() is impossible.

@lrhn
Copy link
Member

lrhn commented Sep 20, 2021

The firstOrNull and lastOrNull work well because the only failure mode is the list being empty, and therefore there being no first/last element. They also work well on all iterables.

An elementAtOrNull(int index) is more complicated. Not a lot, but still a little. You can fail to have the element by passing too big a number, but also if you pass a negative number. I think the latter should still throw. Then elementAtOrNull(index) is just equivalent to .skip(index).firstOrNull.
(The .asMap()[index] only works for lists, elementAtOrNull should work for any iterable).

So, I'm still hesitant to whether it's really needed, but if so, an extension on Iterable<E> like E elementAtOrNull(int index) => skip(RangeError.checkNotNegative(index)).firstOrNull; would make the most sense. Possibly with optimized versions on List/Set/Queue or other classes with efficient length or direct indexing.

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 8, 2021
@windrunner414
Copy link
Contributor Author

@lrhn I just add elementAtOrNull for Iterable and provide a optimized version on List.I'm not sure it makes sense to add this method to Set. And we don't need checkNotNegative because skip and [] will check it

lib/src/iterable_extensions.dart Show resolved Hide resolved
lib/src/iterable_extensions.dart Outdated Show resolved Hide resolved
@lrhn
Copy link
Member

lrhn commented Apr 20, 2022

(This fells like something I'd want to solve generally, instead of introducing extension methods for each possible way to access a list.

Maybe we could create a List<T> wrapper, implementing List<T?>, which doesn't throw on invalid indices or similar state errors, but returns null instead. It would still have a length, just wouldn't check it as much, and would reject any modifications through the wrapper.
Or, better, wait for View-types and have a zero-overhead wrapper.)

Until then, this works.

@natebosch
Copy link
Member

Until then, this works.

Do you mean you'd like to go forward with this change? Should I start getting it ready to land?

@lrhn
Copy link
Member

lrhn commented May 25, 2022

Yes, it's good. Get ready to land!

Not waiting for view types.

@natebosch natebosch changed the title Add List.getOrNull extension Add Iterable.elementAtOrNull extension May 26, 2022
@natebosch
Copy link
Member

I've applied the feedback so far. This is ready for another round of review.

///
/// The [index] must not be negative.
E? elementAtOrNull(int index) => (index < length) ? this[index] : null;

Copy link
Member

Choose a reason for hiding this comment

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

Curious that there is no firstOrNull/lastOrNull defined on List.

Copy link
Member

@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.

LGTM!

@@ -1,5 +1,5 @@
name: collection
version: 1.16.1-dev
Copy link
Member

Choose a reason for hiding this comment

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

It's OK to go directly to 1.17.0 and publish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants