Feat/remove unsafe variables & add data type casting guards #20

thanhdang198 · 2024-11-02T08:11:06Z

Remove unnecessary late keywords
Remove unnecessary class construction
Add guards for data type casting

tvd12 · 2024-11-02T08:18:15Z

.vscode/settings.json

+        "ezyfox",
+        "freechat"
+    ]
+}


Cần bổ sung dòng trắng

tvd12

Anh có comment, tuy nhiên đây là một PR quá lớn, quá nhiều file bị thay đổi, nên sẽ thảo luận rất lâu, có thể mất 1 tuần, 2 tuần để merge được PR này em nhé.

Anh nghĩ lần tới em có thể tạo PR nhỏ hơn nhé.

@thanhdang198 PLTAL

tvd12 · 2024-11-02T08:21:12Z

example/lib/socket_proxy.dart

-    config.clientName = ZONE_NAME;
+    EzyConfig config = EzyConfig(ZONE_NAME);


Các này có vẻ ngắn hơn nhưng nó lại làm phá vỡ style chung với phần ở dưới ví dụ config.enableSSL điều này sẽ dẫn đến việc các lập trình viên khác đặt câu hỏi tại sao phải như vậy, do đó vi phạm vào nguyên tắc KISS

Phần này em có thấy clientName là field khá quan trọng, nhưng lại không required khi tạo class mà phụ thuộc vào việc lấy từ config (cũng là field option) ạ.

clientName cũng quan trọng tuy nhiên nó có thể lấy từ zoneName để làm clientName, ngoài ra nếu em bỏ đi hàm tạo mặc định, thêm hàm tạo có đối số sẽ gây breaking change cho các dự án đang dùng em ạ

tvd12 · 2024-11-02T08:22:35Z

lib/ezy_client.dart

-  late EzyConfig config;
+  EzyConfig config;
  late String name;
-  late EzyZone? zone;
-  late EzyUser? me;
+  EzyZone? zone;
+  EzyUser? me;
  late EzySetup setup;
  late EzyHandlerManager handlerManager;
-  late Uint8List? privateKey;
-  late int sessionId;
-  late String? sessionToken;
-  late Uint8List? sessionKey;
+  Uint8List? privateKey;
+  int? sessionId;
+  String? sessionToken;
+  Uint8List? sessionKey;


Em có thể giải thích tại sao em lại muốn remove late khỏi một số trường không?

tvd12 · 2024-11-02T08:22:57Z

lib/ezy_clients.dart

-  late String defaultClientName;
-  late Map<String, EzyClient> clients;
+  String defaultClientName = "";
+  Map<String, EzyClient> clients = <String, EzyClient>{};


Em có thể giải thích tại sao em lại muốn remove late khỏi một số trường không?

tvd12 · 2024-11-02T08:23:58Z

lib/ezy_clients.dart

+  EzyClient? getClient(String clientName) {
+    EzyClient? client = clients[clientName];
    return client;
  }

-  EzyClient getDefaultClient() {
-    return clients[defaultClientName]!;
+  EzyClient? getDefaultClient() {
+    return clients[defaultClientName];


Chỗ này không thể thay đổi được vì có thể gây breaking change cho các ứng dụng đang sử dụng, nếu muốn em phải tạo hàm mới để đảm bảo nguyên tắc Open/Close

Chỗ này buộc phải thay đổi, nó có nguy cơ khá lớn nếu không tìm thấy client -> dẫn đến việc EzyClient đang được khai báo là non-null nhưng bị gán null vào -> khả năng phát sinh lỗi.
Chỗ này mình có thể mark deprecated rồi mình thực hiện breaking change cho version sau sẽ ổn hơn ạ.

Chỗ này thực ra anh cũng đã từng để EzyClient? nhưng khi sử dụng cực kỳ bất tiện, vì trong thực tế sử dụng các client sẽ được khởi tạo và được đưa vào quản lý tương ứng với tên và không bao giờ null, khi sử dụng nếu cứ để ? thì đầu sử dụng cứ phải thêm ! cũng bất tiện nên anh nghĩ cứ để đó không cần phải deprecated, có thể tạo thêm hàm ? cũng không sao.

Chỗ này thực ra anh cũng đã từng để EzyClient? nhưng khi sử dụng cực kỳ bất tiện, vì trong thực tế sử dụng các client sẽ được khởi tạo và được đưa vào quản lý tương ứng với tên và không bao giờ null, khi sử dụng nếu cứ để ? thì đầu sử dụng cứ phải thêm ! cũng bất tiện nên anh nghĩ cứ để đó không cần phải deprecated, có thể tạo thêm hàm ? cũng không sao.

Vậy chỗ này anh đang chấp nhận đánh đổi rủi ro để lấy sự tiện lợi trong quá trình dev ạ?
Em thấy nó không đủ an toàn á anh, đồng thời các biến nullable đều phải có check if(x!=null) trước thì mới được thêm !, nếu không nó phải được access bằng x?.y?.z thay vì x! luôn ạ.

Anh nghĩ là để EzyClient? cũng là ý tưởng tốt, tuy nhiên phương thức hiện tại nó đã không để là EzyClient? rồi giờ sửa trực tiếp sẽ vi phạm nguyên tắc Open/Close không phải là ý hay, anh nghĩ là có thể tạo hàm mới, còn về deprecated thì anh nghĩ cứ để suy nghĩ thêm em ạ.

tvd12 · 2024-11-02T08:24:11Z

lib/ezy_config.dart

-  late String zoneName = "";
-  late String clientName;
-  late bool enableSSL = false;
-  late bool enableDebug = false;
-  late EzyPingConfig ping = EzyPingConfig();
-  late EzyReconnectConfig reconnect = EzyReconnectConfig();
-
+  String zoneName = "";
+  String clientName;
+  bool enableSSL = false;
+  bool enableDebug = false;
+  EzyPingConfig ping = EzyPingConfig();
+  EzyReconnectConfig reconnect = EzyReconnectConfig();
+  EzyConfig(this.clientName);


Em có thể giải thích tại sao em lại muốn remove late khỏi một số trường không?

tvd12 · 2024-11-02T08:24:30Z

lib/ezy_entities.dart

-  late int id;
-  late String name;
-  late EzyClient client;
+  int id;
+  String name;
+  EzyClient client;


tương tự

tvd12 · 2024-11-02T08:24:36Z

lib/ezy_entities.dart

-  late int id;
-  late String name;
-  late EzyZone zone;
-  late EzyClient client;
+  int id;
+  String name;
+  EzyZone zone;
+  EzyClient client;


tương tự

tvd12 · 2024-11-02T08:25:12Z

lib/ezy_handlers.dart

+    try {
+      var reason = event["reason"] as int;
+      var reasonName =
+          EzyConnectionFailedReasons.getConnectionFailedReasonName(reason);
+      EzyLogger.warn("connection failure, reason = $reasonName");
+    } catch (e) {
+      EzyLogger.error("error when handle connection failure $e");
+      rethrow;
+    }


Tại sao em lại muốn thêm try catch vào đây?

tvd12 · 2024-11-02T08:25:19Z

lib/ezy_handlers.dart

+    int? reason;
+    try {
+      reason = event["reason"] as int;
+      var reasonName = EzyDisconnectReasons.getDisconnectReasonName(reason);
+      EzyLogger.info("handle disconnection, reason = $reasonName");
+    } catch (e) {
+      EzyLogger.error("error when handle disconnection $e");
+      rethrow;
+    }


tương tự

tvd12 · 2024-11-02T08:25:25Z

lib/ezy_handlers.dart

+    int? reason;
+    try {
+      reason = event["reason"] as int;
+    } catch (e) {
+      EzyLogger.error("error when check should reconnect $e");
+    }


tương tự

thanhdang198 · 2024-11-02T10:08:16Z

Hi anh @tvd12, cảm ơn anh đã dành thời gian review PR của em ạ.
Do có nhiều câu hỏi khá trùng lặp nên em xin phép tổng hợp & trả lời chung vào 1 comment nhé ạ.

Vì sao em muốn remove late:

Trong flutter null-safety, các biến được khai báo mà thêm từ khoá late sẽ đánh dấu cho trình compiler của Dart hiểu răng biến này được khai báo tạm và sẽ được gán giá trị trước khi sử dụng. Nhưng trong các file này, em thấy việc dùng late chỉ là để bỏ qua các lỗi của trình khai báo chứ không có chỗ nào chắc chắn rằng biến sẽ được gán giá trị trước khi sử dụng. Việc này dẫn đến các rủi ro trong quá trình run time, có 1 biến nào đấy khởi tạo sai hoặc lỗi, nhưng hàm sử dụng tới biến đó vẫn được thực thi sẽ xảy ra vấn đề cho app. Vậy nên trong quá trình code, late CHỈ NÊN ĐƯỢC SỬ DỤNG KHI CHẮC CHẮN RẰNG NÓ SẼ ĐƯỢC GÁN GIÁ TRỊ TRƯỚC KHI ĐƯỢC GỌI TỪ BẤT KÌ ĐÂU.

Đồng thời, em có thấy 1 số giá trị là dynamic data được gán vào 1 số biến được khai báo non-null (không có dấu ? sau kiểu biến, ví dụ String? thay vì String. Rủi ro việc dynamic data này chứa null và gán cho biến non-null là có thể xảy ra & thêm 1 lỗi có thể đến từ quá trình runtime.

Tài liệu null-safety của flutter

Vì sao em lại thêm try catch vào các hàm có sử dụng data type cast
Vẫn là từ dynamic data mình không thể chắc chắn được rằng native hoặc server đang trả gì cho mình, dẫn tới việc cast nhầm type, thêm 1 rủi ro có thể xảy ra lỗi lọt vào quá trình runtime.
Breaking change
Phần này đúng là em hơi thiếu sót khi sửa hàm có sẵn mà không đánh dấu @Deprecated rồi viết thêm hàm khác.
Có thể em sẽ sửa lại vào 1 PR khác ạ.

Nếu được, anh nên quản lý lib bằng version trên pub.dev để thuận tiện cho việc nâng cấp về sau.
Em có release khá nhiều thư viện nên phần này em có thể hỗ trợ được nếu team cần ạ.
Em cảm ơn.

tvd12 · 2024-11-03T09:00:05Z

@thanhdang198 anh sẽ tách ra thành các phần thảo luận riêng nhé.

Anh nghĩ việc catch exception quá common không phải là ý tưởng tốt, nó có thể dẫn đến việc in ra nhiều log không cần thiết, anh nghĩ sẽ cần phải bắt đúng exception mà mình cần bắt sẽ tốt hơn, nếu chỉ là bắt cho vấn đề type cast thì em có thể kiểm tra kiểu dữ liệu cũng ok.

Ngoài ra các dữ liệu trong handler là do ezyfox-server trả về theo đúng kiểu dữ liệu đó nên không có vấn đề gì về ép kiểu em ạ.

thanhdang198 · 2024-11-06T06:42:18Z

@thanhdang198 anh sẽ tách ra thành các phần thảo luận riêng nhé.

Anh nghĩ việc catch exception quá common không phải là ý tưởng tốt, nó có thể dẫn đến việc in ra nhiều log không cần thiết, anh nghĩ sẽ cần phải bắt đúng exception mà mình cần bắt sẽ tốt hơn, nếu chỉ là bắt cho vấn đề type cast thì em có thể kiểm tra kiểu dữ liệu cũng ok.

Ngoài ra các dữ liệu trong handler là do ezyfox-server trả về theo đúng kiểu dữ liệu đó nên không có vấn đề gì về ép kiểu em ạ.

Em biết việc try-catch exception không tốt cho performance, 1 phần em cũng chưa đọc sâu các đoạn code native, nhưng để đọc code này & tin là chắc chắn không có lỗi trong quá trình runtime thì em không chấp nhận á anh.

tvd12 · 2024-11-07T04:08:35Z

em không chấp nhận

nghĩa là sao nhỉ? anh chưa hiểu ý em câu này

thanhdang198 added 3 commits November 2, 2024 14:59

feat: add pub topic & repository info

41045c9

feat: remove unnecessary string interpolation

ce089f3

feat: refactor un-safe late variables & add data type casting guards

8c683ba

tvd12 reviewed Nov 2, 2024

View reviewed changes

.vscode/settings.json

"ezyfox",

"freechat"

]

}

Copy link

Member

tvd12 Nov 2, 2024

Choose a reason for hiding this comment

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

Cần bổ sung dòng trắng

tvd12 requested changes Nov 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/remove unsafe variables & add data type casting guards #20

Feat/remove unsafe variables & add data type casting guards #20

thanhdang198 commented Nov 2, 2024

tvd12 Nov 2, 2024

tvd12 left a comment •

edited

Loading

tvd12 Nov 2, 2024

thanhdang198 Nov 2, 2024

tvd12 Nov 9, 2024 •

edited

Loading

tvd12 Nov 2, 2024

tvd12 Nov 2, 2024

tvd12 Nov 2, 2024

thanhdang198 Nov 2, 2024

tvd12 Nov 3, 2024

thanhdang198 Nov 6, 2024

tvd12 Nov 9, 2024

tvd12 Nov 2, 2024

tvd12 Nov 2, 2024

tvd12 Nov 2, 2024

tvd12 Nov 2, 2024

tvd12 Nov 2, 2024

tvd12 Nov 2, 2024

thanhdang198 commented Nov 2, 2024 •

edited

Loading

tvd12 commented Nov 3, 2024 •

edited

Loading

thanhdang198 commented Nov 6, 2024

tvd12 commented Nov 7, 2024 •

edited

Loading

		config.clientName = ZONE_NAME;
		EzyConfig config = EzyConfig(ZONE_NAME);

Feat/remove unsafe variables & add data type casting guards #20

Are you sure you want to change the base?

Feat/remove unsafe variables & add data type casting guards #20

Conversation

thanhdang198 commented Nov 2, 2024

Choose a reason for hiding this comment

tvd12 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvd12 Nov 9, 2024 • 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

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

thanhdang198 commented Nov 2, 2024 • edited Loading

tvd12 commented Nov 3, 2024 • edited Loading

thanhdang198 commented Nov 6, 2024

tvd12 commented Nov 7, 2024 • edited Loading

tvd12 left a comment •

edited

Loading

tvd12 Nov 9, 2024 •

edited

Loading

thanhdang198 commented Nov 2, 2024 •

edited

Loading

tvd12 commented Nov 3, 2024 •

edited

Loading

tvd12 commented Nov 7, 2024 •

edited

Loading