Use extension types.

[polina-c]

No description provided.

[kenzieschmoll]

instead of having these consts be in their own class _Json, can we add the constants to the class they are associated with OfflineMemoryData?

[polina-c]

It is more readable for me to have these consts grouped in enum-like way, under their own namespace.

[srawlins]

FWIW I don't think this is a good application of an extension type, as it's written right now.

Previously this code was one class, OfflineMemoryData, with a (private-ish) unnamed constructor, a factory fromJson constructor, and a static toJson function. The change in this PR is essentially moving the factory constructor and the toJson function into their own container, an extension type on a JSON-y Map. But it doesn't buy much.

The TODO comment pointing to https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/screens/profiler/cpu_profile_model.dart/#L760 references an "extension type" refactoring that I did, which I think is also a pretty weak example of extension types. But I think it still provides some semantic value there. There we have a _CpuProfileDataJson extension type that wraps a JSON-y Map, and can extract 7 different fields from the JSON. So the JSON parsing and key references are all restricted to that extension type. It also avoids business logic. For example if a "sampleCount" field is not found, it returns null rather than a default value (which maybe should be different for different applications? 🤷 ). Then in the CpuProfileData.fromJson factory constructor, we wrap the JSON with the extension type, and access the data via fields on the extension type, and apply business logic, like fallback values. I say this is still a weak application of extension types because we instantiate the thing on one line, access some fields over the next ~50 lines and then it's over. We don't store the reference or anything. On the one hand that's great because we didn't have to create and allocate a real wrapper object. But the downside is you read the code and think, "all this ceremony just to isolate some JSON parsing logic?" And my response to this thought is, "yeah that's basically it. 😀 " But I do think it is more readable than involving nitty gritty JSON parsing logic in the fromJson constructor. It still has 50 lines of other logic in it.

OK so back to OfflineMemoryData in here. I think what might make a good extension type would be to scrap the OfflineMemoryData class. So keep the extension type as you have it here, but add getters for each of the fields, selectedTab, filter, etc. Something like:

int? get selectedTab => json[_Json.selectedTab] as int?;

ClassFilter get filter => ClassFilter.fromJson(item(_Json.classFilter));

Then the (singular) code that uses OfflineMemoryData, the MemoryController class, can use OfflineMemoryDataJson, as the OfflineMemoryData class. In fact, the extension type could just be called OfflineMemoryData, and the abstraction should be invisible. I think it can still be used as a type argument in the declaration of MemoryController, and all the field access should work the same.

It should have a functionally equivalent API, and take less memory.

[polina-c]

Thanks. My two cents.

The methods toJson and fromJson reduce readability of a class, when they sit inside the class. They are strait forward, boring and take more space than they deserve.

So, I am happy just to move them to another namespace.
I do not really care what this namespace is. It can be extension type or mixin (with static method instead of constructor) or abstract static class (that is not recommended by dart style).
And we want it to be consistent across DevTools.

This another namespace should not publish any other API on top of toJson and fromJson, to keep it simple and consistent. So, I do not understand why to expose selectedTab.

You suggest to rename parse to filter. There are pros and cons here. Cons - it will be harder to copy paste this boilerplate between classes, you will need to change one more thing.

You suggest to make it getter. I have concerns here: getters cannot be heavy, while deserialization for some classes may take time. So, for consistency, I would make it a method.

@kenzieschmoll , what do you think?

[srawlins]

You suggest to rename parse to filter. There are pros and cons here. Cons - it will be harder to copy paste this boilerplate between classes, you will need to change one more thing.

You suggest to make it getter. I have concerns here: getters cannot be heavy, while deserialization for some classes may take time. So, for consistency, I would make it a method.

Oops, definitely did not mean to suggest either of these. Keep filter as filter. Keep it a getter.

[polina-c]

Thanks for clarifying. Actually, vise versa. It is called parse in this pr, and it is a method.
What is your suggestion then?

[srawlins]

It looks like the fields in OfflineMemoryData should stay as fields / backing-stores, for MemoryController.prepareOfflineScreenData, so we cannot remove the class (without broader refactoring maybe). So that suggestion is no good 😅 . If the goal is to just move fromJson and toJson out of the class, the extension type idea is interesting. I don't think it's too different than two top-level functions, or a mixin. The extension type at this point is just a namespace for the functions.

[polina-c]

Thank you!

@kenzieschmoll , what do you like more: top-level functions or extension type or mixin (with static method replacing constructor) or abstract static class (that is not recommended by dart style)?

[kenzieschmoll]

Given it doesn't seem like using extension types will add much value in this case, I recommend we abandon this PR and leave the current implementation alone.

[polina-c]

#7603