Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: overall improvements #208

Open
wants to merge 9 commits into
base: wifi_basic
Choose a base branch
from
48 changes: 34 additions & 14 deletions packages/wifi_basic/lib/src/extensions.dart
Original file line number Diff line number Diff line change
@@ -1,22 +1,42 @@
import 'package:wifi_basic/wifi_basic.dart';

extension ToEnumExtension on int? {
WiFiGenerations toWifiGeneration() {
if (this == null || this! < 0 || this! > 6) {
return WiFiGenerations.unknown;
extension WiFiGenerationsExtension on WiFiGenerations {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

static WiFiGenerations fromInt(int? value) {
switch (value) {
case 0:
case 1:
case 2:
case 3:
return WiFiGenerations.legacy;
case 4:
return WiFiGenerations.wifi4;
case 5:
return WiFiGenerations.wifi5;
case 6:
return WiFiGenerations.wifi6;
default:
return WiFiGenerations.unknown;
}
// legacy - if less than 3
if (this! <= 3) return WiFiGenerations.legacy;
// convert generationInt -> generationEnum
return WiFiGenerations.values[this! - 2];
}
}

WiFiNetworkSecurity toWifiNetworkSecurity() {
if (this == null ||
this! < 0 ||
this! > WiFiNetworkSecurity.values.length) {
return WiFiNetworkSecurity.unknown;
extension WiFiNetworkSecurityExtension on WiFiNetworkSecurity {
static WiFiNetworkSecurity fromInt(int? value) {
switch (value) {
case 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case 0 - if for none - unknown is just out of range or null. accordng to older logic

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure: I should only remove the case 0, right?

return WiFiNetworkSecurity.unknown;
case 1:
return WiFiNetworkSecurity.none;
case 2:
return WiFiNetworkSecurity.wep;
case 3:
return WiFiNetworkSecurity.wpa;
case 4:
return WiFiNetworkSecurity.wpa2;
case 5:
return WiFiNetworkSecurity.wpa3;
default:
return WiFiNetworkSecurity.unknown;
}
return WiFiNetworkSecurity.values[this! + 1];
}
}
89 changes: 41 additions & 48 deletions packages/wifi_basic/lib/wifi_basic.dart
Original file line number Diff line number Diff line change
@@ -1,38 +1,12 @@
import 'package:async/async.dart';
import 'package:collection/collection.dart';
import 'package:flutter/cupertino.dart';
import 'package:flutter/services.dart';
import 'package:wifi_basic/src/extensions.dart';

enum WiFiGenerations { unknown, legacy, wifi4, wifi5, wifi6 }

enum WiFiNetworkSecurity { unknown, none, wep, wpa, wpa2, wpa3 }

class WiFiInfo {
final String ssid;
final String bssid;
final WiFiNetworkSecurity security;
final bool isHidden;
final int rssi;
final double signalStrength;
final bool hasInternet;
final WiFiGenerations generation;

bool get isNull => ssid.isEmpty;

WiFiInfo._fromMap(Map map)
: ssid = map["ssid"],
bssid = map["bssid"],
security = (map["security"] as int?).toWifiNetworkSecurity(),
isHidden = map["isHidden"],
rssi = map["rssi"],
signalStrength = map["signalStrength"],
hasInternet = map["hasInternet"],
generation = (map["generation"] as int?).toWifiGeneration();

@override
String toString() => "ssid; $ssid; bssid: $bssid; security: $security; "
"isHidden: $isHidden; rssi: $rssi; signalStrength: $signalStrength; "
"hasInternet: $hasInternet; generation: $generation";
}
part 'wifi_generations.dart';
part 'wifi_info.dart';
part 'wifi_network_security.dart';

class WiFiBasic {
WiFiBasic._();
Expand All @@ -42,21 +16,40 @@ class WiFiBasic {
final _isSupportedMemo = AsyncMemoizer<bool>();
final _getGenerationMemo = AsyncMemoizer<WiFiGenerations>();

Future<bool> isSupported() => _isSupportedMemo
.runOnce(() async => await _channel.invokeMethod('isSupported'));

Future<WiFiGenerations> getGeneration() => _getGenerationMemo.runOnce(
() async => (await _channel.invokeMethod("getGeneration") as int?)
.toWifiGeneration());

Future<bool> isEnabled() async => await _channel.invokeMethod('isEnabled');

Future<bool> setEnabled(bool enabled) async =>
await _channel.invokeMethod('setEnabled', {"enabled": enabled});

Future<void> openSettings() async =>
await _channel.invokeMethod("openSettings");

Future<WiFiInfo> getCurrentInfo() async =>
WiFiInfo._fromMap(await _channel.invokeMapMethod("getCurrentInfo") ?? {});
Future<bool> isSupported() {
return _isSupportedMemo.runOnce(() async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think so. At least I don't see any problems this could cause for now.

return (await _channel.invokeMethod<bool>('isSupported'))!;
});
}

Future<WiFiGenerations> getGeneration() {
return _getGenerationMemo.runOnce(() async {
final generation = await _channel.invokeMethod<int?>("getGeneration");
return WiFiGenerationsExtension.fromInt(generation);
});
}

Future<bool> isEnabled() async {
return (await _channel.invokeMethod<bool>('isEnabled'))!;
}

Future<bool> setEnabled(bool enabled) async {
return (await _channel.invokeMethod<bool>(
'setEnabled',
{"enabled": enabled},
))!;
}

Future<void> openSettings() async {
await _channel.invokeMethod<void>("openSettings");
}

Future<WiFiInfo?> getCurrentInfo() async {
final result =
await _channel.invokeMapMethod<String, dynamic>("getCurrentInfo");

if (result == null) return null;

return WiFiInfo.fromMap(result);
}
}
3 changes: 3 additions & 0 deletions packages/wifi_basic/lib/wifi_generations.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
part of 'wifi_basic.dart';

enum WiFiGenerations { unknown, legacy, wifi4, wifi5, wifi6 }
62 changes: 62 additions & 0 deletions packages/wifi_basic/lib/wifi_info.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
part of 'wifi_basic.dart';

class WiFiInfo {
static const Equality _equality = DeepCollectionEquality();

final String ssid;
final String bssid;
final WiFiNetworkSecurity security;
final bool isHidden;
final int rssi;
final double signalStrength;
final bool hasInternet;
final WiFiGenerations generation;

bool get isNull => ssid.isEmpty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove it.

Copy link
Author

@fernando-s97 fernando-s97 Dec 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was also a doubt of mine. I'll do it.


@visibleForTesting
WiFiInfo.fromMap(Map map)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not keep it private?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and if it is public, do we need visibleForTesting annotation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also can we make it const?

Copy link
Author

@fernando-s97 fernando-s97 Dec 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

: ssid = map["ssid"] as String,
bssid = map["bssid"] as String,
security =
WiFiNetworkSecurityExtension.fromInt(map["security"] as int?),
isHidden = map["isHidden"] as bool,
rssi = map["rssi"] as int,
signalStrength = map["signalStrength"] as double,
hasInternet = map["hasInternet"] as bool,
generation =
WiFiGenerationsExtension.fromInt(map["generation"] as int?);

@override
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also need collection dependency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@fernando-s97 fernando-s97 Dec 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bool operator ==(Object other) =>
identical(this, other) ||
(runtimeType == other.runtimeType &&
other is WiFiInfo &&
_equality.equals(ssid, other.ssid) &&
_equality.equals(bssid, other.bssid) &&
_equality.equals(security, other.security) &&
_equality.equals(isHidden, other.isHidden) &&
_equality.equals(rssi, other.rssi) &&
_equality.equals(signalStrength, other.signalStrength) &&
_equality.equals(hasInternet, other.hasInternet) &&
_equality.equals(generation, other.generation));

@override
int get hashCode => Object.hash(
runtimeType,
_equality.hash(ssid),
_equality.hash(bssid),
_equality.hash(security),
_equality.hash(isHidden),
_equality.hash(rssi),
_equality.hash(signalStrength),
_equality.hash(hasInternet),
_equality.hash(generation),
);

@override
String toString() => "WiFiInfo(ssid: $ssid, bssid: $bssid, "
"security: $security, isHidden: $isHidden, rssi: $rssi, "
"signalStrength: $signalStrength, hasInternet: $hasInternet, "
"generation: $generation)";
}
3 changes: 3 additions & 0 deletions packages/wifi_basic/lib/wifi_network_security.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
part of 'wifi_basic.dart';

enum WiFiNetworkSecurity { unknown, none, wep, wpa, wpa2, wpa3 }
1 change: 1 addition & 0 deletions packages/wifi_basic/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ dependencies:
flutter:
sdk: flutter
async: ^2.8.0
collection: ^1.15.0

dev_dependencies:
flutter_test:
Expand Down
92 changes: 48 additions & 44 deletions packages/wifi_basic/test/wifi_basic_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@ import 'dart:math';

import 'package:flutter/services.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:wifi_basic/src/extensions.dart';
import 'package:wifi_basic/wifi_basic.dart';

import 'package:wifi_basic/src/extensions.dart' show ToEnumExtension;

void main() {
const channel = MethodChannel('wifi_basic');
final mockHandlers = <String, Function(dynamic arguments)>{};
Expand Down Expand Up @@ -86,52 +85,57 @@ void main() {
});

test("test getCurrentInfo", () async {
mockHandlers["getCurrentInfo"] = (_) => {
"ssid": "my-wifi",
"bssid": "02:00:00:00:00:00",
"security": 0,
"isHidden": false,
"rssi": -100,
"signalStrength": 1.0,
"hasInternet": true,
"generation": -1,
};
final value = {
"ssid": "my-wifi",
"bssid": "02:00:00:00:00:00",
"security": 0,
"isHidden": false,
"rssi": -100,
"signalStrength": 1.0,
"hasInternet": true,
"generation": -1,
};
mockHandlers["getCurrentInfo"] = (_) => value;
final info = await WiFiBasic.instance.getCurrentInfo();
expect(info.ssid, "my-wifi");
expect(info.bssid, "02:00:00:00:00:00");
expect(info.security, WiFiNetworkSecurity.none);
expect(info.isHidden, false);
expect(info.rssi, -100);
expect(info.signalStrength, 1.0);
expect(info.hasInternet, true);
expect(info.generation, WiFiGenerations.unknown);

expect(info, WiFiInfo.fromMap(value));
});

test("test ToEnumExtension.toWifiGeneration", () async {
// test for unknown
expect((-1).toWifiGeneration(), WiFiGenerations.unknown);
// test for legacy
expect(Random().nextInt(3).toWifiGeneration(), WiFiGenerations.legacy);
// test for wifi4, wifi5 and wifi6
expect(4.toWifiGeneration(), WiFiGenerations.wifi4);
expect(5.toWifiGeneration(), WiFiGenerations.wifi5);
expect(6.toWifiGeneration(), WiFiGenerations.wifi6);
// test for unknown again
expect(
(7 + Random().nextInt(10)).toWifiGeneration(), WiFiGenerations.unknown);
test("test WiFiGenerationsExtension", () {
final intValues = [-1, 0, 1, 2, 3, 4, 5, 6, 7, null];

final enumValues = intValues.map(WiFiGenerationsExtension.fromInt).toList();

expect(enumValues, [
WiFiGenerations.unknown,
WiFiGenerations.legacy,
WiFiGenerations.legacy,
WiFiGenerations.legacy,
WiFiGenerations.legacy,
WiFiGenerations.wifi4,
WiFiGenerations.wifi5,
WiFiGenerations.wifi6,
WiFiGenerations.unknown,
WiFiGenerations.unknown,
]);
});

test("test ToEnumExtension.toWifiNetworkSecurity", () {
// test for unknown
expect((-1).toWifiNetworkSecurity(), WiFiNetworkSecurity.unknown);
// test for values
expect(0.toWifiNetworkSecurity(), WiFiNetworkSecurity.none);
expect(1.toWifiNetworkSecurity(), WiFiNetworkSecurity.wep);
expect(2.toWifiNetworkSecurity(), WiFiNetworkSecurity.wpa);
expect(3.toWifiNetworkSecurity(), WiFiNetworkSecurity.wpa2);
expect(4.toWifiNetworkSecurity(), WiFiNetworkSecurity.wpa3);
// test for unknown again
expect((7 + Random().nextInt(10)).toWifiNetworkSecurity(),
WiFiNetworkSecurity.unknown);
test("test WiFiNetworkSecurityExtension", () {
final intValues = [-1, 0, 1, 2, 3, 4, 5, 6, null];

final enumValues =
intValues.map(WiFiNetworkSecurityExtension.fromInt).toList();

expect(enumValues, [
WiFiNetworkSecurity.unknown,
WiFiNetworkSecurity.unknown,
WiFiNetworkSecurity.none,
WiFiNetworkSecurity.wep,
WiFiNetworkSecurity.wpa,
WiFiNetworkSecurity.wpa2,
WiFiNetworkSecurity.wpa3,
WiFiNetworkSecurity.unknown,
WiFiNetworkSecurity.unknown,
]);
});
}