GH-200 Add option to exclude Creative mode players from combat #200
Conversation
CitralFlo
added
🐛 bug
CitralFlo
changed the title
...lcombat-plugin/src/main/java/com/eternalcode/combat/fight/controller/FightTagController.java
Outdated
Show resolved
Hide resolved
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe recent changes involve a comprehensive update to the Eternal Combat plugin, focusing on enhancing the structure and functionality of various components. Key modifications include updates to the GitHub Actions workflow, which now allows pull requests from any branch, broadening the workflow's applicability. In the codebase, several classes have been refactored into interfaces, such as New services like The notification system has also been overhauled, enhancing how messages are sent to players, with a shift towards a builder pattern for clarity. Overall, these changes reflect a significant restructuring aimed at improving the maintainability and extensibility of the plugin's codebase while ensuring existing functionalities remain intact. 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 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (33)
eternalcombat-api/src/main/java/com/eternalcode/combat/fight/drop/DropService.java (1)
3-8: Consider adding Javadoc comments
Adding brief documentation for each method would help other developers understand how to implement this interface.
public interface DropService {
+ /**
+ * Modifies a drop based on the specified drop type.
+ * @param dropType the type of drop
+ * @param drop the drop to modify
+ * @return the result of the drop modification
+ */
DropResult modify(DropType dropType, Drop drop);
+ /**
+ * Registers a new drop modifier.
+ * @param dropModifier the modifier to register
+ */
void registerModifier(DropModifier dropModifier);
}eternalcombat-api/src/main/java/com/eternalcode/combat/fight/pearl/FightPearlService.java (1)
7-16: Would be helpful to add some documentation.
Consider adding JavaDoc comments to explain:
- What this service does
- What happens when delays don't exist
- Any rules about the UUID parameter
Here's a simple example:
/**
* Manages combat-related pearl delays for players.
*/
public interface FightPearlService {
/**
* Gets the delay timestamp for a player.
* @param uuid The player's UUID
* @return The delay instant, or null if no delay exists
*/
Instant getDelay(UUID uuid);eternalcombat-api/src/main/java/com/eternalcode/combat/fight/drop/DropKeepInventoryService.java (2)
7-16: Add JavaDoc comments to help other developers.
The interface looks good! Consider adding simple documentation to explain what each method does.
public interface DropKeepInventoryService {
+ /**
+ * Gets the next set of items for the specified player
+ * @param uuid Player's UUID
+ * @return List of items
+ */
List<ItemStack> nextItems(UUID uuid);
+ /**
+ * Checks if the player has any stored items
+ * @param uuid Player's UUID
+ * @return true if items exist
+ */
boolean hasItems(UUID uuid);
+ /**
+ * Stores multiple items for the player
+ * @param uuid Player's UUID
+ * @param items List of items to store
+ */
void addItems(UUID uuid, List<ItemStack> item);
+ /**
+ * Stores a single item for the player
+ * @param uuid Player's UUID
+ * @param item Item to store
+ */
void addItem(UUID uuid, ItemStack item);13-13: Fix parameter name to match its purpose.
The parameter name should be 'items' since it's a list.
- void addItems(UUID uuid, List<ItemStack> item);
+ void addItems(UUID uuid, List<ItemStack> items);eternalcombat-api/src/main/java/com/eternalcode/combat/fight/FightTag.java (2)
9-21: Nice and clean interface design!
The interface looks good, but adding some quick JavaDoc comments would help other developers understand how to use it.
public interface FightTag {
+ /**
+ * Gets the UUID of the player who initiated the combat tag.
+ * @return the tagger's UUID, or null if there is no tagger
+ */
@Nullable
@ApiStatus.Experimental
UUID getTagger();19-21: Consider adding @NotNull annotations
Since getTaggedPlayer() and getEndOfCombatLog() always return values, marking them with @NotNull would help prevent null-related bugs.
+ @NotNull
Instant getEndOfCombatLog();
+ @NotNull
UUID getTaggedPlayer();buildSrc/src/main/kotlin/eternalcombat-java.gradle.kts (1)
Java version mismatch needs attention
The codebase shows different Java versions being used:
- Java 21 for toolchain
- Java 17 for source and target compatibility
This setup needs to be aligned to avoid potential issues. Please update either:
- Set toolchain to Java 17 to match compatibility settings, or
- Update compatibility settings to Java 21 to match toolchain
🔗 Analysis chain
Line range hint 39-41: Check Java version configuration
There's a mismatch between Java versions:
- Compilation targets Java 17 (line 31)
- Toolchain uses Java 21 (line 40)
This could cause confusion. Consider aligning these versions or documenting why they differ.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other Java version references in the project
rg "VERSION_\d+" --type kotlin
rg "JavaVersion\." --type kotlin
Length of output: 483
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/tagout/FightTagOutServiceImpl.java (1)
21-23: Consider returning removal status
The method could return a boolean to indicate whether the player was actually tagged out.
- public void unTagOut(UUID player) {
- this.tagOuts.remove(player);
+ public boolean unTagOut(UUID player) {
+ return this.tagOuts.remove(player) != null;
}eternalcombat-plugin/src/main/java/com/eternalcode/combat/updater/UpdaterService.java (1)
Line range hint 19-24: Consider adding debug logging.
It would be helpful to log the version comparison results for easier troubleshooting.
public UpdaterService(PluginDescriptionFile descriptionFile) {
this.gitCheckResult = new Lazy<>(() -> {
String version = descriptionFile.getVersion();
+ // Add logging for version check
+ plugin.getLogger().info("Checking version: " + version);
return this.gitCheck.checkRelease(GIT_REPOSITORY, GitTag.of("v" + version));
});
}eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/drop/DropKeepInventoryServiceImpl.java (1)
Line range hint 31-41: LGTM! Good defensive programming.
The method handles edge cases well and prevents list modification. Consider adding a brief comment explaining that this is a one-time retrieval that removes the items.
+ /**
+ * Retrieves and removes all items for the given UUID.
+ * Once retrieved, the items are no longer available for future calls.
+ */
@Override
public List<ItemStack> nextItems(UUID uuid) {eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/drop/DropServiceImpl.java (2)
Line range hint 14-27: Good error handling, but let's make it even better!
The null and type checks are helpful, but we could make the error messages more user-friendly.
Here's a simpler way to phrase the error messages:
- throw new RuntimeException("Drop type cannot be null! '%s'".formatted(dropModifier.getClass().getSimpleName()));
+ throw new RuntimeException("Drop type is missing for modifier: %s".formatted(dropModifier.getClass().getSimpleName()));
- throw new RuntimeException("You cannot register DropModifier for this type '%s'".formatted(dropType.name()));
+ throw new RuntimeException("Cannot register modifier for UNCHANGED drop type");Line range hint 29-36: Consider adding null check for input parameters.
While the error handling for missing modifiers is good, we should also validate the input parameters.
Add these checks at the start of the method:
public DropResult modify(DropType dropType, Drop drop) {
+ if (dropType == null) {
+ throw new IllegalArgumentException("Drop type cannot be null");
+ }
+ if (drop == null) {
+ throw new IllegalArgumentException("Drop cannot be null");
+ }eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/pearl/FightPearlServiceImpl.java (1)
37-40: Add a comment explaining the Instant.MIN default
The code is good, but it would be helpful to explain why Instant.MIN is used as the default value.
+ /**
+ * Gets the delay for the specified UUID.
+ * Returns Instant.MIN if no delay is set, ensuring hasDelay returns false.
+ */
public Instant getDelay(UUID uuid) {
return this.pearlDelays.asMap().getOrDefault(uuid, Instant.MIN);
}eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/pearl/FightPearlSettings.java (1)
26-28: Consider adding placeholder support
While the message is simple now, adding placeholders would make it more flexible for future updates.
public Notice pearlThrowBlockedDuringCombat = BukkitNotice.builder()
- .chat("&cThrowing ender pearls is prohibited during combat!")
+ .chat("&cThrowing ender pearls is {ACTION} during combat!")
+ .placeholder("ACTION", "prohibited")
.build();eternalcombat-plugin/src/main/java/com/eternalcode/combat/handler/MissingPermissionHandlerImpl.java (2)
11-15: Consider adding class-level documentation
The class structure looks good! Adding a simple JavaDoc comment would help explain the purpose of this handler to other developers.
+/**
+ * Handles cases where a command sender lacks required permissions by sending them
+ * a configured notification message.
+ */
public class MissingPermissionHandlerImpl implements MissingPermissionsHandler<CommandSender> {21-36: Remove extra empty line and consider adding null check
The implementation looks good but has two small suggestions:
- Remove the extra empty line at line 29
- Consider adding a null check for the missing permissions
@Override
public void handle(
Invocation<CommandSender> invocation,
MissingPermissions missingPermissions,
ResultHandlerChain<CommandSender> resultHandlerChain
) {
+ if (missingPermissions == null || missingPermissions.isEmpty()) {
+ return;
+ }
String joinedText = missingPermissions.asJoinedText();
-
this.announcer.create()🧰 Tools
🪛 ast-grep
[warning] 26-26: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: String joinedText = missingPermissions.asJoinedText();
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/FightTagImpl.java (1)
11-13: Add field descriptions
Consider adding simple comments to explain what each field does. This will help other developers understand the code better.
+ /** The UUID of the player in combat */
private final UUID taggedPlayer;
+ /** When the combat tag expires */
private final Instant endOfCombatLog;
+ /** The UUID of the player who initiated combat (if any) */
private final @Nullable UUID tagger;eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/logout/LogoutController.java (1)
Line range hint 26-42: Add Creative mode check to align with PR objective
The PR aims to exclude Creative mode players from combat, but there's no check for the player's game mode before applying combat logout punishment.
Here's a simple fix:
@EventHandler(priority = EventPriority.HIGH)
void onQuit(PlayerQuitEvent event) {
Player player = event.getPlayer();
if (!this.fightManager.isInCombat(player.getUniqueId())) {
return;
}
+ if (player.getGameMode() == GameMode.CREATIVE) {
+ return;
+ }
+
this.logoutService.punishForLogout(player);
player.setHealth(0.0);
this.announcer.create()
.notice(this.config.messages.playerLoggedOutDuringCombat)
.placeholder("{PLAYER}", player.getName())
.all()
.send();
}eternalcombat-plugin/src/main/java/com/eternalcode/combat/EternalCombatReloadCommand.java (1)
16-18: Add a quick class description
A short comment explaining what this command does would help other developers.
Add this comment above the class:
+/**
+ * Command to reload the EternalCombat plugin configuration.
+ */
@Command(name = "combatlog", aliases = "combat")eternalcombat-plugin/src/main/java/com/eternalcode/combat/notification/NotificationAnnouncer.java (1)
15-20: Add class documentation
A simple class description would help others understand the purpose of this notification system.
+/**
+ * Handles the delivery of plugin notifications to players and console,
+ * using MiniMessage for text formatting.
+ */
public final class NotificationAnnouncer extends BukkitMultification<PluginConfig> {eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/controller/FightMessageController.java (1)
54-58: Consider removing the extra blank line
There's an unnecessary blank line at line 58.
this.announcer.create()
.player(player.getUniqueId())
.notice(this.config.messages.playerUntagged)
.send();
-
eternalcombat-api/src/main/java/com/eternalcode/combat/fight/event/FightTagEvent.java (1)
22-23: Consider adding constructor documentation
A simple JavaDoc comment would help explain why we're calling super(false).
+/**
+ * Creates a new FightTagEvent.
+ *
+ * @param player The UUID of the player being tagged
+ * @param cause The reason for the tag
+ */
public FightTagEvent(UUID player, CauseOfTag cause) {eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/pearl/FightPearlController.java (1)
68-82: Consider adding null check for remaining delay.
While the code works well, it might be good to add a quick null check when getting the remaining delay, just to be extra safe.
Here's a simple way to do it:
Duration remainingPearlDelay = this.fightPearlService.getRemainingDelay(uniqueId);
+if (remainingPearlDelay == null) {
+ return;
+}🧰 Tools
🪛 ast-grep
[warning] 70-70: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: Duration remainingPearlDelay = this.fightPearlService.getRemainingDelay(uniqueId);
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/controller/FightTagController.java (2)
58-59: Consider simplifying the condition check
The condition works but could be more readable. Here's a cleaner way to write it:
- if (attacker.isOp() && this.config.settings.excludeOpFromCombat ||
- attacker.getGameMode().equals(GameMode.CREATIVE) && this.config.settings.excludeCreativeFromCombat ) {
+ if ((attacker.isOp() && this.config.settings.excludeOpFromCombat) ||
+ (attacker.getGameMode() == GameMode.CREATIVE && this.config.settings.excludeCreativeFromCombat)) {63-64: Apply the same condition simplification here
Let's keep the style consistent with the previous change:
- if (attackedPlayerByPerson.isOp() && this.config.settings.excludeOpFromCombat ||
- attackedPlayerByPerson.getGameMode().equals(GameMode.CREATIVE) && this.config.settings.excludeCreativeFromCombat) {
+ if ((attackedPlayerByPerson.isOp() && this.config.settings.excludeOpFromCombat) ||
+ (attackedPlayerByPerson.getGameMode() == GameMode.CREATIVE && this.config.settings.excludeCreativeFromCombat)) {eternalcombat-api/src/main/java/com/eternalcode/combat/fight/effect/FightEffectService.java (1)
14-16: Consider using primitive types for amplifiers
Using int instead of Integer for the amplifier parameter might improve performance by avoiding unnecessary object creation.
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/effect/FightEffectServiceImpl.java (1)
47-47: Use primitive int for the amplifier
Changing Integer amplifier to int amplifier can prevent unnecessary boxing and null checks.
eternalcombat-plugin/src/main/java/com/eternalcode/combat/config/implementation/PluginConfig.java (6)
214-214: Clarify the blockPlacingBlockedDuringCombat message.
The message might be confusing. Consider rephrasing it for better understanding.
Here's a suggested change:
-public Notice blockPlacingBlockedDuringCombat = Notice.chat("&cYou cannot place {MODE} {Y} coordinate during combat!");
+public Notice blockPlacingBlockedDuringCombat = Notice.chat("&cYou cannot place blocks {MODE} Y coordinate {Y} during combat!");245-245: Improve clarity in adminTagOutSelf message.
To make it clearer, change "You will be taggable after {TIME}" to "You will be taggable again in {TIME}".
Suggested update:
-public Notice adminTagOutSelf = Notice.chat("&7Successfully disabled tag for Yourself! You will be taggable after &e{TIME} ");
+public Notice adminTagOutSelf = Notice.chat("&7Successfully disabled tag for yourself! You will be taggable again in &e{TIME}");248-248: Enhance the adminTagOut message.
Consider rephrasing "They will be taggable after {TIME}" to "They will be taggable again in {TIME}".
Apply this change:
-public Notice adminTagOut = Notice.chat("&7Successfully disabled tag for &e{PLAYER}! They will be taggable after &e{TIME} ");
+public Notice adminTagOut = Notice.chat("&7Successfully disabled tag for &e{PLAYER}! They will be taggable again in &e{TIME}");251-251: Clarify the playerTagOut message.
Rephrase "You will be taggable in {TIME}" to "You will be taggable again in {TIME}" for better understanding.
Here's the suggested fix:
-public Notice playerTagOut = Notice.chat("&7You will be taggable in &e{TIME} !");
+public Notice playerTagOut = Notice.chat("&7You will be taggable again in &e{TIME}!");254-254: Improve the adminTagOutOff message.
Changing "enabled tag for" to "re-enabled tagging for" makes it clearer.
Update it like this:
-public Notice adminTagOutOff = Notice.chat("&7Successfully enabled tag for &e{PLAYER}!");
+public Notice adminTagOutOff = Notice.chat("&7Successfully re-enabled tagging for &e{PLAYER}!");257-257: Enhance clarity in playerTagOutOff message.
Replace "You are now taggable!" with "You can now be tagged again!" to improve understanding.
Apply this update:
-public Notice playerTagOutOff = Notice.chat("&7You are now taggable!");
+public Notice playerTagOutOff = Notice.chat("&7You can now be tagged again!");🛑 Comments failed to post (25)
eternalcombat-api/src/main/java/com/eternalcode/combat/fight/event/CauseOfTag.java (1)
4-23: 🛠️ Refactor suggestion
Consider adding a CREATIVE_MODE constant
Since this PR aims to handle Creative mode players differently, it might be helpful to add a specific constant for Creative mode interactions.
public enum CauseOfTag { /** * The player was tagged in combat as a result of an attack by another player. */ PLAYER, /** * The player was tagged in combat due to an interaction with a non-player entity * (e.g., mobs or environmental damage). */ NON_PLAYER, /** * A command was executed to apply a combat tag to the player. */ COMMAND, /** * A custom cause, typically defined by external plugins or systems, applied the combat tag. */ - CUSTOM + CUSTOM, + + /** + * The player was excluded from combat due to being in Creative mode. + */ + CREATIVE_MODE }📝 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./** * The player was tagged in combat as a result of an attack by another player. */ PLAYER, /** * The player was tagged in combat due to an interaction with a non-player entity * (e.g., mobs or environmental damage). */ NON_PLAYER, /** * A command was executed to apply a combat tag to the player. */ COMMAND, /** * A custom cause, typically defined by external plugins or systems, applied the combat tag. */ CUSTOM, /** * The player was excluded from combat due to being in Creative mode. */ CREATIVE_MODEeternalcombat-api/src/main/java/com/eternalcode/combat/fight/event/CauseOfUnTag.java (1)
34-36: 🛠️ Refactor suggestion
Consider adding a specific constant for Creative mode
Since this PR aims to handle Creative mode players, it might be helpful to add a specific constant like
CREATIVE_MODEinstead of relying onCUSTOM./** * A custom cause, typically defined by external plugins or systems. */ - CUSTOM + CUSTOM, + + /** + * The player entered Creative mode, which removes the combat tag. + */ + CREATIVE_MODE📝 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./** * A custom cause, typically defined by external plugins or systems. */ CUSTOM, /** * The player entered Creative mode, which removes the combat tag. */ CREATIVE_MODEeternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/tagout/FightTagOutServiceImpl.java (3)
11-11: 🛠️ Refactor suggestion
Consider using ConcurrentHashMap for thread safety
Since combat systems often handle multiple players simultaneously, using
ConcurrentHashMapinstead ofHashMapwould make this safer.- private final Map<UUID, Instant> tagOuts = new HashMap<>(); + private final Map<UUID, Instant> tagOuts = new ConcurrentHashMap<>();📝 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.private final Map<UUID, Instant> tagOuts = new ConcurrentHashMap<>();
14-18:
⚠️ Potential issueAdd input validation
The method should check if the parameters are valid before processing.
public void tagOut(UUID player, Duration duration) { + if (player == null || duration == null) { + throw new IllegalArgumentException("Player and duration cannot be null"); + } + if (duration.isNegative()) { + throw new IllegalArgumentException("Duration cannot be negative"); + } Instant endTime = Instant.now().plus(duration); this.tagOuts.put(player, endTime); }📝 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 void tagOut(UUID player, Duration duration) { if (player == null || duration == null) { throw new IllegalArgumentException("Player and duration cannot be null"); } if (duration.isNegative()) { throw new IllegalArgumentException("Duration cannot be negative"); } Instant endTime = Instant.now().plus(duration); this.tagOuts.put(player, endTime); }
26-35: 🛠️ Refactor suggestion
Add null check and consider caching current time
The method needs input validation, and if this is called frequently, caching the current time might help performance.
public boolean isTaggedOut(UUID player) { + if (player == null) { + throw new IllegalArgumentException("Player cannot be null"); + } Instant endTime = this.tagOuts.get(player); if (endTime == null) { return false; } Instant now = Instant.now(); return now.isBefore(endTime); }📝 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 boolean isTaggedOut(UUID player) { if (player == null) { throw new IllegalArgumentException("Player cannot be null"); } Instant endTime = this.tagOuts.get(player); if (endTime == null) { return false; } Instant now = Instant.now(); return now.isBefore(endTime); }eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/drop/DropKeepInventoryServiceImpl.java (1)
21-24: 🛠️ Refactor suggestion
Consider optimizing for bulk operations.
The current implementation creates a new list for each item. For better performance with large lists, consider adding all items at once.
@Override public void addItems(UUID uuid, List<ItemStack> item) { - item.forEach(i -> this.addItem(uuid, i)); + this.itemsToGiveAfterRespawn.computeIfAbsent(uuid, k -> new ArrayList<>()).addAll(item); }Committable suggestion was skipped due to low confidence.
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/pearl/FightPearlServiceImpl.java (2)
32-35: 🛠️ Refactor suggestion
Consider reusing the current time calculation
Similar to the previous method, we could cache the current time:
- public Duration getRemainingDelay(UUID uuid) { - return Duration.between(Instant.now(), this.getDelay(uuid)); + public Duration getRemainingDelay(UUID uuid) { + Instant now = Instant.now(); + return Duration.between(now, this.getDelay(uuid)); }📝 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.@Override public Duration getRemainingDelay(UUID uuid) { Instant now = Instant.now(); return Duration.between(now, this.getDelay(uuid)); }
27-30: 🛠️ Refactor suggestion
Consider caching the current time
We could make this slightly more efficient by storing the current time once:
- public boolean hasDelay(UUID uuid) { - return Instant.now().isBefore(this.getDelay(uuid)); + public boolean hasDelay(UUID uuid) { + Instant now = Instant.now(); + return now.isBefore(this.getDelay(uuid)); }📝 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.@Override public boolean hasDelay(UUID uuid) { Instant now = Instant.now(); return now.isBefore(this.getDelay(uuid)); }eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/FightTagImpl.java (1)
15-19: 🛠️ Refactor suggestion
Improve constructor clarity and safety
Two small suggestions to make this better:
- Rename parameter to match field name
- Add null checks for important values
- FightTagImpl(UUID personToAddCombat, Instant endOfCombatLog, @Nullable UUID tagger) { + FightTagImpl(UUID taggedPlayer, Instant endOfCombatLog, @Nullable UUID tagger) { + if (taggedPlayer == null || endOfCombatLog == null) { + throw new IllegalArgumentException("Tagged player and combat log end time cannot be null"); + } - this.taggedPlayer = personToAddCombat; + this.taggedPlayer = taggedPlayer; this.endOfCombatLog = endOfCombatLog; this.tagger = tagger; }📝 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.FightTagImpl(UUID taggedPlayer, Instant endOfCombatLog, @Nullable UUID tagger) { if (taggedPlayer == null || endOfCombatLog == null) { throw new IllegalArgumentException("Tagged player and combat log end time cannot be null"); } this.taggedPlayer = taggedPlayer; this.endOfCombatLog = endOfCombatLog; this.tagger = tagger; }eternalcombat-plugin/src/main/java/com/eternalcode/combat/handler/InvalidUsageHandlerImpl.java (2)
17-20: 🛠️ Refactor suggestion
Consider adding null checks to prevent issues.
A quick null check would make this more robust:
public InvalidUsageHandlerImpl(PluginConfig config, NotificationAnnouncer announcer) { + if (config == null || announcer == null) { + throw new IllegalArgumentException("Config and announcer must not be null"); + } this.config = config; this.announcer = announcer; }📝 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 InvalidUsageHandlerImpl(PluginConfig config, NotificationAnnouncer announcer) { if (config == null || announcer == null) { throw new IllegalArgumentException("Config and announcer must not be null"); } this.config = config; this.announcer = announcer; }
22-38: 🛠️ Refactor suggestion
Small suggestions to make the code more robust.
Two quick improvements would help:
- Check if there are any usage strings to show
- Use a more descriptive name than
usage@Override public void handle( Invocation<CommandSender> invocation, InvalidUsage<CommandSender> commandSenderInvalidUsage, ResultHandlerChain<CommandSender> resultHandlerChain ) { Schematic schematic = commandSenderInvalidUsage.getSchematic(); + if (schematic.all().isEmpty()) { + return; + } - for (String usage : schematic.all()) { + for (String commandUsage : schematic.all()) { this.announcer.create() .viewer(invocation.sender()) .notice(this.config.messages.invalidCommandUsage) - .placeholder("{USAGE}", usage) + .placeholder("{USAGE}", commandUsage) .send(); } }📝 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.@Override public void handle( Invocation<CommandSender> invocation, InvalidUsage<CommandSender> commandSenderInvalidUsage, ResultHandlerChain<CommandSender> resultHandlerChain ) { Schematic schematic = commandSenderInvalidUsage.getSchematic(); if (schematic.all().isEmpty()) { return; } for (String commandUsage : schematic.all()) { this.announcer.create() .viewer(invocation.sender()) .notice(this.config.messages.invalidCommandUsage) .placeholder("{USAGE}", commandUsage) .send(); } }eternalcombat-plugin/src/main/java/com/eternalcode/combat/config/ConfigService.java (1)
25-31: 🛠️ Refactor suggestion
Consider extracting configuration setup
The configuration steps could be more readable in their own method.
Here's a simple way to clean this up:
public <T extends OkaeriConfig> T create(Class<T> config, File file) { T configFile = ConfigManager.create(config); - YamlBukkitConfigurer yamlBukkitConfigurer = new YamlBukkitConfigurer(); - NoticeResolverRegistry noticeRegistry = NoticeResolverDefaults.createRegistry() - .registerResolver(new SoundBukkitResolver()); - - configFile.withConfigurer(yamlBukkitConfigurer, new SerdesCommons(), new SerdesBukkit()); - configFile.withConfigurer(yamlBukkitConfigurer, new MultificationSerdesPack(noticeRegistry)); + setupConfiguration(configFile); configFile.withSerdesPack(registry -> registry.register(new NotificationSerializer())); configFile.withBindFile(file); configFile.withRemoveOrphans(true); configFile.saveDefaults(); configFile.load(true); this.configs.add(configFile); return configFile; } +private <T extends OkaeriConfig> void setupConfiguration(T configFile) { + YamlBukkitConfigurer yamlBukkitConfigurer = new YamlBukkitConfigurer(); + NoticeResolverRegistry noticeRegistry = NoticeResolverDefaults.createRegistry() + .registerResolver(new SoundBukkitResolver()); + + configFile.withConfigurer(yamlBukkitConfigurer, new SerdesCommons(), new SerdesBukkit()); + configFile.withConfigurer(yamlBukkitConfigurer, new MultificationSerdesPack(noticeRegistry)); +}📝 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 <T extends OkaeriConfig> T create(Class<T> config, File file) { T configFile = ConfigManager.create(config); setupConfiguration(configFile); configFile.withSerdesPack(registry -> registry.register(new NotificationSerializer())); configFile.withBindFile(file); configFile.withRemoveOrphans(true); configFile.saveDefaults(); configFile.load(true); this.configs.add(configFile); return configFile; } private <T extends OkaeriConfig> void setupConfiguration(T configFile) { YamlBukkitConfigurer yamlBukkitConfigurer = new YamlBukkitConfigurer(); NoticeResolverRegistry noticeRegistry = NoticeResolverDefaults.createRegistry() .registerResolver(new SoundBukkitResolver()); configFile.withConfigurer(yamlBukkitConfigurer, new SerdesCommons(), new SerdesBukkit()); configFile.withConfigurer(yamlBukkitConfigurer, new MultificationSerdesPack(noticeRegistry)); }🧰 Tools
🪛 ast-grep
[warning] 26-26: Detected a cookie where the
HttpOnlyflag is either missing or disabled. TheHttpOnlycookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set theHttpOnlyflag to true` in all other cases.
Context: (new SoundBukkitResolver())
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
[warning] 28-28: Detected a cookie where the
HttpOnlyflag is either missing or disabled. TheHttpOnlycookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set theHttpOnlyflag to true` in all other cases.
Context: (yamlBukkitConfigurer, new SerdesCommons(), new SerdesBukkit())
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
[warning] 29-29: Detected a cookie where the
HttpOnlyflag is either missing or disabled. TheHttpOnlycookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set theHttpOnlyflag to true` in all other cases.
Context: (yamlBukkitConfigurer, new MultificationSerdesPack(noticeRegistry))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
[warning] 30-30: Detected a cookie where the
HttpOnlyflag is either missing or disabled. TheHttpOnlycookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set theHttpOnlyflag to true` in all other cases.
Context: (new NotificationSerializer())
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
[warning] 26-26: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (new SoundBukkitResolver())
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
[warning] 28-28: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (yamlBukkitConfigurer, new SerdesCommons(), new SerdesBukkit())
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
[warning] 29-29: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (yamlBukkitConfigurer, new MultificationSerdesPack(noticeRegistry))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
[warning] 30-30: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (new NotificationSerializer())
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfigurationeternalcombat-plugin/src/main/java/com/eternalcode/combat/EternalCombatReloadCommand.java (2)
32-44: 🛠️ Refactor suggestion
Add error handling for reload operation
The reload might fail, and we should let the user know if it does.
Here's a simple way to handle errors:
@Execute(name = "reload") void execute(@Context CommandSender sender) { Stopwatch stopwatch = Stopwatch.createStarted(); - this.configService.reload(); + try { + this.configService.reload(); + } catch (Exception e) { + this.announcer.create() + .viewer(sender) + .error("Failed to reload: " + e.getMessage()) + .send(); + return; + }📝 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.@Async @Execute(name = "reload") void execute(@Context CommandSender sender) { Stopwatch stopwatch = Stopwatch.createStarted(); try { this.configService.reload(); } catch (Exception e) { this.announcer.create() .viewer(sender) .error("Failed to reload: " + e.getMessage()) .send(); return; } Duration elapsed = stopwatch.elapsed(); this.announcer.create() .viewer(sender) .notice(RELOAD_MESSAGE) .placeholder("{TIME}", String.valueOf(elapsed.toMillis())) .send();🧰 Tools
🪛 ast-grep
[warning] 37-37: Detected a cookie where the
HttpOnlyflag is either missing or disabled. TheHttpOnlycookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set theHttpOnlyflag to true` in all other cases.
Context: Duration elapsed = stopwatch.elapsed();
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
20-22: 💡 Codebase verification
Message style is inconsistent with other plugin messages
The reload message uses fancy gradient and color formatting, while all other messages in the plugin use simple color codes (like &c, &7, &e). Let's stick to the same style as other messages for consistency.
- Change
.chat("<b><gradient:#8a1212:#fc6b03>EternalCombat:</gradient></b> Reloaded EternalCombat in <color:#fce303>{TIME}ms!</color>")to use standard color codes like.chat("&7[&cEternalCombat&7] &aReloaded in &e{TIME}ms!")🔗 Analysis chain
Check message style consistency
Let's make sure this message matches the style of other plugin messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find other message definitions to compare styles rg -l "BukkitNotice.builder()" | xargs rg "<gradient|<color"Length of output: 287
Script:
#!/bin/bash # Let's search for any other message patterns in the codebase rg "Notice.*build\(\)" -B2 -A1Length of output: 32
Script:
#!/bin/bash # Let's try a different approach to find message patterns rg -A2 "chat\("Length of output: 12777
eternalcombat-plugin/src/main/java/com/eternalcode/combat/notification/NotificationAnnouncer.java (1)
38-47: 🛠️ Refactor suggestion
Consider simplifying the audience converter
The code works well but could be more concise.
protected @NotNull AudienceConverter<CommandSender> audienceConverter() { - return commandSender -> { - if (commandSender instanceof Player player) { - return this.audienceProvider.player(player.getUniqueId()); - } - - return this.audienceProvider.console(); - }; + return sender -> sender instanceof Player player + ? this.audienceProvider.player(player.getUniqueId()) + : this.audienceProvider.console(); }📝 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.protected @NotNull AudienceConverter<CommandSender> audienceConverter() { return sender -> sender instanceof Player player ? this.audienceProvider.player(player.getUniqueId()) : this.audienceProvider.console(); }eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/controller/FightActionBlockerController.java (1)
56-62: 🛠️ Refactor suggestion
Let's clean up the duplicate announcement code
The same announcement code appears twice. We can make this simpler by moving it before the return.
Here's a cleaner way to write it:
if (isPlacementBlocked && specificBlocksToPreventPlacing.isEmpty()) { event.setCancelled(true); - this.announcer.create() - .player(uniqueId) - .notice(this.config.messages.blockPlacingBlockedDuringCombat) - .placeholder("{Y}", String.valueOf(this.config.settings.blockPlacingYCoordinate)) - .placeholder("{MODE}", this.config.settings.blockPlacingModeName) - .send(); + sendBlockPlacementMessage(uniqueId); return; } Material blockMaterial = block.getType(); boolean isBlockInDisabledList = specificBlocksToPreventPlacing.contains(blockMaterial); if (isPlacementBlocked && isBlockInDisabledList) { event.setCancelled(true); - this.announcer.create() - .player(uniqueId) - .notice(this.config.messages.blockPlacingBlockedDuringCombat) - .placeholder("{Y}", String.valueOf(this.config.settings.blockPlacingYCoordinate)) - .placeholder("{MODE}", this.config.settings.blockPlacingModeName) - .send(); + sendBlockPlacementMessage(uniqueId); } +private void sendBlockPlacementMessage(UUID uniqueId) { + this.announcer.create() + .player(uniqueId) + .notice(this.config.messages.blockPlacingBlockedDuringCombat) + .placeholder("{Y}", String.valueOf(this.config.settings.blockPlacingYCoordinate)) + .placeholder("{MODE}", this.config.settings.blockPlacingModeName) + .send(); +}Also applies to: 70-76
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/FightManagerImpl.java (3)
78-80:
⚠️ Potential issueHandle expired fights in
getTagmethodWhen retrieving a fight tag, if it's expired, consider removing it from the
fightsmap and returningnullto indicate the player is no longer in combat.Here's how you might adjust the method:
public FightTag getTag(UUID target) { FightTag fightTag = this.fights.get(target); + if (fightTag != null && fightTag.isExpired()) { + this.fights.remove(target); + return null; + } return fightTag; }📝 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 FightTag getTag(UUID target) { FightTag fightTag = this.fights.get(target); if (fightTag != null && fightTag.isExpired()) { this.fights.remove(target); return null; } return fightTag; }
73-75:
⚠️ Potential issueReturn only active fights in
getFightsCurrently,
getFightsincludes expired fights. Filtering out expired fights will provide a more accurate list of active combats.You can modify the method like this:
+import java.util.stream.Collectors; public Collection<FightTag> getFights() { return Collections.unmodifiableCollection( this.fights.values().stream() .filter(fightTag -> !fightTag.isExpired()) .collect(Collectors.toList()) ); }📝 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 Collection<FightTag> getFights() { return Collections.unmodifiableCollection( this.fights.values().stream() .filter(fightTag -> !fightTag.isExpired()) .collect(Collectors.toList()) ); }
29-37:
⚠️ Potential issueConsider removing expired fights to optimize memory usage
In the
isInCombatmethod, expired fights remain in thefightsmap, which might lead to unnecessary memory consumption over time. Removing expired entries when they are detected can help keep the application efficient.Here's a suggested change:
public boolean isInCombat(UUID player) { FightTag fightTag = this.fights.get(player); if (fightTag == null || fightTag.isExpired()) { + this.fights.remove(player); return false; } return true; }📝 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 boolean isInCombat(UUID player) { FightTag fightTag = this.fights.get(player); if (fightTag == null || fightTag.isExpired()) { this.fights.remove(player); return false; } return true; }eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/tagout/FightTagOutCommand.java (2)
47-66: 🛠️ Refactor suggestion
Consider separate permission for tagging others
It might be good to require a different permission when tagging out other players to enhance access control.
68-88: 🛠️ Refactor suggestion
Consider separate permission for untagging others
Similar to tagging, untagging other players may benefit from a distinct permission to ensure proper authorization.
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/FightTagCommand.java (3)
54-66: 🛠️ Refactor suggestion
Add check for Creative mode players
Ensure that players in Creative mode are not tagged into combat.
Consider adding a check to exclude Creative mode players:
void tag(@Context CommandSender sender, @Arg Player target) { UUID targetUniqueId = target.getUniqueId(); Duration time = this.config.settings.combatDuration; + if (target.getGameMode() == GameMode.CREATIVE) { + this.announcer.create() + .notice("Cannot tag a player in Creative mode.") + .viewer(sender) + .send(); + return; + } FightTagEvent event = this.fightManager.tag(targetUniqueId, time, CauseOfTag.COMMAND); // ... }Committable suggestion was skipped due to low confidence.
126-148: 🛠️ Refactor suggestion
Use CommandSender instead of Player
Consider using
CommandSenderas the sender parameter in theuntagmethod for consistency.Update the method signature:
- void untag(@Context Player sender, @Arg Player target) { + void untag(@Context CommandSender sender, @Arg Player target) {📝 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.void untag(@Context CommandSender sender, @Arg Player target) { UUID targetUniqueId = target.getUniqueId(); if (!this.fightManager.isInCombat(targetUniqueId)) { this.announcer.create() .viewer(sender) .notice(this.config.messages.admin.adminPlayerNotInCombat) .send(); return; } FightUntagEvent event = this.fightManager.untag(targetUniqueId, CauseOfUnTag.COMMAND); if (event.isCancelled()) { return; } this.announcer.create() .notice(this.config.messages.admin.adminUntagPlayer) .placeholder("{PLAYER}", target.getName()) .viewer(sender) .send(); }
76-122: 🛠️ Refactor suggestion
Exclude Creative mode players when tagging multiple players
Ensure that players in Creative mode are not tagged when using the
tagMultiplecommand.Consider adding checks for each target player:
void tagMultiple(@Context CommandSender sender, @Arg Player firstTarget, @Arg Player secondTarget) { Duration combatTime = this.config.settings.combatDuration; PluginConfig.Messages messages = this.config.messages; + if (firstTarget.getGameMode() == GameMode.CREATIVE || secondTarget.getGameMode() == GameMode.CREATIVE) { + this.announcer.create() + .notice("Cannot tag players in Creative mode.") + .viewer(sender) + .send(); + return; + } if (sender.equals(firstTarget) || sender.equals(secondTarget)) { // ...Committable suggestion was skipped due to low confidence.
eternalcombat-plugin/src/main/java/com/eternalcode/combat/config/implementation/PluginConfig.java (1)
75-75:
⚠️ Potential issueFix typo in comment for
excludeCreativeFromCombat.There's a typo in the comment on line 75. It should be "can tag others" instead of "can be tag others".
Apply this fix:
-# Setting this to false - players in creative mode can be tag others +# Setting this to false - players in creative mode can tag others📝 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."# Setting this to false - players in creative mode can tag others"
Additionally refactor config variable name