Code metric fixes #4890

jacob314 · 2022-12-02T23:25:47Z

Experiment applying most of the fixes suggested by the default dart_code_metrics configuration.

Feedback appreciated. Most of the switches to conditionals seem like an improvement but a few might be going overboard.

jacob314 · 2022-12-02T23:28:15Z

packages/devtools_app/lib/src/http/_http_date.dart

-      } else {
-        pos = date.length;
-      }
+      pos = separator.length > 0 ? date.indexOf(separator, index) : date.length;


changes like this seem like an improvement. Fyi @pq have we thought about adding a lint for preferring conditionals?
The one in Dart Code Metrics goes a bit overboard. I'd imagine if we added it to the core linter it would only fire for short expressions like this and not the large ones that code metrics suggests.

what about isNotEmpty for seperator?

I'd also be happy with a lint + fix for isEmpty and isNotEmpty. Seems like a nice simple lint to nudge people to use the Dart core libraries. Looks like our users use .isNotEmpty about 10X as often then .length > 0 so the lint shouldn't be contreversial.

We HAVE that lint. Somewhere...

https://dart-lang.github.io/linter/lints/prefer_is_not_empty.html – HA! We should expand this to String!

https://github.com/dart-lang/linter/issues/3889

Related: avoid_bool_literals_in_conditional_expressions.

jacob314 · 2022-12-02T23:28:54Z

packages/devtools_app/lib/src/screens/debugger/call_stack.dart

-            child: Text(
-              frameDescription,
-              style: selected ? theme.selectedTextStyle : theme.subtleTextStyle,
+    child = asyncMarker


@kenzieschmoll what do you think about cases like this? Is this cleaner or more obfuscated?

I find it harder to read when one or more of the three operands need to be split across multiple lines.

I do think the if / else block is a little bit easier to read here given how many lines each operand is.

Agreed. Reverted this one

jacob314 · 2022-12-02T23:30:23Z

packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart

@@ -807,7 +805,7 @@ class InspectorController extends DisposableController
  }

  void selectionChanged() {
-    if (visibleToUser == false) {
+    if (!visibleToUser) {


Fyi @pq. This is a lint fix from Dart code metrics that I'd be comfortable having in the recommended set if we were able to run Dart code metrics without significant performance overhead.

Right, that's:

https://dartcodemetrics.dev/docs/rules/common/no-boolean-literal-compare

I'm a little surprised that was never defined in the core linter...

jacob314 · 2022-12-02T23:31:01Z

packages/devtools_app/lib/src/screens/inspector/inspector_service.dart

-        invokeServiceMethodObservatoryInspectorRef(methodName, ref),
-      );
-    }
+    return useDaemonApi


this was a larger block but I still think this was a little bit of an improvement over the original code.

jacob314 · 2022-12-02T23:35:42Z

packages/devtools_app/lib/src/shared/theme.dart

-  Color get grey => isLight
-      ? const Color.fromARGB(255, 128, 128, 128)
-      : const Color.fromARGB(255, 128, 128, 128);
+  Color get grey => const Color.fromARGB(255, 128, 128, 128);


Case the linter caught we had a useless check for isLight

jacob314 · 2022-12-02T23:36:47Z

packages/devtools_app/lib/src/ui/utils.dart

-      return MediaSize.l;
-    else
-      return MediaSize.xl;
+    if (height < 900) return MediaSize.l;


Fyi @pq this sort of case is probably one we would want to start suggesting and quick fixing to Patterns.

See: https://github.com/dart-lang/linter/issues/3673

bwilkerson · 2022-12-03T00:47:11Z

packages/devtools_app/lib/src/charts/flame_chart.dart

-    } else {
-      _altKeyPressed = false;
-    }
+    _altKeyPressed = event.isAltPressed ? true : false;


This should be further simplified to _altKeyPressed = event.isAltPressed; (as the branches just return the value of the condition).

done. filed https://github.com/dart-lang/linter/issues/3888 to track this as this really obvious unnecessary code has been committed over 200X in g3.

bwilkerson · 2022-12-03T00:50:09Z

packages/devtools_app/lib/src/screens/debugger/call_stack.dart

-            child: Text(
-              frameDescription,
-              style: selected ? theme.selectedTextStyle : theme.subtleTextStyle,
+    child = asyncMarker


I find it harder to read when one or more of the three operands need to be split across multiple lines.

kenzieschmoll · 2022-12-05T17:11:47Z

packages/devtools_app/lib/src/framework/status_line.dart

-        ),
-      );
-    } else {
-      // Use a placeholder for pages with no explicit documentation.


was the comment automatically removed? we should probably leave this even with the ternary operator

kenzieschmoll · 2022-12-05T17:15:34Z

packages/devtools_app/lib/src/screens/debugger/debugger_model.dart

+    return scriptRef == null
+        ? node.scriptRef != null
+            ? false
+            : name == node.name
+        : node.scriptRef == null
+            ? false
+            : scriptRef == node.scriptRef;


the nested ternary operators makes this a bit difficult to read

agreed this is overly clever. reverted.

kenzieschmoll · 2022-12-05T17:18:29Z

packages/devtools_app/lib/src/screens/memory/panes/chart/chart_pane.dart

+          : image == null
+              ? const SizedBox()
+              : scaleImage
+                  ? Image(image: AssetImage(image), width: 20, height: 10)
+                  : Image(image: AssetImage(image));


triple nest 😱

revertedh. if we were to add a lint about ternary operators, it would never suggest more than one nesting level and never suggest large blocks.

kenzieschmoll · 2022-12-05T17:19:45Z

packages/devtools_app/lib/src/screens/vm_developer/vm_service_private_extensions.dart

+    return guardClassType == '@Class' || guardClassType == 'Class'
+        ? true
+        : false;


could be further simplified to return guardClassType == '@Class' || guardClassType == 'Class';

Fixed and filed https://github.com/dart-lang/linter/issues/3888

jacob314 · 2022-12-08T00:10:03Z

packages/devtools_app/lib/src/screens/debugger/call_stack.dart

-      );
-    }
+    return isAsyncBreak
+        ? result


@kenzieschmoll what do you think of this case is. Brian thought it is a bit harder to read but I could go either way on this one.

I also could go either way. I think I prefer the if / else here but the ternary isn't terrible since of of the options is a one-liner.

jacob314

PTAL

incendial · 2022-12-08T06:56:51Z

@jacob314 hi, if you have any feedback about the package (aside the comments above which I agree with, especially ? true : false cases), please share! Also interested in the rules configuration you used, not sure what you meant by default dart_code_metrics configuration..

This is a lint fix from Dart code metrics that I'd be comfortable having in the recommended set if we were able to run Dart code metrics without significant performance overhead.

This is interesting 🙂 . I'm working on a DCM version without the plugins API (and thus not listed in pubspec at all) and the overall performance is very good (VS Code integration is ready, now finishing the IntelliJ part). I'd be happy to discuss this idea in more details to understand what can be done from our side to enable DCM for the Flutter teams.

jacob314 · 2022-12-08T16:52:35Z

@jacob314 hi, if you have any feedback about the package (aside the comments above which I agree with, especially ? true : false cases), please share! Also interested in the rules configuration you used, not sure what you meant by default dart_code_metrics configuration..

This is a lint fix from Dart code metrics that I'd be comfortable having in the recommended set if we were able to run Dart code metrics without significant performance overhead.

This is interesting 🙂 . I'm working on a DCM version without the plugins API (and thus not listed in pubspec at all) and the overall performance is very good (VS Code integration is ready, now finishing the IntelliJ part). I'd be happy to discuss this idea in more details to understand what can be done from our side to enable DCM for the Flutter teams.

By default configuration I mean the basic configuration suggested by https://dartcodemetrics.dev/docs/getting-started/configuration
I'm very interested in hearing more about the DCM version without the plugins API as well. In particular I'm interested in what memory usage and performance you are seeing when you avoid the plugins API.

General feedback: all of the lints I tried were ones I would have liked to have turned on by default except for prefer-conditional-expressions which would have too many cases I'd need to ignore to enable.
I'd love a more restricted version of prefer-conditional-expressions that doesn't ever prefer double nested conditional expressions and doesn't prefer conditional expressions if either clause of the conditional is more than a couple lines long. The prefer-conditonal-expressions lint also caught a bunch of silly redundant logic in our code so I'd be really excited to enable it if it could be made more restricted.
I think the restrictions are needed due to it being harder to tell what the block boundaries are for a conditional expression than an if, else resulting in code that was harder to read as seen in this CL.
The other piece of feedback is the quick fix for prefer-conditional-expression corrupted by code a few times. I would suggest a bit more error checking to verify that the quick fix only does safe code transformations.
It is preferable for a quick-fix to sometimes be unable to fix code than to introduce errors in the code. I can try to give a repro if it helps. In addition to sometimes corrupting logic, the fixes also removed comments which was a more minor but annoying case when trying to bulk cleanup issues.

kenzieschmoll · 2022-12-09T00:15:51Z

packages/devtools_app/lib/src/screens/memory/panes/chart/legend.dart

+      return color != null
+          ? dashedLine as bool
+              ? createDashWidget(color)
+              : createSolidLine(color)
+          : image == null
+              ? const SizedBox()
+              : Image(image: AssetImage(image));


can we revert the double ternary?

Done. Reverted this file.

kenzieschmoll

one comment then lgtm

incendial · 2022-12-09T09:22:51Z

By default configuration I mean the basic configuration suggested by https://dartcodemetrics.dev/docs/getting-started/configuration

Oh, I see. I guess we need to update it coz the list of rules is far more bigger right now 🙂 (you can find the whole list in presets https://github.com/dart-code-checker/dart-code-metrics/tree/master/lib/presets)

I'm very interested in hearing more about the DCM version without the plugins API as well. In particular I'm interested in what memory usage and performance you are seeing when you avoid the plugins API.

VS Code: the overall memory consumption for so called DCM analysis-server is about one and a half times less than for the Dart analysis-server

In numbers (for monorepo of like 6 packages):

DCM analysis server - ~260 MB
Dart - ~370 MB

Just for the DCM repo https://github.com/dart-code-checker/dart-code-metrics:

DCM analysis server - ~ 200 MB
Dart - ~370 MB
Dart + plugin - ~1,1 GB 😢

Meaning that the DCM + Dart consume about twice less as Dart + plugin 😁.

Please note: the measurements above taken after both processes were running for some time. The measurement on a start gives an even bigger difference (like 3 times).

Additionally, there is no "new plugin instance for each package" problem, meaning that it scales pretty good. And the overall responsiveness is better (if both the extension and the plugin are enabled, I see the highlight from the extension and then from the plugin in a noticeable amount of time).

IntelliJ: I haven't yet measured the IntelliJ difference, since the implementation is still in progress, but hopefully soon I'll be able to measure. Overall performance for the issues highlight is good, IMO.

I'd love a more restricted version of prefer-conditional-expressions that doesn't ever prefer double nested conditional expressions and doesn't prefer conditional expressions if either clause of the conditional is more than a couple lines long. The prefer-conditonal-expressions lint also caught a bunch of silly redundant logic in our code so I'd be really excited to enable it if it could be made more restricted.

Thanks, noted! We have a separate rule for limiting the nesting level https://dartcodemetrics.dev/docs/rules/common/avoid-nested-conditional-expressions, but idea to make prefer-conditonal-expressions aware of nesting sounds solid!

The other piece of feedback is the quick fix for prefer-conditional-expression corrupted by code a few times. I would suggest a bit more error checking to verify that the quick fix only does safe code transformations.

An example would be great, tbh (so I'd be able to fix it). But I can agree that the quick fixes we have don't check for the output validity and more like "this should work" type of a result.

I can try to give a repro if it helps.

Would be amazing!

the fixes also removed comments which was a more minor but annoying case when trying to bulk cleanup issues.

Definitely a bug, thank you!

jacob314 requested review from bkonyi, elliette, polina-c, kenzieschmoll, CoderDake and a team as code owners December 2, 2022 23:25

jacob314 force-pushed the code_metric_fixes branch from daff603 to cd230e7 Compare December 2, 2022 23:26

jacob314 commented Dec 2, 2022

View reviewed changes

jacob314 removed the request for review from kenzieschmoll December 2, 2022 23:37

bwilkerson reviewed Dec 3, 2022

View reviewed changes

kenzieschmoll reviewed Dec 5, 2022

View reviewed changes

jacob314 mentioned this pull request Dec 8, 2022

proposal: unneccessary_ternary_operator dart-lang/sdk#58954

Open

jacob314 commented Dec 8, 2022

View reviewed changes

jacob314 force-pushed the code_metric_fixes branch from 6358e9c to d889164 Compare December 8, 2022 00:16

jacob314 commented Dec 8, 2022

View reviewed changes

incendial mentioned this pull request Dec 8, 2022

feat: add avoid-unnecessary-conditionals rule dart-code-checker/dart-code-metrics#1094

Merged

8 tasks

jacob314 force-pushed the code_metric_fixes branch 2 times, most recently from ef945bd to 3c0291c Compare December 9, 2022 00:10

kenzieschmoll reviewed Dec 9, 2022

View reviewed changes

kenzieschmoll approved these changes Dec 9, 2022

View reviewed changes

Fixes from code metrics lints

217d07b

jacob314 force-pushed the code_metric_fixes branch from 3c0291c to 217d07b Compare December 9, 2022 00:24

jacob314 merged commit d695482 into flutter:master Dec 13, 2022

jacob314 deleted the code_metric_fixes branch December 13, 2022 22:50

CoderDake mentioned this pull request Dec 22, 2022

Prepare Release Notes for 2.21.0 release #4982

Closed

64 tasks

Code metric fixes #4890

Code metric fixes #4890

Conversation

jacob314 commented Dec 2, 2022

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

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

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

jacob314 Dec 8, 2022 • edited Loading

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

jacob314 left a comment

Choose a reason for hiding this comment

incendial commented Dec 8, 2022

jacob314 commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll left a comment

Choose a reason for hiding this comment

incendial commented Dec 9, 2022 • edited Loading

jacob314 Dec 8, 2022 •

edited

Loading

incendial commented Dec 9, 2022 •

edited

Loading