feat: Fishing System v2#6
Conversation
* Reworked fishing system * Added Javadoc for classes and methods * Added demo loot pools
WalkthroughThe PR introduces permission enforcement guards in admin commands, systematically renames the currency system from "favors" to "gems" across multiple components, completely refactors the fishing loot system from an interface-based design to a concrete builder pattern, adds the Dummy entity, and makes various refinements to entity handling, data persistence, and utility classes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
MrInspection
added
area: engine
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/fr/moussax/blightedMC/core/entities/spawnable/SpawnableEntityProfile.java (1)
36-43: Add missing validation for positive group size values.The method validates
max >= minbut doesn't ensure both values are actually positive. According to the AI summary, themaxparameter should also be annotated with@Positive. More importantly, negative group sizes likegroupSize(-5, -1)would currently be accepted.Apply this diff to add proper validation:
- public SpawnableEntityProfile groupSize(@Positive int min, int max) { + public SpawnableEntityProfile groupSize(@Positive int min, @Positive int max) { + if (min <= 0 || max <= 0) { + throw new IllegalArgumentException("Group sizes must be positive: min=" + min + ", max=" + max); + } if (max < min) { throw new IllegalArgumentException("Invalid group size: min=" + min + ", max=" + max); }src/main/java/fr/moussax/blightedMC/core/entities/loot/gems/GemsItem.java (1)
20-88: Persistent data key mismatch breaks redemptionNewly created gemstones still store their amount under
"favorsValue", but the constructor now only reads"gemsValue". Every fresh item therefore redeems as 1 gem, regardless of the intended amount. Existing legacy items also lose their value once picked up because we never read the old key.Please read both keys (new first, old as a fallback) and write the new key when generating items—optionally mirroring the legacy key to stay backward compatible:
- Integer value = meta.getPersistentDataContainer() - .get(new NamespacedKey(BlightedMC.getInstance(), "gemsValue"), PersistentDataType.INTEGER); - this(value != null ? value : 1); + NamespacedKey gemsKey = new NamespacedKey(BlightedMC.getInstance(), "gemsValue"); + NamespacedKey legacyKey = new NamespacedKey(BlightedMC.getInstance(), "favorsValue"); + Integer value = meta.getPersistentDataContainer().get(gemsKey, PersistentDataType.INTEGER); + if (value == null) { + value = meta.getPersistentDataContainer().get(legacyKey, PersistentDataType.INTEGER); + } + this(value != null ? value : 1); ... - meta.getPersistentDataContainer().set(new NamespacedKey(BlightedMC.getInstance(), "favorsValue"), PersistentDataType.INTEGER, amount); + NamespacedKey gemsKey = new NamespacedKey(BlightedMC.getInstance(), "gemsValue"); + NamespacedKey legacyKey = new NamespacedKey(BlightedMC.getInstance(), "favorsValue"); + meta.getPersistentDataContainer().set(gemsKey, PersistentDataType.INTEGER, amount); + meta.getPersistentDataContainer().set(legacyKey, PersistentDataType.INTEGER, amount);src/main/java/fr/moussax/blightedMC/commands/admin/GemsCommand.java (1)
127-156: Tweak gems messaging for consistencyThe player-facing strings still say “You gems has been set” and “players favor”, which is confusing after the currency rename.
- target.sendMessage("§8 ■ §7You gems has been set to §d" + amount + "§7."); + target.sendMessage("§8 ■ §7Your gems have been set to §d" + amount + "§7."); ... - sender.sendMessage("§8 ■ §7All online players favor have been reset to §d0§7."); + sender.sendMessage("§8 ■ §7All online players' gems have been reset to §d0§7.");
🧹 Nitpick comments (4)
.idea/artifacts/blightedmc_jar.xml (1)
1-8: Consider excluding IDE configuration files from version control.This is an IntelliJ IDEA artifact configuration file containing local development environment paths (
$USER_HOME$paths). IDE configuration files (.idea/directory) are typically environment-specific and are often excluded from version control to prevent merge conflicts and environment-specific pollution.The changes themselves (artifact name capitalization and output path) are non-functional and environment-specific. However, if not already present, consider adding the
.idea/directory to.gitignoreto follow best practices.If you'd like, verify the current state of your
.gitignore:#!/bin/bash # Check if .idea is in .gitignore grep -E '^\s*\.idea' .gitignore || echo ".idea is not explicitly listed in .gitignore"src/main/java/fr/moussax/blightedMC/utils/sound/SoundSequence.java (1)
6-6: Consider using explicit imports instead of wildcard.Wildcard imports can reduce code clarity by obscuring which specific classes are in use and may introduce naming conflicts.
Apply this diff to restore explicit imports:
-import java.util.*; +import java.util.Arrays; +import java.util.List; +import java.util.Objects;src/main/java/fr/moussax/blightedMC/gameplay/abilities/BonemerangAbility.java (1)
210-216: Consider simplifying the entity type check.The current pattern checks
instanceof Damageable, casts todamageable, then requiresinstanceof LivingEntity. SinceLivingEntityimplementsDamageable, you can simplify:private void damageNearbyEntities(ArmorStand projectile, Player player) { for (Entity entity : projectile.getNearbyEntities(COLLISION_RADIUS, 1, COLLISION_RADIUS)) { - if (entity instanceof Damageable damageable && entity != player && entity instanceof LivingEntity) { - damageable.damage(DAMAGE_AMOUNT, player); + if (entity instanceof LivingEntity livingEntity && entity != player) { + livingEntity.damage(DAMAGE_AMOUNT, player); } } }src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootPool.java (1)
83-105: Remove Java version concern; fallback logic is optional refactoring.The Java version check is resolved—the project targets Java 25, well above the Java 21 requirement for
getLast().The fallback at line 104 remains a valid observation: the weighted selection loop should always return when
validEntriesis non-empty, making the fallback theoretically unreachable. Consider whether this is defensive coding or can be simplified for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
.idea/artifacts/blightedmc_jar.xml(1 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/BroadcastCommand.java(1 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/FlyCommand.java(1 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/ForceCommand.java(1 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/GemsCommand.java(7 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/GiveItemCommand.java(1 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/GodCommand.java(1 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/KaboomCommand.java(1 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/LoopCommand.java(1 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/SpawnCustomMobCommand.java(2 hunks)src/main/java/fr/moussax/blightedMC/commands/admin/TeleportCommands.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/entities/listeners/BlightedEntitiesListener.java(3 hunks)src/main/java/fr/moussax/blightedMC/core/entities/loot/gems/GemsItem.java(3 hunks)src/main/java/fr/moussax/blightedMC/core/entities/loot/gems/GemsLoot.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/entities/registry/EntitiesRegistry.java(3 hunks)src/main/java/fr/moussax/blightedMC/core/entities/spawnable/SpawnConditions.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/entities/spawnable/SpawnableEntityProfile.java(4 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/FishingListener.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/FishingLootTable.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/FishingSystem.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/FishingLootTable.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootCondition.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootContext.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootEntry.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootPool.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/pools/EndFishingPool.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/pools/OverworldFishingPool.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/environment/EndFishing.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/environment/OverworldFishing.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootCondition.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootContext.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootEntry.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootPool.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/fishing/package-info.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/items/ItemTemplate.java(0 hunks)src/main/java/fr/moussax/blightedMC/core/items/registry/ItemDirectory.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/items/registry/ItemDirectoryMenu.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/players/BlightedPlayer.java(2 hunks)src/main/java/fr/moussax/blightedMC/core/players/data/PlayerData.java(1 hunks)src/main/java/fr/moussax/blightedMC/core/players/data/PlayerDataHandler.java(1 hunks)src/main/java/fr/moussax/blightedMC/gameplay/abilities/BonemerangAbility.java(1 hunks)src/main/java/fr/moussax/blightedMC/gameplay/abilities/RocketBootsAbility.java(2 hunks)src/main/java/fr/moussax/blightedMC/gameplay/entities/Dummy.java(3 hunks)src/main/java/fr/moussax/blightedMC/utils/formatting/CommandInfo.java(2 hunks)src/main/java/fr/moussax/blightedMC/utils/formatting/Formatter.java(11 hunks)src/main/java/fr/moussax/blightedMC/utils/sound/SoundSequence.java(1 hunks)
💤 Files with no reviewable changes (9)
- src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/pools/OverworldFishingPool.java
- src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootPool.java
- src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/pools/EndFishingPool.java
- src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootCondition.java
- src/main/java/fr/moussax/blightedMC/core/fishing/FishingSystem.java
- src/main/java/fr/moussax/blightedMC/core/items/ItemTemplate.java
- src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/FishingLootTable.java
- src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootEntry.java
- src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootContext.java
🧰 Additional context used
🧬 Code graph analysis (13)
src/main/java/fr/moussax/blightedMC/core/entities/registry/EntitiesRegistry.java (2)
src/main/java/fr/moussax/blightedMC/gameplay/entities/Dummy.java (1)
Dummy(15-66)src/main/java/fr/moussax/blightedMC/core/entities/registry/SpawnableEntitiesRegistry.java (1)
SpawnableEntitiesRegistry(9-33)
src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootPool.java (2)
src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootEntry.java (1)
LootEntry(20-199)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootPool.java (1)
LootPool(8-43)
src/main/java/fr/moussax/blightedMC/commands/admin/FlyCommand.java (1)
src/main/java/fr/moussax/blightedMC/utils/formatting/Formatter.java (1)
Formatter(33-312)
src/main/java/fr/moussax/blightedMC/core/fishing/environment/EndFishing.java (2)
src/main/java/fr/moussax/blightedMC/core/fishing/FishingLootTable.java (1)
FishingLootTable(21-165)src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootEntry.java (1)
LootEntry(20-199)
src/main/java/fr/moussax/blightedMC/core/fishing/FishingListener.java (3)
src/main/java/fr/moussax/blightedMC/core/fishing/environment/EndFishing.java (1)
EndFishing(9-28)src/main/java/fr/moussax/blightedMC/core/fishing/environment/OverworldFishing.java (1)
OverworldFishing(12-35)src/main/java/fr/moussax/blightedMC/core/players/BlightedPlayer.java (1)
BlightedPlayer(20-216)
src/main/java/fr/moussax/blightedMC/commands/admin/ForceCommand.java (1)
src/main/java/fr/moussax/blightedMC/utils/formatting/CommandInfo.java (1)
CommandInfo(25-206)
src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootContext.java (2)
src/main/java/fr/moussax/blightedMC/core/players/BlightedPlayer.java (1)
BlightedPlayer(20-216)src/main/java/fr/moussax/blightedMC/core/fishing/LootTable/LootContext.java (1)
LootContext(7-8)
src/main/java/fr/moussax/blightedMC/core/fishing/FishingLootTable.java (3)
src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootEntry.java (1)
LootEntry(20-199)src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootPool.java (1)
LootPool(15-124)src/main/java/fr/moussax/blightedMC/core/players/BlightedPlayer.java (1)
BlightedPlayer(20-216)
src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootEntry.java (2)
src/main/java/fr/moussax/blightedMC/core/entities/BlightedEntity.java (1)
BlightedEntity(32-436)src/main/java/fr/moussax/blightedMC/core/players/BlightedPlayer.java (1)
BlightedPlayer(20-216)
src/main/java/fr/moussax/blightedMC/commands/admin/GemsCommand.java (1)
src/main/java/fr/moussax/blightedMC/utils/formatting/CommandInfo.java (1)
CommandInfo(25-206)
src/main/java/fr/moussax/blightedMC/commands/admin/TeleportCommands.java (1)
src/main/java/fr/moussax/blightedMC/utils/formatting/CommandInfo.java (1)
CommandInfo(25-206)
src/main/java/fr/moussax/blightedMC/core/fishing/environment/OverworldFishing.java (3)
src/main/java/fr/moussax/blightedMC/core/fishing/FishingLootTable.java (1)
FishingLootTable(21-165)src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootEntry.java (1)
LootEntry(20-199)src/main/java/fr/moussax/blightedMC/gameplay/entities/Dummy.java (1)
Dummy(15-66)
src/main/java/fr/moussax/blightedMC/core/entities/loot/gems/GemsItem.java (2)
src/main/java/fr/moussax/blightedMC/BlightedMC.java (1)
BlightedMC(9-46)src/main/java/fr/moussax/blightedMC/utils/formatting/Formatter.java (1)
Formatter(33-312)
🔇 Additional comments (31)
src/main/java/fr/moussax/blightedMC/core/fishing/package-info.java (1)
1-7: LGTM!The package documentation is clear, well-formatted, and accurately describes the fishing system's responsibilities. The Javadoc follows standard conventions and aligns with the refactored fishing loot system introduced in this PR.
src/main/java/fr/moussax/blightedMC/utils/sound/SoundSequence.java (1)
9-11: Excellent use ofList.copyOffor stronger immutability.The switch from
Collections.unmodifiableListtoList.copyOfprevents aliasing—modifications to the original list after construction won't affect this instance. This aligns well with the immutable record pattern.src/main/java/fr/moussax/blightedMC/gameplay/abilities/BonemerangAbility.java (1)
60-63: Verify the zero-second cooldown is intentional.The method returns
0, which means the cooldown system at Line 50 sets expiry to the current timestamp. This makes the cooldown check at Line 86 always returnfalseimmediately, effectively disabling cooldowns.If this is a placeholder, consider setting a non-zero default (e.g.,
5seconds). If instant reuse is intended, the entire cooldown tracking system could be simplified or removed.src/main/java/fr/moussax/blightedMC/core/entities/spawnable/SpawnableEntityProfile.java (2)
54-68: LGTM!The new getter methods are well-implemented and follow standard Java bean conventions. They provide appropriate read access to the profile's configuration properties.
78-78: LGTM!The error message now correctly references the class name
SpawnableEntityProfile, improving error clarity.src/main/java/fr/moussax/blightedMC/core/items/registry/ItemDirectory.java (1)
36-36: No thread safety concerns detected. The change is safe for Bukkit plugin architecture.The codebase uses Bukkit's plugin lifecycle pattern where initialization only occurs in
onEnable()(single call at line 17 of BlightedMC.java) and cleanup only inonDisable()(line 40). For/reloadscenarios, Bukkit invokes the normal disable/enable cycle. There are no concurrent calls toinitializeAllRegistries(), and Bukkit event handlers run on a single server thread, eliminating race conditions. The addition ofclearItems()before registration makesinitializeItems()idempotent without introducing safety issues—it simply makes individual registries self-contained rather than relying on CoreRegistry-level clearing.src/main/java/fr/moussax/blightedMC/core/entities/spawnable/SpawnConditions.java (1)
23-25: Behavior change confirmed: surface-level spawns are now excluded.The actual change from the git history confirms the review comment: the condition was changed from
==to<, which means theskyExposed()method now returnstrueonly when a location is floating above the highest non-air block, not when it's exactly at the surface level.
- Old behavior (
==): Spawn only at the surface block level- New behavior (
<): Spawn only in air above the surfaceThis is a breaking semantic change for entity spawning. The method is currently used in BlightedZombie spawning (line 178), meaning zombies will no longer spawn on the ground surface itself.
The question is whether this is intentional or a regression:
- If spawning should include the surface level (typical for "sky exposed" checks), the condition should be
<=- If spawning should only occur floating in air (strict interpretation), the current
<is correctVerify that all entities using
skyExposed()have been tested and behave as expected with this stricter condition, or confirm whether<=was the intended boundary.src/main/java/fr/moussax/blightedMC/gameplay/abilities/RocketBootsAbility.java (1)
98-121: Durability loss now respects Unbreaking.The per-point roll mirrors vanilla’s 1/(level+1) loss chance and the break cleanup is tidy. Nicely done.
src/main/java/fr/moussax/blightedMC/core/players/data/PlayerData.java (1)
9-12: LGTM!The constructor parameter and field assignment have been cleanly renamed from
favorstogems, aligning with the broader currency system rename across the PR.src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootPool.java (3)
26-51: LGTM!The three overloaded
addmethods provide a flexible API for adding entries. The builder pattern withreturn thisenables clean method chaining.
71-74: LGTM!The clamping logic ensures
rollChancestays within the valid probability range [0.0, 1.0], preventing invalid configurations.
112-123: LGTM!The helper methods
isEmpty()andsize()provide a clean API for checking the pool state.src/main/java/fr/moussax/blightedMC/core/fishing/loot/LootCondition.java (2)
23-38: LGTM!The null-safe canonical constructor and
testmethod are well-implemented. The semantics are clear: empty sets mean "no restriction," and both biome and environment conditions must be satisfied.
41-63: LGTM!The factory methods provide clear, convenient ways to create common conditions. Using
Collections.singletonensures immutability.src/main/java/fr/moussax/blightedMC/core/entities/registry/EntitiesRegistry.java (2)
18-21: LGTM!Adding the call to
SpawnableEntitiesRegistry.clearEntities()ensures that spawnable entities are properly cleared alongside regular entities, maintaining consistency across both registries.
52-52: LGTM!The Dummy entity registration follows the established pattern and integrates cleanly with the existing entity system.
src/main/java/fr/moussax/blightedMC/commands/admin/KaboomCommand.java (1)
23-23: LGTM!The permission guard now properly prevents command execution when the admin permission check fails. This security improvement aligns with the broader pattern applied across admin commands in this PR.
src/main/java/fr/moussax/blightedMC/commands/admin/LoopCommand.java (1)
27-27: LGTM!The permission guard is especially important for this command, as it enables bulk command execution. Proper authorization prevents potential abuse.
src/main/java/fr/moussax/blightedMC/core/players/BlightedPlayer.java (1)
173-183: LGTM!The method renames from
addFavors/removeFavorstoaddGems/removeGems, along with the persistence update tosetGems, are consistent with the broader currency system rename across the PR.Also applies to: 200-200
src/main/java/fr/moussax/blightedMC/core/entities/loot/gems/GemsLoot.java (1)
13-13: LGTM!The update from
addFavorstoaddGemscorrectly reflects the renamed API inBlightedPlayer, maintaining consistency with the currency system changes.src/main/java/fr/moussax/blightedMC/commands/admin/GodCommand.java (1)
17-17: Permission guard correctly implemented.The early return pattern properly enforces admin permissions before executing the command logic.
src/main/java/fr/moussax/blightedMC/commands/admin/BroadcastCommand.java (1)
18-18: Permission guard correctly implemented.The early return pattern properly enforces admin permissions before executing the command logic.
src/main/java/fr/moussax/blightedMC/commands/admin/ForceCommand.java (2)
21-21: Permission guard correctly implemented.The early return pattern properly enforces admin permissions before executing the command logic.
24-25: Improved readability with multi-line format.Splitting the usage message across multiple lines enhances code readability.
src/main/java/fr/moussax/blightedMC/commands/admin/SpawnCustomMobCommand.java (2)
20-20: Permission guard correctly implemented.The early return pattern properly enforces admin permissions before executing the command logic.
39-39: Improved error message specificity.Including the entity name in the error message provides clearer feedback to the user about which entity failed to spawn.
src/main/java/fr/moussax/blightedMC/commands/admin/GiveItemCommand.java (1)
23-23: Permission guard correctly implemented.The early return pattern properly enforces admin permissions before executing the command logic.
src/main/java/fr/moussax/blightedMC/commands/admin/FlyCommand.java (2)
10-10: Static import appropriately added.The static import for
enforceAdminPermissionimproves code readability.
17-18: Permission guard correctly implemented with good spacing.The early return pattern properly enforces admin permissions, and the blank line improves code readability by separating the guard from the main logic.
src/main/java/fr/moussax/blightedMC/utils/formatting/CommandInfo.java (2)
103-105: Input validation appropriately added.The validation prevents potential issues downstream when accessing array indices. The error message mentions "null or empty" but only checks for empty arrays; however, the
@Nonnullannotation on the parameter already prevents null values, making this acceptable.
170-172: Input validation appropriately added.The validation prevents potential issues downstream when accessing array indices. The error message mentions "null or empty" but only checks for empty arrays; however, the
@Nonnullannotation on the parameter already prevents null values, making this acceptable.
| import org.checkerframework.checker.index.qual.Positive; | ||
|
|
||
| import javax.annotation.Nonnull; |
There was a problem hiding this comment.
Verify the @Positive annotation source and remove unused imports.
Two concerns with the imports:
- The
@Positiveannotation from Checkerframework (org.checkerframework.checker.index.qual.Positive) is for static analysis only and provides no runtime validation. If runtime validation is intended, consider usingjavax.validation.constraints.Positivefrom Bean Validation (JSR 380) instead, or rely solely on the explicit runtime checks you've added. - The
@Nonnullimport on line 7 appears unused in this file.
</invoke_comment_end -->
| public SpawnableEntityProfile weight(@Positive double weight) { | ||
| if (weight < 0.00) { | ||
| throw new IllegalArgumentException("Weight cannot be negative: " + weight); | ||
| } | ||
| this.weight = weight; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Align validation logic with the @Positive annotation semantics.
The @Positive annotation typically means values strictly greater than zero, but your validation check weight < 0.00 permits weight = 0.0. This creates a semantic mismatch.
If zero is a valid weight, consider using @PositiveOrZero instead, or if weights must be strictly positive, update the validation to weight <= 0.
Apply this diff if weights must be strictly positive:
- if (weight < 0.00) {
+ if (weight <= 0) {
throw new IllegalArgumentException("Weight cannot be negative: " + weight);
}Note: Also update the error message to say "non-positive" rather than "negative" if you make this change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public SpawnableEntityProfile weight(@Positive double weight) { | |
| if (weight < 0.00) { | |
| throw new IllegalArgumentException("Weight cannot be negative: " + weight); | |
| } | |
| this.weight = weight; | |
| return this; | |
| } | |
| public SpawnableEntityProfile weight(@Positive double weight) { | |
| if (weight <= 0) { | |
| throw new IllegalArgumentException("Weight cannot be negative: " + weight); | |
| } | |
| this.weight = weight; | |
| return this; | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/fr/moussax/blightedMC/core/entities/spawnable/SpawnableEntityProfile.java
around lines 28 to 34, the runtime check (weight < 0.00) conflicts with the
@Positive annotation because it allows zero; either change the annotation to
@PositiveOrZero if zero is valid, or enforce strict positivity by changing the
check to weight <= 0 and update the thrown message to "Weight must be positive"
or "Weight cannot be non-positive: " + weight to reflect the tighter validation.
| public class BonemerangAbility implements AbilityManager<PlayerInteractEvent> { | ||
| private static final NamespacedKey UUID_KEY = new NamespacedKey("blightedmc", "bonemerang_uuid"); | ||
| private static final Map<UUID, Map<String, Long>> cooldowns = new WeakHashMap<>(); | ||
| private static final Map<UUID, Map<String, Long>> cooldowns = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Memory leak: cooldown entries accumulate indefinitely.
Switching from WeakHashMap to ConcurrentHashMap removes automatic cleanup of player entries. On long-running servers, this map will accumulate player UUIDs and their associated item cooldown maps indefinitely, even after cooldowns expire and players disconnect.
Consider one of these solutions:
Option 1: Implement periodic cleanup
private static void cleanupExpiredCooldowns() {
long now = System.currentTimeMillis();
cooldowns.forEach((playerId, itemCooldowns) -> {
itemCooldowns.entrySet().removeIf(entry -> entry.getValue() <= now);
if (itemCooldowns.isEmpty()) {
cooldowns.remove(playerId);
}
});
}Schedule this cleanup task to run periodically (e.g., every 5 minutes).
Option 2: Use Caffeine or Guava Cache
private static final Cache<UUID, Map<String, Long>> cooldowns =
Caffeine.newBuilder()
.expireAfterAccess(Duration.ofHours(1))
.build();Option 3: Clean up on access
private boolean isItemOnCooldown(Player player, String uuid) {
Map<String, Long> playerCooldowns = cooldowns.computeIfAbsent(
player.getUniqueId(), k -> new ConcurrentHashMap<>());
long currentTime = System.currentTimeMillis();
// Remove expired entry
Long expiry = playerCooldowns.get(uuid);
if (expiry != null && expiry <= currentTime) {
playerCooldowns.remove(uuid);
return false;
}
return expiry != null;
}🤖 Prompt for AI Agents
In src/main/java/fr/moussax/blightedMC/gameplay/abilities/BonemerangAbility.java
around line 21, the static ConcurrentHashMap storing per-player cooldown maps
never removes player entries, causing unbounded memory growth; fix by removing
expired cooldowns and player maps when empty: implement cleanup-on-access inside
the methods that check/set cooldowns (remove individual item entries when
expired and if the per-player map becomes empty remove the player UUID from the
outer map) and additionally schedule a periodic cleanup task (e.g., run every
few minutes) that iterates the outer map to prune expired item entries and
remove empty per-player maps; alternatively consider replacing the map with a
time-aware cache (Caffeine/Guava) if preferred.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Changes