GH-939 Fix /tprp command often teleports to the same player repeatedly
#941
Conversation
WalkthroughThe changes remove the old command that teleported a player to a random online player. In its place, a new service is added to track teleport history between players with a 30-minute expiry. A new command uses this service to teleport a player to someone they haven’t teleported to recently, respecting configuration settings. The command also sends notifications to the player about success or failure. This update splits the teleport logic into a dedicated service and a command class, replacing the previous single-command approach. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 6
🧹 Nitpick comments (6)
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java (3)
23-23: Consider memory management for the teleportation history.The static map will grow indefinitely as players join the server, but entries are never removed. This could lead to a memory leak over time.
Consider adding a cleanup mechanism, such as:
- Removing entries for players who haven't been online for a certain period
- Using a cache with size/time-based eviction
- Clearing entries when the server restarts
40-57: Verify the player selection logic handles edge cases.The method looks correct but let's ensure it handles all scenarios properly. The logic should work but could be simplified.
Consider this cleaner approach:
public Player findLeastRecentlyTeleportedPlayer() { List<Player> eligiblePlayers = getEligiblePlayers(); if (eligiblePlayers.isEmpty()) { return null; } return eligiblePlayers.stream() .min(Comparator.comparing( p -> TELEPORTATION_HISTORY.getOrDefault(p.getUniqueId(), Instant.EPOCH) )) .orElse(null); }The
nullsLastcomparator isn't needed sincegetOrDefaulthandles missing entries.
47-49: Simplify single player case handling.The early return for single player is good, but the logic could be cleaner.
-if (eligiblePlayers.size() == 1) { - return eligiblePlayers.get(0); -} +if (eligiblePlayers.size() <= 1) { + return eligiblePlayers.isEmpty() ? null : eligiblePlayers.get(0); +}This consolidates the empty and single player cases.
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java (3)
44-44: Consider validating target player is still online.There's a small chance the target player could disconnect between selection and teleportation.
Add a quick online check before teleporting:
+if (!targetPlayer.isOnline()) { + this.noticeService.create() + .player(player.getUniqueId()) + .notice(translation -> translation.teleport().randomPlayerNotFound()) + .send(); + return; +} + player.teleport(targetPlayer);
32-40: Consider checking if target is the same as executor.The command might teleport a player to themselves if they're the only eligible player, which could be confusing.
Add a check after finding the target:
Player targetPlayer = this.teleportRandomPlayerService.findLeastRecentlyTeleportedPlayer(); +if (targetPlayer != null && targetPlayer.equals(player)) { + this.noticeService.create() + .player(player.getUniqueId()) + .notice(translation -> translation.teleport().cannotTeleportToSelf()) + .send(); + return; +} + if (targetPlayer == null) {
31-32: Consider filtering out the executing player.The current implementation could teleport a player to themselves if they're the only eligible player.
Consider modifying the service call to exclude the current player:
Player targetPlayer = this.teleportRandomPlayerService.findLeastRecentlyTeleportedPlayer(player);And update the service method to filter out the executing player from eligible targets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleport/command/TeleportToRandomPlayerCommand.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java(1 hunks)
💤 Files with no reviewable changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/teleport/command/TeleportToRandomPlayerCommand.java
👮 Files not reviewed due to content moderation or server errors (2)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java
🔇 Additional comments (11)
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java (5)
34-38: Good implementation of player filtering.The eligible players logic correctly filters based on the operator configuration setting.
59-61: Good history tracking implementation.The update method correctly records the current timestamp for the teleported player.
51-56: Good implementation of least recently teleported logic.The comparator logic correctly handles the case where players haven't been teleported before by using
Instant.EPOCHas default.
40-57: Logic looks good but consider self-teleportation edge case.The method correctly finds the least recently teleported player using a clean comparator approach.
However, consider what happens if the executing player is the only eligible player - they would teleport to themselves. You might want to filter out the executing player from eligible targets.
52-54: Good use of Instant.EPOCH as default value.Using
Instant.EPOCHas the default ensures players who haven't been teleported to will be prioritized, which is exactly the intended behavior.eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java (6)
13-14: Good command setup with clear naming.The command name and alias are appropriate, and the permission is properly configured.
29-51: Good use of service pattern and error handling.The command properly uses the service to find targets and handles the null case with appropriate user feedback.
46-50: Good notice implementation with placeholder.The success notice correctly uses the target player's name as a placeholder, providing clear feedback to the user.
29-31: Excellent documentation and command setup.The description clearly explains the fair distribution logic, and the command annotation is properly configured.
32-40: Good error handling for no available targets.The null check and user notification is well implemented.
42-44: Correct order of operations.Updating the teleportation history before the actual teleport is the right approach to ensure accurate tracking.
...main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java
Outdated
Show resolved
Hide resolved
...in/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java
Show resolved
Hide resolved
...in/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java (1)
46-48: Add validation for sender and target.Consider adding a check to ensure sender and target are different players to prevent unnecessary cache entries.
public void updateTeleportationHistory(Player sender, Player target) { + if (sender.equals(target)) { + return; + } teleportationHistory.put(new HistoryKey(sender.getUniqueId(), target.getUniqueId()), Instant.now()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java (3)
23-25: Great improvement with Caffeine cache!The cache with 30-minute expiration nicely solves the memory management issues from previous implementations.
42-44: Good use of cache default value.Using
Instant.EPOCHas the default ensures players with no teleportation history are prioritized correctly.
50-51: Clean record implementation.The HistoryKey record provides a clear and efficient way to store teleportation pairs in the cache.
...main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java
Show resolved
Hide resolved
Rollczi
left a comment
Rollczi
left a comment
There was a problem hiding this comment.
I have done code review via commit. Check out my changes, everythink looks good? I added the key because we want to handle multiple admins.
...main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java
Outdated
Show resolved
Hide resolved
...in/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportToRandomPlayerCommand.java
Outdated
Show resolved
Hide resolved
CitralFlo
left a comment
CitralFlo
left a comment
There was a problem hiding this comment.
Minor this and that. I like usage of Caffeine.
Tprp will work better than spotify random play (I still get the same messages)
...main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/core/feature/teleportrandomplayer/TeleportRandomPlayerService.java
Outdated
Show resolved
Hide resolved
No description provided.