Skip to content

Conversation

@nate-thegrate
Copy link
Contributor

@nate-thegrate nate-thegrate commented Sep 6, 2024

This pull request aims to improve code readability, based on feedback gathered in a recent design doc.


There are two factors that hugely impact how easy it is to understand a piece of code: verbosity and complexity.

Reducing verbosity is important, because boilerplate makes a project more difficult to navigate. It also has a tendency to make one's eyes gloss over, and subtle typos/bugs become more likely to slip through.

Reducing complexity makes the code more accessible to more people. This is especially important for open-source projects like Flutter, where the code is read by those who make contributions, as well as others who read through source code as they debug their own projects.



The following examples show how pattern-matching might affect these two factors:

Example 1 (GOOD)

[click to expand]
if (ancestor case InheritedElement(:final InheritedTheme widget)) {
  themes.add(widget);
}

Without using patterns, this might expand to

if (ancestor is InheritedElement) {
  final InheritedWidget widget = ancestor.widget;
  if (widget is InheritedTheme) {
    themes.add(widget);
  }
}

Had ancestor been a non-local variable, it would need to be "converted" as well:

final Element ancestor = this.ancestor;
if (ancestor is InheritedElement) {
  final InheritedWidget inheritedWidget = ancestor.widget;
  if (widget is InheritedTheme) {
    themes.add(theme);
  }
}

Example 2 (BAD)

[click to expand]
if (widget case PreferredSizeWidget(preferredSize: Size(:final double height))) {
  return height;
}

Assuming widget is a non-local variable, this would expand to:

final Widget widget = this.widget;
if (widget is PreferredSizeWidget) {
  return widget.preferredSize.height;
}

In both of the examples above, an if-case statement simultaneously verifies that an object meets the specified criteria and performs a variable assignment accordingly.

But there are some differences: Example 2 uses a more deeply-nested pattern than Example 1 but makes fewer useful checks.

Example 1:

  • checks that ancestor is an InheritedElement
  • checks that the inherited element's widget is an InheritedTheme

Example 2:

  • checks that widget is a PreferredSizeWidget
    (every PreferredSizeWidget has a size field, and every Size has a height field)


I feel hesitant to try presenting a set of cut-and-dry rules as to which scenarios should/shouldn't use pattern-matching, since there are an abundance of different types of patterns, and an abundance of different places where they might be used.

But hopefully the conversations we've had recently will help us converge toward a common intuition of how pattern-matching can best be utilized for improved readability.



@nate-thegrate nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Sep 6, 2024
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: routes Navigator, Router, and related APIs. f: focus Focus traversal, gaining or losing focus labels Sep 6, 2024
@nate-thegrate nate-thegrate marked this pull request as ready for review September 7, 2024 00:18
@matanlurey
Copy link
Contributor

I took a cursory glance (it's EOD Friday here so I'll have to look again next week) and in general this seems like an improvement, and in the couple of places where code owners might prefer the legacy style I suspect you could make 1-off adjustments.

That being said, I'm having a hard time understanding the state of the proposal; the original design has a combination of positive and negative comments, and the only other (public?) comment I see from @goderbauer implies not a strong amount of support.

If there were other outcomes or conversations you can share to explain this change, that would be great.

@nate-thegrate
Copy link
Contributor Author

Sure! I'd be happy to give an overview of what I've learned.



Hixie engaged with the doc the most, and also was the most strongly opposed to pattern-matching out of the people who responded.

His negative responses (e.g. here, here, here, and here) are summarized really well by a comment he made at the top: simplicity is more important than concision.

There were other spots where he gave a more neutral response (here, here, here, and here) along the lines of "neither one is very readable."

As far as his supportive feedback, one comment in particular stuck out:

i think non-pattern switch is probably simpler than a long if chain, because the reader can immediately see "ok we're going to just be comparing this one value against a bunch of other values" whereas with the if chain you have to check "is every condition the same form? are they actually chained with 'else' properly?" etc. When we start adding patterns to the switch though I think it can quickly get more complicated.

That said, I think there are cases where even pattern switch is better than an if chain. For example if you have a sealed class hierarchy and you're checking against every option and matching every field, that's probably clearer than a bunch of ifs and casts.


Mylo Fawcett also engaged quite a bit and showed more support overall for implementing patterns.
But one notable exception that I found surprising was the firm disapproval of doing an Offset object pattern assignment.


I was also interested to hear from Bob Nystrom that Dart pattern-matching has a lot in common with Swift syntax.




It's definitely not cut-and-dry whether pattern-matching is overall "better" or "worse" than the alternative (there are some situations where whether the pattern is more readable depends on how long the variable names are). There's a lot of variety, both with regards to how patterns would apply to a particular code snippet, and also pertaining to the individual reactions and overall opinions that different people have.

That being said, there was one huge takeaway that hadn't occurred to me until I started talking with people and gathering feedback: map patterns are far more readable than object patterns.

if (someMap case {'key': final Object value}) {
  // ...
}

It's distinct, concise, and expressive: the key being directly adjacent to the value is huge.


By comparison, object patterns are often not as great, especially with the leading colon:

  • Multiple people have mentioned that Size(:final double height) looks wonky.
  • And Size(height: final double height) is more verbose and not much less confusing.



To decide what went into this PR, I basically just included all the map patterns, and then some other things that I thought looked nice.

in the couple of places where code owners might prefer the legacy style I suspect you could make 1-off adjustments.

Sounds like a great plan!

@github-actions github-actions bot removed the f: scrolling Viewports, list views, slivers, etc. label Sep 7, 2024
Copy link
Contributor

@victorsanni victorsanni left a comment

Choose a reason for hiding this comment

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

I love how this reduces the if foo is null/T checks. Also love the switch statements, especially when they're aligned nicely. The if..case..when statements also make more sense to me now.

nate-thegrate

This comment was marked as resolved.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

So, some of the case statements gave me the heebie-jeebies initially but once I cam across something I knew really well, they made sense.

Regarding anonymous parameters, maybe we need to revisit that in the style guide, as there are a lot of them here. I wonder if they're already pervasive in the codebase and the style guide is just not reflecting reality...

I'll circulate this with the team once more so we can finally move this forward.

I always love your creative titles, but can you update it to reflect the change now? Thanks!

@auto-submit auto-submit bot merged commit 5ecf100 into flutter:master Oct 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024
@nate-thegrate nate-thegrate deleted the patterns branch October 3, 2024 22:20
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 8, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 8, 2024
Manual roll requested by [email protected]

flutter/flutter@ec2e12b...0917e9d

2024-10-05 [email protected] Roll Flutter Engine from 2068577e510b to 01de9dcf6bbf (1 revision) (flutter/flutter#156274)
2024-10-05 [email protected] Roll Flutter Engine from 6d6bc39fe8b1 to 2068577e510b (1 revision) (flutter/flutter#156267)
2024-10-05 [email protected] Roll Flutter Engine from 92b5b318190b to 6d6bc39fe8b1 (4 revisions) (flutter/flutter#156266)
2024-10-05 [email protected] Roll Flutter Engine from fd6082ab6693 to 92b5b318190b (1 revision) (flutter/flutter#156261)
2024-10-05 [email protected] Roll Flutter Engine from bc5f9fc80b06 to fd6082ab6693 (4 revisions) (flutter/flutter#156258)
2024-10-04 [email protected] Roll Flutter Engine from eece6c3ec63a to bc5f9fc80b06 (2 revisions) (flutter/flutter#156252)
2024-10-04 [email protected] Roll pub packages (flutter/flutter#156254)
2024-10-04 [email protected] Roll Flutter Engine from 9fa363bdde20 to eece6c3ec63a (1 revision) (flutter/flutter#156250)
2024-10-04 [email protected] Roll Flutter Engine from d38f5e560a98 to 9fa363bdde20 (1 revision) (flutter/flutter#156249)
2024-10-04 [email protected] remove bringup from Windows tool_integration_tests_* (flutter/flutter#156179)
2024-10-04 [email protected] Roll Flutter Engine from 205484009711 to d38f5e560a98 (4 revisions) (flutter/flutter#156239)
2024-10-04 [email protected] Normalize `ThemeData.cardTheme` (flutter/flutter#153254)
2024-10-04 [email protected] Roll Packages from 429650f to 05bf1d4 (5 revisions) (flutter/flutter#156225)
2024-10-04 [email protected] Roll Flutter Engine from e0109f0fd5b2 to 205484009711 (1 revision) (flutter/flutter#156216)
2024-10-04 [email protected] Roll pub packages (flutter/flutter#156215)
2024-10-04 [email protected] Roll Flutter Engine from c2a9fb051bd0 to e0109f0fd5b2 (1 revision) (flutter/flutter#156213)
2024-10-04 [email protected] Roll Flutter Engine from 66d397dff87a to c2a9fb051bd0 (5 revisions) (flutter/flutter#156206)
2024-10-04 [email protected] Roll Flutter Engine from 20369c5d2b93 to 66d397dff87a (5 revisions) (flutter/flutter#156188)
2024-10-03 [email protected] Roll Flutter Engine from de1762dbc5cc to 20369c5d2b93 (7 revisions) (flutter/flutter#156174)
2024-10-03 [email protected] Refactor devfs_web_ddc_modules_test.dart (flutter/flutter#155609)
2024-10-03 [email protected] Allow arrow keys to navigate `MenuAnchor` independently of global shortcut definition (flutter/flutter#155728)
2024-10-03 [email protected] Inherited Theme: zero rebuilds (flutter/flutter#155699)
2024-10-03 [email protected] Roll Flutter Engine from 247bc68c578e to de1762dbc5cc (4 revisions) (flutter/flutter#156160)
2024-10-03 [email protected] Roll pub packages (flutter/flutter#156159)
2024-10-03 [email protected] pattern-matching refactor (flutter/flutter#154753)
2024-10-03 [email protected] Add autocorrect and enableSuggestions to SearchDelegate (flutter/flutter#154932)
2024-10-03 [email protected] Roll Packages from 7c97c88 to 429650f (6 revisions) (flutter/flutter#156155)
2024-10-03 [email protected] Move some tools test ownership to Ben (flutter/flutter#156123)
2024-10-03 [email protected] further shard the Windows tool_integration_tests* targets (flutter/flutter#156121)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This was referenced Oct 9, 2024
auto-submit bot pushed a commit that referenced this pull request Oct 17, 2024
This egotistical PR aims to draw attention to a [style guideline](https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#use--for-getters-and-callbacks-that-just-return-literals-or-switch-expressions) that I changed:

> #### Use `=>` for getters and callbacks that just return literals or switch expressions

<br>

There was an opportunity for valuable discussion in #154753 about how this structure affects readability, but I shut it down pretty quick since there was a lot of other stuff going on there.

Interested to hear thoughts from @Piinks and others.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: focus Focus traversal, gaining or losing focus f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically refactor Improving readability/efficiency without behavioral changes tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Patterns in the Flutter framework