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

feat: persistent map serialize #498

Merged
merged 4 commits into from
Nov 20, 2024
Merged

feat: persistent map serialize #498

merged 4 commits into from
Nov 20, 2024

Conversation

JNdhlovu
Copy link
Contributor

Story: https://app.shortcut.com/smileid/story/14361/update-wrapper-sdks-with-latest-native-updates

Summary

Update wrapper SDKs with latest native updates
these add serialization to immutable maps passed from the wrappers

Known Issues

N/A

Test Instructions

N/A

Screenshot

If applicable (e.g. UI changes), add screenshots to help explain your work.

@prfectionist
Copy link

prfectionist bot commented Nov 19, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Code Smell
The PersistentMapSerializer constructor takes String serializers as parameters but they are not used anywhere else in the code. Consider making the class a singleton object since it only handles String key-value maps.

Documentation Missing
The serializer class lacks documentation explaining its purpose and usage. Consider adding KDoc comments describing the class functionality and its experimental status.

@prfectionist
Copy link

prfectionist bot commented Nov 19, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Performance
Cache serializer instance to avoid repeated object creation during serialization operations

Cache the MapSerializer instance instead of creating a new one for each
serialize/deserialize operation to improve performance.

lib/src/main/java/com/smileidentity/compose/nav/PersistentMapSerializer.kt [27-29]

+private val mapSerializer = MapSerializer(keySerializer, valueSerializer)
+
 override fun serialize(encoder: Encoder, value: ImmutableMap<String, String>) {
-    return MapSerializer(keySerializer, valueSerializer).serialize(encoder, value)
+    return mapSerializer.serialize(encoder, value)
 }
Suggestion importance[1-10]: 7

Why: Valid optimization that would prevent unnecessary object creation on each serialization call, improving performance by reusing the MapSerializer instance.

7
Convert stateless class to singleton object to improve memory efficiency and simplify usage

Consider making the serializer a singleton object since it's stateless and always
operates on String key-value pairs. This would improve memory usage and avoid
unnecessary instance creation.

lib/src/main/java/com/smileidentity/compose/nav/PersistentMapSerializer.kt [14-17]

-class PersistentMapSerializer(
-    private val keySerializer: KSerializer<String>,
-    private val valueSerializer: KSerializer<String>,
-) : KSerializer<ImmutableMap<String, String>> {
+object PersistentMapSerializer : KSerializer<ImmutableMap<String, String>> {
+    private val mapSerializer = MapSerializer(String.serializer(), String.serializer())
Suggestion importance[1-10]: 4

Why: Converting to a singleton could improve memory usage, but the current implementation needs parameterized serializers which wouldn't work well with a singleton pattern. The suggestion oversimplifies the requirements.

4
Possible issue
Add error handling to serialization operations to prevent crashes and provide fallback values

The serializer class could potentially throw exceptions during
serialization/deserialization operations. Consider wrapping these operations in a
try-catch block to handle potential errors gracefully.

lib/src/main/java/com/smileidentity/compose/nav/PersistentMapSerializer.kt [31-33]

 override fun deserialize(decoder: Decoder): ImmutableMap<String, String> {
-    return MapSerializer(keySerializer, valueSerializer).deserialize(decoder).toPersistentMap()
+    return try {
+        mapSerializer.deserialize(decoder).toPersistentMap()
+    } catch (e: Exception) {
+        persistentMapOf()
+    }
 }
Suggestion importance[1-10]: 3

Why: While error handling is generally good practice, silently returning an empty map on any exception is too broad and could hide serious issues. Better to let exceptions propagate for proper handling upstream.

3

@JNdhlovu JNdhlovu merged commit 27e7719 into main Nov 20, 2024
3 checks passed
@JNdhlovu JNdhlovu deleted the bugfix/navserialization branch November 20, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants