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

Confusing singleWhereOrNull extension method #294

Open
jaripekkala opened this issue Jun 20, 2023 · 7 comments
Open

Confusing singleWhereOrNull extension method #294

jaripekkala opened this issue Jun 20, 2023 · 7 comments

Comments

@jaripekkala
Copy link

https://github.com/dart-lang/collection/blob/v1.17.2/lib/src/iterable_extensions.dart#L296-L304

My proposal: Deprecate and delete singleWhereOrNull, and introduce more clear methods

Reasoning:
Many use the singleWhere method as replacement to firstWhere when the list should not contain more than one matching value, in cases when the logic would not work if code somewhere later changes in way that there are multiple matching values, so throwing error would make sense (developer need to go and handle the case).

But when also no matching value is acceptable, developer can mistakenly use singleWhereOrNull and have different behaviour than intended. These cases are not so commonly unit tested, as these cases are preventing mistakes in the future.

@lrhn
Copy link
Member

lrhn commented Feb 29, 2024

I can see how the singleWhereOrNull behavior might not be what someone wants, if they're really asking for a zeroOrOneWhere: If there is none, give me null, if there is one, give me it, if there are more, something is seriously wrong, so throw.

I do think that elements.singleWhereOrNull(test) should behave like elements.where(test).singleOrNull, so maybe we just need to add another method to cover the zeroOrOneWhere/optionalSingleWhere case.

@mattsimgithub
Copy link

Chiming in that the implementation of this method is confusing, for the reasons discussed here. I expect a single... method to throw if multiple matches are found.

@lrhn
Copy link
Member

lrhn commented Jun 7, 2024

I expect a single... method to throw if multiple matches are found.

Do you expect single... method to throw if zero matches are found? The single getter and singleWhere function does.

The singleOrNull and singleWhereOrNull both return null where the other variant would have thrown.
(The "or Null" doesn't mean "or None". It means "or return Null if a single value isn't found.").

@mattsimgithub
Copy link

Do you expect single... method to throw if zero matches are found? The single getter and singleWhere function does.

Not if the suffix is OrNull. My expectation was the OrNull would apply in the case of zero matches, but not in the case of multiple matches.

(The "or Null" doesn't mean "or None". It means "or return Null if a single value isn't found.").

Understood, that is what it does. It's not what I expected, and I'm glad I read the docs.

What you call zeroOrOneWhere, I want commonly. What singleWhereOrNull actually does, I doubt I will ever use.

I agree with jaripekkala that the behavior of this method is confusing, and I think it's likely that others are wrongly expecting singleWhereOrNull to throw on multiple matches. But whether we are common in this or outliers I don't know.

@tp
Copy link

tp commented Jun 10, 2024

I was also just confused by the difference in behavior of .single in core and .singleOrNull in collection.

I would've expected both to throw when the collection has 2 or more elements, but only single does.

Thus I updated our code to an internal extension for singleOrNull that would behave as I expect (e.g. isEmpty ? null : single).

Do you expect single... method to throw if zero matches are found? The single getter and singleWhere function does.

Yes, and I think that (for me) goes back to the .single in C#/LINQ. I would just not expect different single* methods to behave this different. (On a side-note I was also bitten by singleWhere(…) when I wanted the behavior of .where(…).single.)

I really don't get the use-case for "oh, we have 2 (or more) matching elements, let's work with none of them)", whereas "uh oh, we have more than the 1 expected element" is something I want to guard against all the time.

@gnprice
Copy link

gnprice commented Jun 11, 2024

I really don't get the use-case for "oh, we have 2 (or more) matching elements, let's work with none of them)", whereas "uh oh, we have more than the 1 expected element" is something I want to guard against all the time.

This is the key point for me.

Personally, I find the reasoning at #294 (comment) for why a method called singleWhereOrNull behaves the way it does makes perfect sense.

But I think it's pretty rare to be writing code where that's the behavior one actually wants; I'm at a bit of a loss to think up an example. Given that use cases for the zeroOrOneWhere behavior are so much more common, I suspect that out of however many times people have used singleWhereOrNull in the wild, the majority of them were because the developer thought they were getting the zeroOrOneWhere behavior.

I'd suggest, in addition to adding a zeroOrOneWhere (or optionalSingleWhere), going so far as to put a prominent note in the singleWhereOrNull documentation pointing out zeroOrOneWhere and noting the difference, with the subtext that that's the one you're probably looking for.

@tatumizer
Copy link

tatumizer commented Jun 11, 2024

going so far as to put a prominent note in the singleWhereOrNull documentation

Instead of a prominent note, why not introduce an (optional) parameter like throwOnMany (boolean)? The presence of a parameter is (arguably) more prominent than the note. :-)
(The name of the parameter needs some bikeshedding).

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

No branches or pull requests

6 participants