AnalyzerConverter class has no fields, only instance methods

[srawlins]

AnalyzerConverter class has no fields, only instance methods. It should be refactored to be top-level utility functions and/or extension methods.

As an example of the danger of a class like this, in _DartNavigationCollector, which is invoked upon every key press in the editor, we construct a new AnalyzerConverter, in _addRegionForElement, for every element in the open file which we might want to navigate to. 😨

[bwilkerson]

To the best of my knowledge, objects without state aren't a code smell, they're a valid technique for grouping functions with a similar purpose. I don't see this as a code health issue.

Because AnalyzerConverter has no state, it's very fast to create an instance, and short lived objects are cheap in terms of garbage collection. But if there's evidence that this is actually a performance issue then we could simply create a single instance in _DartNavigationCollector and re-use it.

And given that AnalyzerConverter is in the analyzer_plugin package, I don't believe that the impact on plugin authors is justified by any perceived value of making these changes. (Nor do I think we want to put effort into this package until we have decided whether we're going to support it in earnest. If we do decide to support it then it needs a much bigger overhaul that this.)

[srawlins]

It's certainly cheaper to not instantiate objects. It's also difficult for me to reason about the AnalyzerConverter class. Regardless of the performance benefits, measurable or not, it would be simpler to remove the AnalyzerConverter class and make the static methods top-level. It would be easier to reason about and not invite unnecessary instantiations.

It also goes against Effective Dart.

(Nor do I think we want to put effort into this package until we have decided whether we're going to support it in earnest. If we do decide to support it then it needs a much bigger overhaul that this.)

I think it's perfectly valid to improve the code before we decide to support it. The code is used by analysis_server, and it's location in analyzer_plugin is unrelated to whether it should be improved.

[bwilkerson]

I disagree about priorities, but I'll note that in a recent CL you converted a couple of these methods to being extension methods, which is much more palatable to me than top-level functions (which might have been a misunderstanding on my part; maybe that wasn't what you were thinking).

But I still believe that this work should have a much lower priority than you gave it. Whichever way the decision goes I expect this work to be moot in terms of what the final product will contain.

[srawlins]

Ah, I missed that I had given it a priority. OK, downgraded to P3.

in a recent CL you converted a couple of these methods to being extension methods, which is much more palatable to me than top-level functions

I agree. I think it's a method-by-method call, but I do think I'm enjoying the extension methods more than top-level functions.

[bwilkerson]

For me, that that's because I believe that object-oriented code is much better suited than functional code to the kind of software we're writing.