Intensive `if` chain refactoring #145194

nate-thegrate · 2024-03-15T02:40:10Z

This pull request refactors if-statements into switch expressions, as part of the effort to solve issue #144903.

Making changes beyond just swapping syntax is more difficult (and also more difficult to review, I apologize), but much more satisfying too.

justinmc

LGTM with nits 👍 Thanks as always for doing these migrations!

dev/benchmarks/macrobenchmarks/lib/src/very_long_picture_scrolling.dart

dev/manual_tests/lib/star_border.dart

justinmc · 2024-03-18T20:52:45Z

packages/flutter/lib/src/material/menu_anchor.dart

+      (TraversalDirection.right, Axis.vertical) => buttonIsFocused ? _moveToPreviousFocusableTopLevel : _moveToParent,
+    };
+    if (!traversal(anchor)) {
+      super.invoke(intent);


Or maybe this one is the record haha.

reduction of 93 lines 😎

justinmc · 2024-03-18T21:25:56Z

packages/flutter/lib/src/material/slider.dart

-      case _SliderAdjustmentType.down:
-        renderSlider.decreaseAction();
-    }
+    final bool rtl = switch (Directionality.of(_renderObjectKey.currentContext!)) {


Nit: Maybe just assign the Directionality to a variable, then below do _SliderAdjustmentType.left => directionality == TextDirection.rtl, etc.?

This was set up as a switch expression since the style guide recommends not using == with enum values.

But in the case of TextDirection, there's already an abundance of == TextDirection.rtl in this repo. And the probability of this enum eventually being expanded into more than two values is pretty much zero.

I'm good with making this change 👍

packages/flutter/lib/src/material/reorderable_list.dart

bleroux

LGTM!!! with nits.
Thanks for this impressive work!

bleroux · 2024-03-19T07:17:19Z

dev/manual_tests/lib/star_border.dart

+  final OutlinedBorder targetBorder = switch (target) {
+    LerpTarget.circle  => const CircleBorder(side: lerpToBorder, eccentricity: 0.5),
+    LerpTarget.rect    => const RoundedRectangleBorder(side: lerpToBorder),
+    LerpTarget.stadium => const StadiumBorder(side: lerpToBorder),
+    LerpTarget.polygon => const StarBorder.polygon(side: lerpToBorder, sides: 4),
+    LerpTarget.star    => const StarBorder(side: lerpToBorder, innerRadiusRatio: .5),
+    LerpTarget.roundedRect => RoundedRectangleBorder(side: lerpToBorder, borderRadius: BorderRadius.circular(10)),


Impressive change!
You know better than me, consider aligning the arrows (it is not always better but in this case It might be).

It was there before but maybe consider replacing .5 with 0.5 (for homogeneity).

Sounds good—and I totally missed the 0.5, thanks for catching that!

bleroux · 2024-03-19T07:51:50Z

packages/flutter/lib/src/material/menu_anchor.dart

+      (TraversalDirection.right, Axis.vertical) when !rtl => buttonIsFocused ? _moveToSubmenu : _moveToNextFocusableTopLevel,
+      (TraversalDirection.left, Axis.vertical) when differentParent => _moveToPreviousFocusableTopLevel,
+      (TraversalDirection.right, Axis.vertical) when differentParent => _moveToPreviousFocusableTopLevel,
+      (TraversalDirection.left, Axis.vertical) => buttonIsFocused ? _moveToPreviousFocusableTopLevel : _moveToParent,


Not sure about how to make it clearer here. When comparing to the previous logic, I feel that having lines 2451/2453/2455 next to each others would be more readable.

That's actually how I had it a few days ago, and then I switched to this cause I thought it looked just a bit nicer.

Honestly not a huge deal, so I'll go ahead and revert.

packages/flutter/lib/src/material/menu_anchor.dart

flutter/flutter@18340ea...14774b9 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from eba6e31498b8 to 09dadce76828 (1 revision) (flutter/flutter#145603) 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9a34ae0b14f to eba6e31498b8 (1 revision) (flutter/flutter#145598) 2024-03-22 nate.w5687@gmail.com Intensive `if` chain refactoring (flutter/flutter#145194) 2024-03-22 leroux_bruno@yahoo.fr Adds numpad navigation shortcuts for Linux (flutter/flutter#145464) 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5a12de1beab7 to f9a34ae0b14f (1 revision) (flutter/flutter#145581) 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2f324beac3b to 5a12de1beab7 (1 revision) (flutter/flutter#145578) 2024-03-22 31859944+LongCatIsLooong@users.noreply.github.com Replace `RenderBox.compute*` with `RenderBox.get*` and add `@visibleForOverriding` (flutter/flutter#145503) 2024-03-22 gspencergoog@users.noreply.github.com Add some cross references in the docs, move an example to a dartpad example (flutter/flutter#145571) 2024-03-22 bernaferrari2@gmail.com Fix `BorderSide.none` requiring explicit transparent color for `UnderlineInputBorder` (flutter/flutter#145329) 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from a46a7b273a5b to e2f324beac3b (1 revision) (flutter/flutter#145576) 2024-03-21 goderbauer@google.com Fix nullability of getFullHeightForCaret (flutter/flutter#145554) 2024-03-21 34871572+gmackall@users.noreply.github.com Add a `--no-gradle-generation` mode to the `generate_gradle_lockfiles.dart` script (flutter/flutter#145568) 2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b842ae58b3d to a46a7b273a5b (2 revisions) (flutter/flutter#145569) 2024-03-21 chingjun@google.com Fixed race condition in PollingDeviceDiscovery. (flutter/flutter#145506) 2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from a2ed373fa70f to 1b842ae58b3d (1 revision) (flutter/flutter#145565) 2024-03-21 ian@hixie.ch Clarify AutomaticKeepAliveClientMixin semantics in build method (flutter/flutter#145297) 2024-03-21 goderbauer@google.com Eliminate more window singleton usages (flutter/flutter#145560) 2024-03-21 jacksongardner@google.com `flutter test --wasm` support (flutter/flutter#145347) 2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from eb262e9c34db to a2ed373fa70f (2 revisions) (flutter/flutter#145556) 2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from bad4a30e1c75 to eb262e9c34db (1 revision) (flutter/flutter#145555) 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 camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com 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

Previous "if-chains" pull requests: - #144905 - #144977 - #145194 - #146293 - #147472 <br> I think this one should be enough to wrap things up! fixes #144903 --------- Co-authored-by: Victor Sanni <victorsanniay@gmail.com>

…r#6376) flutter/flutter@18340ea...14774b9 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from eba6e31498b8 to 09dadce76828 (1 revision) (flutter/flutter#145603) 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9a34ae0b14f to eba6e31498b8 (1 revision) (flutter/flutter#145598) 2024-03-22 nate.w5687@gmail.com Intensive `if` chain refactoring (flutter/flutter#145194) 2024-03-22 leroux_bruno@yahoo.fr Adds numpad navigation shortcuts for Linux (flutter/flutter#145464) 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5a12de1beab7 to f9a34ae0b14f (1 revision) (flutter/flutter#145581) 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2f324beac3b to 5a12de1beab7 (1 revision) (flutter/flutter#145578) 2024-03-22 31859944+LongCatIsLooong@users.noreply.github.com Replace `RenderBox.compute*` with `RenderBox.get*` and add `@visibleForOverriding` (flutter/flutter#145503) 2024-03-22 gspencergoog@users.noreply.github.com Add some cross references in the docs, move an example to a dartpad example (flutter/flutter#145571) 2024-03-22 bernaferrari2@gmail.com Fix `BorderSide.none` requiring explicit transparent color for `UnderlineInputBorder` (flutter/flutter#145329) 2024-03-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from a46a7b273a5b to e2f324beac3b (1 revision) (flutter/flutter#145576) 2024-03-21 goderbauer@google.com Fix nullability of getFullHeightForCaret (flutter/flutter#145554) 2024-03-21 34871572+gmackall@users.noreply.github.com Add a `--no-gradle-generation` mode to the `generate_gradle_lockfiles.dart` script (flutter/flutter#145568) 2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b842ae58b3d to a46a7b273a5b (2 revisions) (flutter/flutter#145569) 2024-03-21 chingjun@google.com Fixed race condition in PollingDeviceDiscovery. (flutter/flutter#145506) 2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from a2ed373fa70f to 1b842ae58b3d (1 revision) (flutter/flutter#145565) 2024-03-21 ian@hixie.ch Clarify AutomaticKeepAliveClientMixin semantics in build method (flutter/flutter#145297) 2024-03-21 goderbauer@google.com Eliminate more window singleton usages (flutter/flutter#145560) 2024-03-21 jacksongardner@google.com `flutter test --wasm` support (flutter/flutter#145347) 2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from eb262e9c34db to a2ed373fa70f (2 revisions) (flutter/flutter#145556) 2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from bad4a30e1c75 to eb262e9c34db (1 revision) (flutter/flutter#145555) 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 camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com 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

nate-thegrate added 4 commits March 13, 2024 12:47

refactor into switch expressions

c7e4e3c

fix star_border, revert curves

4885d1e

revert datatable example

4342538

menu_anchor readability

24499a8

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. labels Mar 15, 2024

nate-thegrate added 5 commits March 14, 2024 20:45

fix variable order

4cd3d51

fix _borderRadius() function

a2fa582

revert mergable_material, update colors

ec60a53

revert colors.dart

173b248

fix reorderable_list.dart

0292405

nate-thegrate marked this pull request as ready for review March 15, 2024 05:27

HansMuller requested a review from justinmc March 15, 2024 21:59

justinmc approved these changes Mar 18, 2024

View reviewed changes

nate-thegrate added 2 commits March 18, 2024 16:00

implement @justinmc's suggestion

29875b1

Merge branch 'master' into intensive-if-chain-refactoring

ab6cb4f

bleroux approved these changes Mar 19, 2024

View reviewed changes

nate-thegrate added 2 commits March 19, 2024 08:47

changes suggested by @bleroux

ecdca09

Merge branch 'master' into intensive-if-chain-refactoring

c76f340

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 22, 2024

auto-submit bot merged commit 7fb35db into flutter:master Mar 22, 2024
132 checks passed

nate-thegrate deleted the intensive-if-chain-refactoring branch March 22, 2024 14:58

engine-flutter-autoroll mentioned this pull request Mar 22, 2024

Roll Flutter from 18340ea16cee to 7fb35db62fa5 (18 revisions) flutter/packages#6374

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2024

7fb35db62 Intensive if chain refactoring (flutter/flutter#145194)

8933c01

engine-flutter-autoroll mentioned this pull request Mar 22, 2024

Roll Flutter from 18340ea16cee to 7ad647ab9296 (19 revisions) flutter/packages#6375

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2024

7fb35db62 Intensive if chain refactoring (flutter/flutter#145194)

b908ffd

engine-flutter-autoroll mentioned this pull request Mar 22, 2024

Roll Flutter from 18340ea16cee to 14774b95c250 (20 revisions) flutter/packages#6376

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2024

7fb35db62 Intensive if chain refactoring (flutter/flutter#145194)

64679a0

HansMuller mentioned this pull request Mar 22, 2024

Flutter Framework Team: Notable Commits #121415

Open

nate-thegrate mentioned this pull request May 3, 2024

if chains → switch expressions #147793

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024

7fb35db62 Intensive if chain refactoring (flutter/flutter#145194)

51a562c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intensive `if` chain refactoring #145194

Intensive `if` chain refactoring #145194

Intensive if chain refactoring #145194

Intensive if chain refactoring #145194

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Intensive `if` chain refactoring #145194

Intensive `if` chain refactoring #145194