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

Add a comment about NaN in min/max extensions #234

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

slovnicki
Copy link
Contributor

I noticed that NaN will be both min and max, which might be confusing to the user if not documented.

I do think returning NaNs is better approach than the other 2 alternatives I can think of:

  • Ignoring them, which will not let user know they have NaNs in the iterable.
  • Relying on false from comparisons to NaN, which will lead to inconsistent returns depending on NaN's placement (e.g. first or not first) in the iterable.

@devoncarew devoncarew requested a review from lrhn April 6, 2022 21:42
@@ -592,6 +592,8 @@ extension IterableNullableExtension<T extends Object> on Iterable<T?> {
/// since doubles require special handling of [double.nan].
extension IterableNumberExtension on Iterable<num> {
/// A minimal element of the iterable, or `null` it the iterable is empty.
///
/// If an element is NaN, it is considered to be a minimal element.
Copy link
Contributor

@lrhn lrhn Apr 7, 2022

Choose a reason for hiding this comment

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

Hmm, pretty over-engineered code, now that I look at it again.

  num? get minOrNull {
    var empty = true;
    var min = double.infinity;
    for (var value in this) {
       empty = false;
       if (value.isNaN) return value;
       if (value < min) min = value;
    }
    if (empty) return null;
    return min;
  }

seems much more optimizable.

(Maybe I should make a benchmark 😁)

Copy link
Contributor

@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 with the different phrasing.

Copy link
Contributor

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

Looks good to me. Thanks!

@lrhn lrhn merged commit c1a07e4 into dart-archive:master Apr 8, 2022
mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 18, 2024
…n#234)

* Add a comment about the treatment of NaN in `min`/`max` extensions on number types.
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.

2 participants