refactor: overall improvements#208
refactor: overall improvements#208fernando-s97 wants to merge 9 commits intoflutternetwork:wifi_basicfrom
Conversation
daadu
left a comment
daadu
left a comment
There was a problem hiding this comment.
Reviewed it, let me know your views.
| extension WiFiNetworkSecurityExtension on WiFiNetworkSecurity { | ||
| static WiFiNetworkSecurity fromInt(int? value) { | ||
| switch (value) { | ||
| case 0: |
There was a problem hiding this comment.
case 0 - if for none - unknown is just out of range or null. accordng to older logic
There was a problem hiding this comment.
Just to make sure: I should only remove the case 0, right?
| WiFiGenerations toWifiGeneration() { | ||
| if (this == null || this! < 0 || this! > 6) { | ||
| return WiFiGenerations.unknown; | ||
| extension WiFiGenerationsExtension on WiFiGenerations { |
There was a problem hiding this comment.
extensions where used as a convince to call methods on integer directly like - 2.toWiFiGenerations.
The changed approach looks like a normal function call - in that case I suggest we avoid extensions completely - and have a global util function like _serializeWiFiGeneration/_deserializeWiFiGeneration.
There was a problem hiding this comment.
Don't you think that we having the (de)serialize logic here is more concise than a global utility method? It's the same concept of using having a Class.fromJson and Class.toJson.
The way I see, having the fromInt in an extension better gives the message of "this parse should only be used here", different from a global function.
It's also better for maintenance, since if we ever rename this enum, we won't need to remember to rename to function.
And last, but not least, for me, the extensions approach makes the code more elegant.
| bool get isNull => ssid.isEmpty; | ||
|
|
||
| @visibleForTesting | ||
| WiFiInfo.fromMap(Map map) |
There was a problem hiding this comment.
and if it is public, do we need visibleForTesting annotation?
There was a problem hiding this comment.
The visibleForTesting requires the thing in question to be "public". But it's only public inside test classes. For the rest of the code, the property is considered private, that's why I still needed to use part and part of keywords.
And yes, it probably can be const.
| final bool hasInternet; | ||
| final WiFiGenerations generation; | ||
|
|
||
| bool get isNull => ssid.isEmpty; |
There was a problem hiding this comment.
That was also a doubt of mine. I'll do it.
| generation = | ||
| WiFiGenerationsExtension.fromInt(map["generation"] as int?); | ||
|
|
||
| @override |
There was a problem hiding this comment.
Now that I think of == override and hashCode looks unnecessary - what could be the use case to do it? If it has solid use case then we might want to implement it for all classes.
There was a problem hiding this comment.
It also need collection dependency.
There was a problem hiding this comment.
I see the WifiInfo as data class, since it does exatcly it: holds data. And every data class should override == and hashCode, otherwise, two identical classes would be considered different, which is wrong.
There was a problem hiding this comment.
The collection dependency isn't exatcly required. It's totally possible override those methods without it, but this packages already handle lists, sets, deep objects andsome more use cases, that otherwise, we would have to handle by ouselves.
I'm aware that have a simple data class now, and all those cases aren't indeed necessary right now, but if we ever change that, or add other data classes that can benefit from it, it's already there.
And since the collections package is maintained by the dart team, I don't see a problem using it.
There was a problem hiding this comment.
A concrete use case can be seen in tests, where I compare info against the object WifiInfo. That comparison would have failed without those overrides
| Future<WiFiInfo> getCurrentInfo() async => | ||
| WiFiInfo._fromMap(await _channel.invokeMapMethod("getCurrentInfo") ?? {}); | ||
| Future<bool> isSupported() { | ||
| return _isSupportedMemo.runOnce(() async { |
There was a problem hiding this comment.
instead of memoizer can we do it with simple var? if null we calculate else return the result directly. we can avoid depency for async in that case.
There was a problem hiding this comment.
I do think so. At least I don't see any problems this could cause for now.
2 participants
Changes discussed at #201 (comment)
I've made each change in a separate commit to make it easier to review.