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 [email protected] Roll Flutter Engine from eba6e31498b8 to 09dadce76828 (1 revision) (flutter/flutter#145603) 2024-03-22 [email protected] Roll Flutter Engine from f9a34ae0b14f to eba6e31498b8 (1 revision) (flutter/flutter#145598) 2024-03-22 [email protected] Intensive `if` chain refactoring (flutter/flutter#145194) 2024-03-22 [email protected] Adds numpad navigation shortcuts for Linux (flutter/flutter#145464) 2024-03-22 [email protected] Roll Flutter Engine from 5a12de1beab7 to f9a34ae0b14f (1 revision) (flutter/flutter#145581) 2024-03-22 [email protected] Roll Flutter Engine from e2f324beac3b to 5a12de1beab7 (1 revision) (flutter/flutter#145578) 2024-03-22 [email protected] Replace `RenderBox.compute*` with `RenderBox.get*` and add `@visibleForOverriding` (flutter/flutter#145503) 2024-03-22 [email protected] Add some cross references in the docs, move an example to a dartpad example (flutter/flutter#145571) 2024-03-22 [email protected] Fix `BorderSide.none` requiring explicit transparent color for `UnderlineInputBorder` (flutter/flutter#145329) 2024-03-22 [email protected] Roll Flutter Engine from a46a7b273a5b to e2f324beac3b (1 revision) (flutter/flutter#145576) 2024-03-21 [email protected] Fix nullability of getFullHeightForCaret (flutter/flutter#145554) 2024-03-21 [email protected] Add a `--no-gradle-generation` mode to the `generate_gradle_lockfiles.dart` script (flutter/flutter#145568) 2024-03-21 [email protected] Roll Flutter Engine from 1b842ae58b3d to a46a7b273a5b (2 revisions) (flutter/flutter#145569) 2024-03-21 [email protected] Fixed race condition in PollingDeviceDiscovery. (flutter/flutter#145506) 2024-03-21 [email protected] Roll Flutter Engine from a2ed373fa70f to 1b842ae58b3d (1 revision) (flutter/flutter#145565) 2024-03-21 [email protected] Clarify AutomaticKeepAliveClientMixin semantics in build method (flutter/flutter#145297) 2024-03-21 [email protected] Eliminate more window singleton usages (flutter/flutter#145560) 2024-03-21 [email protected] `flutter test --wasm` support (flutter/flutter#145347) 2024-03-21 [email protected] Roll Flutter Engine from eb262e9c34db to a2ed373fa70f (2 revisions) (flutter/flutter#145556) 2024-03-21 [email protected] 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 [email protected],[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

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 <[email protected]>

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

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

7fb35db 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

7fb35db 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

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

64679a0

HansMuller mentioned this pull request Mar 22, 2024

Flutter Framework Team: Notable Commits #121415

Closed

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

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

51a562c

nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Jul 12, 2024

Intensive if chain refactoring #145194

Intensive if chain refactoring #145194

Uh oh!

Conversation

nate-thegrate commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinmc Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bleroux left a comment

Choose a reason for hiding this comment

Uh oh!

bleroux Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

bleroux Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Intensive `if` chain refactoring #145194

Intensive `if` chain refactoring #145194

nate-thegrate commented Mar 15, 2024 •

edited

Loading

nate-thegrate Mar 18, 2024 •

edited

Loading