Avoid unnecessary map instance creations on Presences #200
Conversation
WalkthroughThe recent updates focus on enhancing the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- yorkie/src/main/kotlin/dev/yorkie/core/Presences.kt (1 hunks)
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (1 hunks)
Additional comments not posted (6)
yorkie/src/main/kotlin/dev/yorkie/core/Presences.kt (5)
14-14: The implementation of theplusoperator is correct and adheres to the immutability requirement by creating a newPresencesinstance.
18-18: The implementation of theminusoperator is correct and adheres to the immutability requirement by creating a newPresencesinstance.
27-31: TheasPresencesfunction correctly handles both cases where the input is already aPresencesinstance and when it's not, ensuring immutability.
35-35: TheasPresencesfunction for aPair<ActorID, P>correctly creates a newPresencesinstance, ensuring immutability.
8-8: Ensure that all usages ofmapin thePresencesclass handle the change fromMutableMaptoMapcorrectly.Verification successful
The script executed successfully and did not find any mutable operations (
put,remove,clear,putAll) on themapwithin thePresencesclass. This indicates that the change fromMutableMaptoMaphas been handled correctly, as no mutable operations are being performed on themap.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all usages of `map` in the `Presences` class handle the change from `MutableMap` to `Map` correctly. # Test: Search for the usage of `map` in the `Presences` class. Expect: No mutable operations on `map`. ast-grep --lang kotlin --pattern $'class Presences { $$$ map($_, $_) { $$$ } $$$ }'Length of output: 582
Script:
#!/bin/bash # Description: Verify that all usages of `map` in the `Presences` class handle the change from `MutableMap` to `Map` correctly. # Find the definition of the `Presences` class to get its location presences_class_file=$(fd Presences.kt) # Search for all usages of `map` within the `Presences` class and check for mutable operations rg --pcre2 --glob "$presences_class_file" '(map\s*=\s*|map\.\s*(put|remove|clear|putAll))'Length of output: 214
yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (1)
114-114: The modification to thestateInmethod simplifies the initialization of thepresencesproperty by directly using_presences.valueinstead of converting it withasPresences(). This change is consistent with the PR's objective to reduce unnecessary instance creations.
What this PR does / why we need it?
Each time there is a change in presence, we create a new Presences instance for StateFlow. And yet we used a MutableMap, which resulted in multiple toMutableMap invocations.
Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit