GH-61 Add balance top command #67
Conversation
WalkthroughThe changes integrate a leaderboard feature into the economy system. New classes and records manage leaderboard functionality, including services for retrieving and sorting accounts based on their balances, commands to display leaderboard information, and simple data carriers for leaderboard entries and pages. The account management code now tracks accounts by balance and uses updated creation logic. Additionally, new configuration options and permission constants have been added to support leaderboard settings. Message templates for leaderboard notifications have been introduced to improve player communication. Benchmark tests for the leaderboard service and related operations have been added to evaluate performance, while one existing benchmark class has been commented out. Minor modifications in the build configuration add a caching dependency for testing. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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:
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 (
|
eripe14
changed the title
eripe14
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java (1)
26-27: Consider adding size limitsThe top balance size should probably have minimum and maximum values to prevent performance issues.
@Comment("Top balance size") + @Comment("Min: 1, Max: 100") public int topBalanceSize = 10;eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/TopBalanceCommand.java (1)
16-16: Remove unused importThe Random class is imported but never used.
-import java.util.Random;eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java (1)
4-4: Remove unused importThe Not class is imported but never used.
-import com.j256.ormlite.stmt.query.Not;eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java (1)
102-108: Consider caching account positionsThe current implementation works but might be slow for many accounts. Consider caching positions or using an index for better performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java(2 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyPermissionConstant.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java(3 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountPosition.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/TopBalanceCommand.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountPosition.java
🔇 Additional comments (3)
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/TopBalanceCommand.java (1)
51-61: Add null check for player namesSome accounts might have null names, which could cause issues when displaying the list.
for (Account topAccount : this.accountManager.getSortedTopAccounts(topBalanceSize)) { + String playerName = topAccount.name() != null ? topAccount.name() : "Unknown"; this.noticeService.create() .notice(messageConfig -> messageConfig.player.topBalanceEntry) .placeholder("{POSITION}", String.valueOf(position)) - .placeholder("{PLAYER}", topAccount.name()) + .placeholder("{PLAYER}", playerName) .placeholder("{BALANCE}", this.decimalFormatter.format(topAccount.balance())) .player(uuid) .send();✅ Verification successful
Null Check for Player Names Verified
The new code correctly handles potential null player names by using a ternary operator to provide a default value. This approach helps avoid issues when a player's name isn't available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java (1)
70-74: LGTM!Making the save method public is a good choice for better integration with other features.
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (1)
132-133: LGTM!The TopBalanceCommand is properly registered with all needed dependencies.
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyPermissionConstant.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardUpdater.java (2)
11-14: Consider adding error handlingThe
runmethod should catch and handle any exceptions that might occur during the update.Here's a simple way to handle errors:
@Override public void run() { + try { this.leaderboardService.updateTopAccounts(); + } catch (Exception e) { + // Log the error or handle it appropriately + e.printStackTrace(); + } }
3-3: Add class documentationConsider adding a simple comment to explain what this updater does and how often it runs.
Add this documentation:
+/** + * Updates the player balance leaderboard periodically when run. + */ public class LeaderboardUpdater implements Runnable {eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java (1)
27-29: Add validation for leaderboard size.The default value of 10 looks good, but we should add checks to prevent very large or negative values.
@Comment("Leaderboard size") - public int leaderboardSize = 10; + @Min(1) + @Max(100) + public int leaderboardSize = 10;eternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatterUtil.java (1)
17-21: Make the formatter more efficientCreating a new formatter for each call can slow things down. Let's cache it instead.
+ private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern(PATTERN_FORMAT); + public static String format(Instant instant) { - ZonedDateTime zonedDateTime = instant.atZone(ZoneId.of(ZONE)); - DateTimeFormatter formatter = DateTimeFormatter.ofPattern(PATTERN_FORMAT); - return formatter.format(zonedDateTime); + return FORMATTER.format(instant.atZone(ZoneId.of(ZONE))); }eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (2)
31-39: Simplify stream operationsThe code creates a PriorityQueue and then streams it again, which is unnecessary.
Here's a simpler version:
- accounts.stream() - .collect(Collectors.toCollection(() -> - new PriorityQueue<>((a1, a2) -> a2.balance().compareTo(a1.balance())) - )) - .stream() - .limit(this.pluginConfig.leaderboardSize) - .forEach(account -> { - this.topAccounts.computeIfAbsent(account.balance(), (k) -> new ArrayList<>()).add(account); - }); + accounts.stream() + .sorted((a1, a2) -> a2.balance().compareTo(a1.balance())) + .limit(this.pluginConfig.leaderboardSize) + .forEach(account -> + this.topAccounts.computeIfAbsent(account.balance(), k -> new ArrayList<>()).add(account) + );
59-67: Consider caching account positionsLoading all accounts for each position check might be inefficient for frequent requests.
Consider caching positions during
loadTopAccountsand updating them with each refresh.eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (1)
104-110: Simplify update interval calculationThe conversion of Duration to ticks could be clearer.
Consider creating a utility method for converting Duration to ticks:
+ private long durationToTicks(Duration duration) { + return duration.toSeconds() * 20L; + } server.getScheduler().runTaskTimerAsynchronously( this, new LeaderboardUpdater(leaderboardService), 0L, - pluginConfig.leaderboardUpdateInterval.toSeconds() * 20L + durationToTicks(pluginConfig.leaderboardUpdateInterval) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java(4 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java(2 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/TopBalanceCommand.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java(2 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatterUtil.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardUpdater.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/TopBalanceCommand.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java
🧰 Additional context used
🪛 GitHub Actions: Build
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java
[error] 102-102: cannot find symbol: method test() in variable accountManager of type AccountManager
🔇 Additional comments (5)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardUpdater.java (1)
1-16: Nice and clean implementation!The class structure looks good with proper dependency injection.
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java (2)
33-34: Consider performance impact of update interval.A 5-minute update interval might be too frequent for servers with many players. Consider increasing the default or adding a note about adjusting based on server size.
Would you like me to suggest a more conservative default value based on typical server configurations?
30-32: Nice work on the configuration clarity!The comment and boolean flag for showing update time is well-documented and user-friendly.
eternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatterUtil.java (1)
13-15: Nice work on the constructor!Good job preventing instantiation with a clear error message.
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
21-26: Consider potential race condition in constructorThe
lastUpdatedtimestamp is set beforeloadTopAccountsfinishes loading the data asynchronously. This might give incorrect information about when the data was actually loaded.Move line 25 into the completion callback of
loadTopAccounts.
eternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatterUtil.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatterUtil.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountPosition.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/TopBalanceCommand.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java (2)
36-38: Consider allowing longer update intervals.5 minutes is good for small servers, but might be a bit frequent for larger ones.
- public Duration leaderboardUpdateInterval = Duration.ofMinutes(5); + public Duration leaderboardUpdateInterval = Duration.ofMinutes(15);
39-47: Great documentation for time settings!The link to time zones and the clear pattern format explanation are super helpful. Just one small suggestion - consider adding an example of the formatted output in the comments.
@Comment({ "# Pattern format for time", - "# Where 'dd' is day, 'MM' is month, 'yyyy' is year, 'HH' is hour, 'mm' is minute, 'ss' is second" + "# Where 'dd' is day, 'MM' is month, 'yyyy' is year, 'HH' is hour, 'mm' is minute, 'ss' is second", + "# Example output: 25.02.2025 14:30:45" })eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (1)
105-109: Consider adding a small initial delayStarting the leaderboard updates immediately might be too early. A small initial delay would give the system time to properly initialize.
- Duration.ZERO, + Duration.ofSeconds(5), pluginConfig.leaderboardUpdateIntervaleternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatter.java (1)
12-16: Consider validating config values in constructorThe constructor could validate the timezone and pattern format when the class is created.
Here's a suggestion:
private final PluginConfig pluginConfig; public DurationFormatter(PluginConfig pluginConfig) { + if (pluginConfig == null) { + throw new IllegalArgumentException("Config cannot be null"); + } + // Validate timezone and pattern format + try { + ZoneId.of(pluginConfig.timeZone); + DateTimeFormatter.ofPattern(pluginConfig.timePatternFormat); + } catch (Exception e) { + throw new IllegalArgumentException("Invalid timezone or pattern format in config", e); + } this.pluginConfig = pluginConfig; }eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
60-69: Optimize position calculationConsider reusing the already sorted data from topAccounts instead of re-sorting all accounts.
public CompletableFuture<LeaderboardPosition> getLeaderboardPosition(Account targetAccount) { - return this.accountRepository.getAllAccounts().thenApply(accounts -> { - List<Account> sorted = accounts.stream() - .sorted(Comparator.comparing(Account::balance).reversed()) - .toList(); - int position = sorted.indexOf(targetAccount) + 1; + return CompletableFuture.completedFuture( + new LeaderboardPosition( + targetAccount, + findPositionInTopAccounts(targetAccount) + ) + ); } +private int findPositionInTopAccounts(Account targetAccount) { + int position = 1; + for (Map.Entry<BigDecimal, List<Account>> entry : topAccounts.entrySet()) { + if (entry.getValue().contains(targetAccount)) { + return position; + } + position += entry.getValue().size(); + } + return position; }eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java (1)
9-10: Remove unused importsThe Collection and Collections imports appear to be unused after removing the getAccounts method.
-import java.util.Collection; -import java.util.Collections;eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardCommand.java (1)
42-82: Make position tracking more robustThe position counter could get out of sync if the leaderboard updates while processing entries.
Consider getting positions directly from the leaderboard entries:
-int position = 1; +List<Account> leaderboard = this.leaderboardService.getLeaderboard().stream().toList(); -for (Account topAccount : this.leaderboardService.getLeaderboard()) { +for (int i = 0; i < leaderboard.size(); i++) { + Account topAccount = leaderboard.get(i); + int position = i + 1; this.noticeService.create() .notice(messageConfig -> messageConfig.player.leaderboardEntry) .placeholder("{POSITION}", String.valueOf(position)) .placeholder("{PLAYER}", topAccount.name()) .placeholder("{BALANCE}", this.decimalFormatter.format(topAccount.balance())) .player(uuid) .send(); - - position++; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java(4 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java(2 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatter.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardCommand.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardPosition.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardUpdater.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardPosition.java
🚧 Files skipped from review as they are similar to previous changes (2)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardUpdater.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java
🧰 Additional context used
📓 Learnings (1)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalEconomy#67
File: eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java:1-1
Timestamp: 2025-02-04T20:41:28.072Z
Learning: In leaderboard implementations, when grouping by balance, apply size limits after grouping to avoid unfairly clipping accounts with the same balance. PriorityQueue should be avoided in streams as it doesn't guarantee ordered traversal - use simple sorting instead.
🔇 Additional comments (5)
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java (2)
27-29: Nice addition of the leaderboard size config!The default value of 10 is a good choice for most use cases.
30-35: Good user experience settings!These visibility options give users nice flexibility in how the leaderboard appears.
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (1)
104-109: Update scheduler usage for Folia compatibilityThe current scheduler implementation might not work with Folia. Consider using the scheduler pattern from the provided example:
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (2)
44-49:⚠️ Potential issueApply size limit to groups, not individual accounts
The current implementation might exclude accounts that have the same balance as included accounts.
Here's a better approach:
public Collection<Account> getLeaderboard() { - return this.topAccounts.values().stream() - .flatMap(List::stream) - .limit(this.pluginConfig.leaderboardSize) - .toList(); + return this.topAccounts.values().stream() + .limit(this.pluginConfig.leaderboardSize) + .flatMap(List::stream) + .toList(); }Likely invalid or redundant comment.
31-42:⚠️ Potential issueFix leaderboard grouping logic
The current implementation may unfairly exclude accounts with the same balance. Also, the pre-sorting is unnecessary since TreeMap already maintains order.
Try this simpler approach:
private void loadLeaderboard() { this.accountRepository.getAllAccounts().thenAccept(accounts -> { this.topAccounts.clear(); this.topAccounts.putAll(accounts.stream() - .sorted((a1, a2) -> a2.balance().compareTo(a1.balance())) .collect(Collectors.groupingBy( Account::balance, () -> new TreeMap<>(Comparator.reverseOrder()), Collectors.toList() ))); }); }Likely invalid or redundant comment.
eternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatter.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/format/DurationFormatter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
40-78: 🛠️ Refactor suggestionSimplify position calculation.
The position calculation is overly complex and duplicates logic.
public CompletableFuture<LeaderboardPosition> getLeaderboardPosition(Account targetAccount) { - return this.accountRepository.getAllAccounts().thenApply(accounts -> { - Map<BigDecimal, List<Account>> grouped = accounts.stream() - .sorted(Comparator.comparing(Account::balance).reversed()) - .collect(Collectors.groupingBy( - Account::balance, - () -> new TreeMap<>(Comparator.reverseOrder()), - Collectors.toList() - )); - - BigDecimal targetBalance = targetAccount.balance(); - int cumulative = 0; - - for (Map.Entry<BigDecimal, List<Account>> entry : grouped.entrySet()) { - BigDecimal currentBalance = entry.getKey(); - List<Account> group = entry.getValue(); - - int comparison = currentBalance.compareTo(targetBalance); - - if (comparison > 0) { - cumulative += group.size(); - continue; - } - - if (comparison == 0) { - for (Account account : group) { - if (account.equals(targetAccount)) { - return new LeaderboardPosition(targetAccount, cumulative + 1); - } - } - return new LeaderboardPosition(targetAccount, -1); - } - - break; - } - - return new LeaderboardPosition(targetAccount, -1); + return getLeaderboard().thenApply(accounts -> { + int position = accounts.indexOf(targetAccount) + 1; + return new LeaderboardPosition(targetAccount, position == 0 ? -1 : position); }); }
🧹 Nitpick comments (2)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java (2)
45-49: Consider increasing warmup and measurement iterations.For more stable results, consider increasing the warmup and measurement iterations. This will help reduce variance in the benchmark results.
- @Warmup(iterations = 5, time = 1) - @Measurement(iterations = 10, time = 1) + @Warmup(iterations = 10, time = 1) + @Measurement(iterations = 20, time = 1)
50-54: Consider measuring async performance.The current benchmark might not accurately reflect real-world performance as it blocks on the future.
- public void benchmarkGetLeaderboard(Blackhole blackhole) { - CompletableFuture<Collection<Account>> future = this.leaderboardService.getLeaderboard(); - Collection<Account> leaderboard = future.join(); - blackhole.consume(leaderboard); + public void benchmarkGetLeaderboard(Blackhole blackhole) throws Exception { + CompletableFuture<Collection<Account>> future = this.leaderboardService.getLeaderboard(); + blackhole.consume(future); + future.get(5, TimeUnit.SECONDS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
eternaleconomy-core/build.gradle.kts(1 hunks)eternaleconomy-core/src/jmh/java/com/eternalcode/economy/account/AccountBenchmark.java(1 hunks)eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java(4 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardCommand.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java(1 hunks)eternaleconomy-core/src/test/java/com/eternalcode/economy/leaderboard/LeaderboardServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternaleconomy-core/src/jmh/java/com/eternalcode/economy/account/AccountBenchmark.java
🚧 Files skipped from review as they are similar to previous changes (4)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardCommand.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/PluginConfig.java
- eternaleconomy-core/src/test/java/com/eternalcode/economy/leaderboard/LeaderboardServiceTest.java
🧰 Additional context used
📓 Learnings (2)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalEconomy#67
File: eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java:1-1
Timestamp: 2025-02-04T20:41:28.072Z
Learning: In leaderboard implementations, when grouping by balance, apply size limits after grouping to avoid unfairly clipping accounts with the same balance. PriorityQueue should be avoided in streams as it doesn't guarantee ordered traversal - use simple sorting instead.
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalEconomy#67
File: eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java:1-1
Timestamp: 2025-02-04T20:41:28.072Z
Learning: In leaderboard implementations, when grouping by balance, apply size limits after grouping to avoid unfairly clipping accounts with the same balance. PriorityQueue should be avoided in streams as it doesn't guarantee ordered traversal - use simple sorting instead.
🔇 Additional comments (2)
eternaleconomy-core/build.gradle.kts (1)
57-58: Solid addition of the Caffeine dependency!
Adding the Caffeine library as a test dependency is a great move. It will help with caching tests related to the leaderboard functionality, supporting the performance improvements this PR aims for.eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java (1)
27-43: LGTM! Good benchmark setup.The setup creates a realistic test environment with 10,000 accounts and appropriate configuration.
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java
Outdated
Show resolved
Hide resolved
…erboard/LeaderboardService.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…erboard/LeaderboardService.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java
Outdated
Show resolved
Hide resolved
|
Jak dla mnie przepychanki w review bo "eripe" Jak dla mnie to index powinien być w AccountManager bo tam dokonujemy operacji na danych lub jeśli chcecie pobierać dane zawsze z repo to index in-memory jest do śmietnika bo DB ma swoje indexy. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (2)
19-31: Optimize memory usage with pagination.Loading all accounts at once could cause memory issues with large datasets. Consider implementing pagination at the data source level.
- public CompletableFuture<List<LeaderboardEntry>> getLeaderboard() { + public CompletableFuture<List<LeaderboardEntry>> getLeaderboard(int limit) { List<LeaderboardEntry> entries = new ArrayList<>(); int position = 1; + int count = 0; NavigableMap<?, Set<Account>> balanceTiers = accountManager.getAccountsByBalance(); for (Set<Account> accounts : balanceTiers.values()) { for (Account account : accounts) { + if (count >= limit) { + return CompletableFuture.completedFuture(entries); + } entries.add(new LeaderboardEntry(account, position)); + count++; } position += accounts.size(); } return CompletableFuture.completedFuture(entries); }
46-56: Optimize position lookup.Consider using the account's balance to directly access the relevant tier instead of iterating through all tiers.
public CompletableFuture<LeaderboardEntry> getLeaderboardPosition(Account target) { int position = 1; NavigableMap<?, Set<Account>> balanceTiers = accountManager.getAccountsByBalance(); + Set<Account> targetTier = balanceTiers.get(target.balance()); + if (targetTier != null && targetTier.contains(target)) { + return CompletableFuture.completedFuture(new LeaderboardEntry(target, position)); + } - for (Set<Account> accounts : balanceTiers.values()) { - if (accounts.contains(target)) { - return CompletableFuture.completedFuture(new LeaderboardEntry(target, position)); - } - position += accounts.size(); - } return CompletableFuture.completedFuture(new LeaderboardEntry(target, -1)); }eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java (1)
68-93: Reduce code duplication in create methods.Both create methods share similar logic for updating collections. Consider extracting the common code into a private helper method.
+ private void updateCollections(Account account) { + this.accountByUniqueId.put(account.uuid(), account); + this.accountByName.put(account.name(), account); + this.accountIndex.put(account.name(), account); + this.accountsByBalance.computeIfAbsent(account.balance(), k -> ConcurrentHashMap.newKeySet()).add(account); + } public Account create(UUID uuid, String name) { if (this.accountByUniqueId.containsKey(uuid) || this.accountByName.containsKey(name)) { throw new IllegalArgumentException("Account with UUID " + uuid + " already exists"); } Account account = new Account(uuid, name, BigDecimal.ZERO); - this.accountByUniqueId.put(uuid, account); - this.accountByName.put(name, account); - this.accountIndex.put(name, account); - this.accountsByBalance.computeIfAbsent(account.balance(), k -> new HashSet<>()).add(account); + updateCollections(account); return account; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/account/AccountBenchmark.java(1 hunks)eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java(4 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java(5 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java(2 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardCommand.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardEntry.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardPage.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardEntry.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardPage.java
🚧 Files skipped from review as they are similar to previous changes (5)
- eternaleconomy-core/src/jmh/java/com/eternalcode/economy/account/AccountBenchmark.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java
- eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardCommand.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java
🧰 Additional context used
🧠 Learnings (1)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalEconomy#67
File: eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java:1-1
Timestamp: 2025-02-04T20:41:28.072Z
Learning: In leaderboard implementations, when grouping by balance, apply size limits after grouping to avoid unfairly clipping accounts with the same balance. PriorityQueue should be avoided in streams as it doesn't guarantee ordered traversal - use simple sorting instead.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
33-44: Well-implemented pagination logic!The pagination implementation handles edge cases properly and uses appropriate mathematical operations for page calculations.
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Outdated
Show resolved
Hide resolved
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java (1)
37-59: Improve account creation strategy.Creating accounts with random balances might not reflect real-world balance distribution patterns. Also, storing every 100,000th account for position testing could miss edge cases.
Consider:
- if (i % 100_000 == 0) { + // Store accounts at different percentiles for better coverage + if (i % (ACCOUNTS_COUNT / 20) == 0) { targetAccounts.add(account); }eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java (2)
83-94: Consider combining the create methodsYou have two similar create methods. The new one could reuse code from the existing one to avoid duplication.
- public Account create(Account account) { - if (this.accountByUniqueId.containsKey(account.uuid())) { - throw new IllegalArgumentException("Account already exists: " + account.uuid()); - } - - this.accountByUniqueId.put(account.uuid(), account); - this.accountByName.put(account.name(), account); - this.accountIndex.put(account.name(), account); - this.accountsByBalance.put(account.balance(), account); - - return account; - } + public Account create(Account account) { + if (this.accountByUniqueId.containsKey(account.uuid())) { + throw new IllegalArgumentException("Account already exists: " + account.uuid()); + } + return this.save(account); + }
128-134: Consider protecting the balance dataReturning the TreeMultimap directly could let other code modify it. Maybe return an unmodifiable view instead?
- public TreeMultimap<BigDecimal, Account> getAccountsByBalance() { - return this.accountsByBalance; + public TreeMultimap<BigDecimal, Account> getAccountsByBalance() { + return TreeMultimap.create(this.accountsByBalance); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eternaleconomy-core/build.gradle.kts(1 hunks)eternaleconomy-core/src/jmh/java/com/eternalcode/economy/account/AccountBenchmark.java(3 hunks)eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java(1 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java(5 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- eternaleconomy-core/src/jmh/java/com/eternalcode/economy/account/AccountBenchmark.java
- eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java
- eternaleconomy-core/build.gradle.kts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java (2)
24-33: Consider adjusting benchmark parameters for more realistic testing.The current setup with 10 iterations might not give stable results. Also, 2M accounts might not represent real-world data size.
Would you like me to suggest more appropriate benchmark parameters based on your actual production data size?
1-82: Address database indexing concerns.Based on Rollczi's comments, this benchmark might not accurately represent real-world performance since it uses in-memory storage instead of testing actual database indexing.
Would you like me to help create additional benchmarks that test with your actual database implementation?
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java (3)
19-22: Consider making the balance tracking thread-safeThe accountsByBalance map might have issues if multiple threads access it at once. Using concurrent collections would help prevent problems.
37-38: Initialization looks good!The balance tracking is properly set up during account creation.
96-116: Clean implementation of account updates!The save and remove methods handle all collections properly and keep everything in sync.
...onomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java
Outdated
Show resolved
Hide resolved
| @@ -22,6 +21,8 @@ public class AccountBenchmark { | |||
|
|
|||
| private final List<String> searches = new ArrayList<>(); | |||
| private AccountManager accountManager; | |||
| // mimo że nie jest to bezpieczne dla wielu wątków, to w przypadku JMH można to zignorować i tak potrzebujemy losowości | |||
There was a problem hiding this comment.
| // mimo że nie jest to bezpieczne dla wielu wątków, to w przypadku JMH można to zignorować i tak potrzebujemy losowości | |
| // While this is not multithread-safe, in case of JMH we can ignore it, since we need randomness anyway |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
21-53: Handle ties carefully.This approach might cut off accounts that share the same balance near the page boundary. Consider grouping by balance first and then slicing to avoid confusion for accounts tied on the same balance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java(4 hunks)eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalEconomy#67
File: eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java:1-1
Timestamp: 2025-02-04T20:41:28.072Z
Learning: In leaderboard implementations, when grouping by balance, apply size limits after grouping to avoid unfairly clipping accounts with the same balance. PriorityQueue should be avoided in streams as it doesn't guarantee ordered traversal - use simple sorting instead.
🔇 Additional comments (4)
eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java (1)
55-66: Ensure concurrency safety.These operations rely on data from
AccountManager, which might change in parallel. Check ifAccountManageris thread-safe or if you should lock the data structures before iterating.eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java (3)
90-102: Check existing accounts.You're correctly preventing duplicates, but ensure that both name and UUID checks stay in sync so there’s no partial mismatch (e.g., same name but different UUID).
104-118: Good call removing the old account first.This method neatly handles balance changes by removing the old record. Looks clear and straightforward!
138-148: Validate usage of these new getters.Your new getters (
getAccountsByBalance,getAccountsByBalanceSet, andgetAccountsCount) expose internal data structures. Verify that consumers handle any potential mutation or concurrency concerns properly.
eternaleconomy-core/src/main/java/com/eternalcode/economy/account/AccountManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java (1)
77-82: Consider adding edge case testsYour benchmark checks normal cases but doesn't test edge cases like looking up users with very low balances.
Try adding a test for the worst-case scenario:
@Benchmark public void benchmarkGetLeaderboardPosition(Blackhole blackhole) { Account target = targetAccounts.get(ThreadLocalRandom.current().nextInt(targetAccounts.size())); CompletableFuture<LeaderboardEntry> future = leaderboardService.getLeaderboardPosition(target); blackhole.consume(future.join()); } +@Benchmark +public void benchmarkGetLeaderboardPositionWorstCase(Blackhole blackhole) { + // Get the account with the lowest balance + Account target = targetAccounts.get(2); // Using the lowest balance account from setup + CompletableFuture<LeaderboardEntry> future = leaderboardService.getLeaderboardPosition(target); + blackhole.consume(future.join()); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java (1)
Learnt from: vLuckyyy
PR: EternalCodeTeam/EternalEconomy#67
File: eternaleconomy-core/src/main/java/com/eternalcode/economy/leaderboard/LeaderboardService.java:1-1
Timestamp: 2025-02-04T20:41:28.072Z
Learning: In leaderboard implementations, when grouping by balance, apply size limits after grouping to avoid unfairly clipping accounts with the same balance. PriorityQueue should be avoided in streams as it doesn't guarantee ordered traversal - use simple sorting instead.
🔇 Additional comments (4)
eternaleconomy-core/src/jmh/java/com/eternalcode/economy/leaderboard/LeaderboardServiceBenchmark.java (4)
29-34: Good job with the benchmark setup!The warmup iterations and configuration look right for getting reliable results.
40-41: Nice range of test data!Testing with different account counts (100k, 1M, 2M) will help you understand how well this scales.
53-59: Smart choice using Pareto distributionUsing a Pareto distribution for balances makes your benchmark more realistic since it matches real-world wealth distribution patterns.
70-75: Benchmark looks good but needs concurrent testingThe benchmark tests page retrieval correctly but doesn't check how it performs when multiple users access it at once.
Consider adding a test for concurrent access:
@Benchmark public void benchmarkGetLeaderboardPage(Blackhole blackhole) { int randomPage = ThreadLocalRandom.current().nextInt(1, MAX_PAGES + 1); CompletableFuture<LeaderboardPage> future = leaderboardService.getLeaderboardPage(randomPage, PAGE_SIZE); blackhole.consume(future.join()); } +@Benchmark +@Threads(8) +public void benchmarkGetLeaderboardPageConcurrent(Blackhole blackhole) { + int randomPage = ThreadLocalRandom.current().nextInt(1, MAX_PAGES + 1); + CompletableFuture<LeaderboardPage> future = leaderboardService.getLeaderboardPage(randomPage, PAGE_SIZE); + blackhole.consume(future.join()); +}
As @vLuckyyy:
I refactored generally this code.
The code is so fast, I decided to remove the time top refresh.
Screenshots:
2025-02-09.19-02-09.mp4
Result Benchmark of LeadeboardService (maybe rollczi can optimize more):