Hub: Code quality, enhancments and better maintainability #23
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request implements changes to the Zander Hub plugin, enhancing the player join experience and overall configuration. Modifications include the addition of new repositories and dependencies in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (5)
30-34: Consider making constants configurable.
Currently, the constants (TEST_ALWAYS_FIRST_JOIN,NAV_COMPASS_SLOT, etc.) may be better placed in a config file if you anticipate needing to change these values without recompiling.
43-46: Keep pitch or volume dynamic for variation.
If you wish to add a bit of variety, randomizing pitch and volume can produce a more immersive experience. This is just a minor enhancement idea.
47-91: Potential memory optimization.
Storing a large array of sounds is generally fine, but if the list grows more, consider referencing smaller curated subsets or external config. This can help keep the plugin footprint more manageable and flexible.
153-187: Firework logic is well-structured; watch out for performance.
Repeatedly spawning and detonating fireworks is fine, but keep in mind that too many fireworks in short intervals can lag the server. The random color palette is a nice touch.
189-196: Consider placeholders or translations for the welcome message.
For better localization or dynamic placeholders, you might integrate a library (like MiniMessage or a custom translator) so certain parts of the message can be adapted more easily or support multiple languages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
zander-hub/pom.xml(0 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java(1 hunks)zander-hub/src/main/resources/plugin.yml(1 hunks)
💤 Files with no reviewable changes (1)
- zander-hub/pom.xml
✅ Files skipped from review due to trivial changes (1)
- zander-hub/src/main/resources/plugin.yml
🔇 Additional comments (9)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (9)
35-42: Validate negative or zero bounds for FIREWORK_GROUND_HEIGHT.
If a negative or zero height is unintentionally set, the firework might spawn underground or inside a block, causing weird behavior or immediate detonation. Consider catching invalid values in the configuration.
98-105: Ensure consistent permission checks for flight.
The onPlayerLogin event sets flight for players with zander.hub.fly. Verify that no other constraints or meltdown states are needed (e.g., creative mode checks).
106-122: Suppressing default join message is valid.
Your approach (event.joinMessage(null)) effectively hides the system join notification. Just ensure your custom join messaging logic handles all relevant cases (e.g. vanish states).
124-129: Good approach to separate permission logic.
Centralizing the permission handling in setPermissions() method keeps the code maintainable. No issues found.
131-138: Consider null-checking the hub location.
If the configuration or location is missing or invalid, player.teleport(...) can fail. Add a small safeguard to ensure ConfigurationManager.getHubLocation() returns a valid location.
140-151: Team collision rule is properly set.
Using Team.Option.COLLISION_RULE is the recommended approach. Be sure to handle scoreboard conflicts if multiple plugins share the main scoreboard.
198-206: Broadcaster pattern for join message is clear.
Bukkit.getOnlinePlayers().forEach(...) is straightforward. If performance becomes a concern with large player counts, you might switch to asynchronous broadcasting.
209-213: Randomizing welcome sound is a nice touch.
This ensures that each join feels slightly different. Just watch for potentially loud or disruptive sounds if they are triggered frequently.
215-215: isVanished method logic.
This logic looks correct, as it checks metadata for “vanished.” Ensure the vanish plugin sets the metadata reliably on all use cases.
Also applies to: 217-218
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (2)
30-34: Consider moving TEST_ALWAYS_FIRST_JOIN to configurationThe TEST_ALWAYS_FIRST_JOIN flag should ideally be moved to the ConfigurationManager for easier testing environment management.
- private static final boolean TEST_ALWAYS_FIRST_JOIN = false; // * default falseAdd this to your configuration file instead:
debug: always_first_join: false
154-187: Simplify firework generation logicThe firework generation code could be made more concise and maintainable.
private void spawnWelcomeFirework(Player player) { Location spawnLoc = player.getLocation().add(0, FIREWORK_GROUND_HEIGHT, 0); Firework firework = player.getWorld().spawn(spawnLoc, Firework.class); FireworkMeta meta = firework.getFireworkMeta(); - - Random random = new Random(); - Color[] FCP = FIREWORK_COLOR_PALETTE; - - // random primary-colors - List<Color> colors = new ArrayList<>(); - int numColors = random.nextInt(3) + 2; // 2-4 colors - for (int i = 0; i < numColors; i++) { - colors.add(FCP[random.nextInt(FCP.length)]); - } - // random fade-color and firework type - Color fadeColor = FCP[random.nextInt(FCP.length)]; - FireworkEffect.Type[] types = FireworkEffect.Type.values(); - FireworkEffect.Type type = types[random.nextInt(types.length)]; + Random random = new Random(); + List<Color> colors = random.ints(random.nextInt(3) + 2, 0, FIREWORK_COLOR_PALETTE.length) + .mapToObj(i -> FIREWORK_COLOR_PALETTE[i]) + .collect(Collectors.toList()); + + Color fadeColor = FIREWORK_COLOR_PALETTE[random.nextInt(FIREWORK_COLOR_PALETTE.length)]; + FireworkEffect.Type type = FireworkEffect.Type.values()[random.nextInt(FireworkEffect.Type.values().length)]; FireworkEffect effect = FireworkEffect.builder()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java(1 hunks)
🔇 Additional comments (4)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (4)
98-122: Well-structured event handling with clear separation of concerns!
Good practices observed:
- Login event for permissions and validation
- Join event for world interactions
- Delayed execution for welcome actions
- Clear suppression of default join message
Line range hint 1-220: Overall excellent improvements to code quality and maintainability!
The changes successfully achieve the PR objectives with:
- Clear separation of concerns in event handling
- Well-structured constants for easy configuration
- Improved player experience with better effects
- Good documentation and method organization
The code is now more maintainable and easier to test.
131-138: Verify teleport success and handle inventory errors
The setInitialState method should verify teleport success and handle potential inventory errors.
140-151: Consider handling team creation failure
The team creation code should handle potential exceptions when creating or modifying the scoreboard team.
private void setupNoCollision(Player player) {
Scoreboard scoreboard = Bukkit.getScoreboardManager().getMainScoreboard();
- Team team = scoreboard.getTeam("nocollision");
- if (team == null) {
- team = scoreboard.registerNewTeam("nocollision");
- team.setOption(Team.Option.COLLISION_RULE, Team.OptionStatus.NEVER);
+ Team team;
+ try {
+ team = scoreboard.getTeam("nocollision");
+ if (team == null) {
+ team = scoreboard.registerNewTeam("nocollision");
+ team.setOption(Team.Option.COLLISION_RULE, Team.OptionStatus.NEVER);
+ }
+ team.addEntry(player.getName());
+ } catch (IllegalArgumentException e) {
+ plugin.getLogger().warning("Failed to setup no-collision for player: " + player.getName());
}
- team.addEntry(player.getName());
}| /// Check if the player is currently vanished. | ||
| private boolean isVanished(Player player) { | ||
| for (MetadataValue meta : player.getMetadata("vanished")) { | ||
| if (meta.asBoolean()) return true; | ||
| } | ||
| return false; | ||
| return player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean); | ||
|
|
There was a problem hiding this comment.
Add null check in isVanished method
The stream operation on metadata could throw NPE if the "vanished" metadata is not set.
private boolean isVanished(Player player) {
- return player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean);
+ return player.hasMetadata("vanished") &&
+ player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean);
}📝 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.
| /// Check if the player is currently vanished. | |
| private boolean isVanished(Player player) { | |
| for (MetadataValue meta : player.getMetadata("vanished")) { | |
| if (meta.asBoolean()) return true; | |
| } | |
| return false; | |
| return player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean); | |
| /// Check if the player is currently vanished. | |
| private boolean isVanished(Player player) { | |
| return player.hasMetadata("vanished") && | |
| player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean); |
There was a problem hiding this comment.
@palsmo you'll need a null check as stated.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…e message (replacing the placeholder text)
|
I have replaced the placeholder text in welcome.yml to a fitting message for the CFC server. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
zander-hub/src/main/java/org/modularsoft/zander/hub/utils/WelcomeSounds.java (1)
6-50: Consider verifying that SOUNDS is non-empty and using a more robust RNG.
WhileMath.random()is a valid approach, you could utilizeThreadLocalRandom.current().nextInt(SOUNDS.length)for potentially better performance and distribution. Additionally, in an edge case scenario where the SOUNDS array might be empty or manipulated, adding a sanity check could prevent an IndexOutOfBoundsException.public static Sound getRandomSound() { - return SOUNDS[(int) (Math.random() * SOUNDS.length)]; + int index = ThreadLocalRandom.current().nextInt(SOUNDS.length); + return SOUNDS[index]; }zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (3)
55-62: Centralizing permission-based setup logic
This event handler is straightforward. You might consider centralizing permission-based logic if there are other permissions or flags to set in the future, improving maintainability.
97-108: Possible collision rule conflicts
While this works for your immediate needs, be aware that if other teams exist, they may override the collision rule or cause confusion. If multiple functionalities require custom collision rules, a more granular approach might be needed.
167-172: Random pitch or config-driven approach
Currently, the pitch is fixed at 1.0. Some servers prefer slight variations or a configurable pitch to provide a more dynamic audio experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java(1 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/utils/WelcomeSounds.java(1 hunks)zander-hub/src/main/resources/welcome.yml(1 hunks)zander-hub/src/main/resources/welcome_old.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- zander-hub/src/main/resources/welcome_old.yml
🔇 Additional comments (2)
zander-hub/src/main/resources/welcome.yml (1)
2-11: Looks good!
The new content is clear and aligns with the updated branding/messaging. No issues from a YAML standpoint.
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (1)
173-176: Add null-check to avoid NPE
This is a repeated suggestion from a previous review. Please ensure the "vanished" metadata entry is present before streaming over it to avoid NullPointerExceptions.
| /// Triggers when player's client has joined the world. | ||
| /// Best used for initial world interactions (player world state changes etc). | ||
| @EventHandler | ||
| public void onPlayerJoin(PlayerJoinEvent event) { | ||
| Player player = event.getPlayer(); | ||
| event.joinMessage(null); // * suppress default | ||
| chatJoinMessage(player); | ||
| setInitialState(player); | ||
| Bukkit.getScheduler().runTaskLater(plugin, () -> { | ||
| // * bukkit uses 'world/playerdata' dir for tracking | ||
| if (!player.hasPlayedBefore() || TEST_ALWAYS_FIRST_JOIN) { | ||
| chatWelcomeMessage(player); | ||
| spawnWelcomeFirework(player); | ||
| } | ||
| playWelcomeSound(player); | ||
| }, ROUTINE_PLAYER_JOINED_DELAY); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add player online check before delayed task execution
The verification shows that the delayed task performs several player-dependent operations that could fail if the player disconnects:
spawnWelcomeFirework: Spawns entities and modifies world state at player's locationchatWelcomeMessage: Sends messages to the playerplayWelcomeSound: Plays sound at player's location
Add a player.isOnline() check at the start of the delayed task to prevent potential exceptions:
Bukkit.getScheduler().runTaskLater(plugin, () -> {
if (!player.isOnline()) return;
if (!player.hasPlayedBefore() || TEST_ALWAYS_FIRST_JOIN) {
chatWelcomeMessage(player);
spawnWelcomeFirework(player);
}
playWelcomeSound(player);
}, ROUTINE_PLAYER_JOINED_DELAY);🔗 Analysis chain
Potential scheduling check
When scheduling tasks with a delay, consider verifying that the player is still online before running logic that references player state (e.g., for spawnWelcomeFirework). This helps avoid possible exceptions or NPEs if the player quits early.
Let me search for more context about the player-related methods being called to better understand the potential impact.
Let me try a different approach to find these methods using ripgrep instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# This script searches for runTaskLater calls to see if there are any checks around player presence afterward.
rg "runTaskLater" -A 5
Length of output: 1445
Script:
#!/bin/bash
# Search for the implementation of methods that interact with the player
ast-grep --pattern 'void chatWelcomeMessage(Player $_) { $$$ }'
ast-grep --pattern 'void spawnWelcomeFirework(Player $_) { $$$ }'
ast-grep --pattern 'void playWelcomeSound(Player $_) { $$$ }'
Length of output: 197
Script:
#!/bin/bash
# Search for the method implementations using ripgrep
rg "chatWelcomeMessage|spawnWelcomeFirework|playWelcomeSound" -A 10
Length of output: 5507
…oin/leave messages which can be configured from 'config.yml'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
zander-hub/pom.xml (1)
50-63: Consider documenting dependency usage.Since these dependencies are crucial for core functionality (player visibility and effects), consider:
- Adding comments in the code where these dependencies are used
- Documenting fallback behavior if these plugins are missing
- Adding version checks in your plugin's onEnable method
zander-hub/src/main/java/org/modularsoft/zander/hub/utils/Misc.java (1)
12-14: Consider additional player checks for robust vanish detection.
You may want to ensure theplayer.isOnline()state or handle cases whereplayermay be offline to avoid unexpected behavior in edge cases.zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerLeave.java (1)
15-17: Plugin reference usage check.
Since this code stores an instance ofZanderHubMaininplugin, confirm how it will be leveraged later. If it's strictly not needed here, consider removing to keep the class cleaner.zander-hub/src/main/java/org/modularsoft/zander/hub/ConfigurationManager.java (1)
16-16: Static field usage caution.
A staticMessageConfigcan simplify access but be mindful of potential concurrency issues in environments with multiple threads. As long as the underlying data isn't mutated unsafely, this is acceptable.zander-hub/src/main/java/org/modularsoft/zander/hub/protection/HubProtection.java (1)
18-21: Consider marking plugin field as private final and validating non-null.Declaring the field as
private finalensures immutability, preventing accidental reassignments and making the code safer. Also, you may want to confirm thatpluginis non-null by throwing anIllegalArgumentExceptionif it’s null, to quickly detect invalid states.- ZanderHubMain plugin; + private final ZanderHubMain plugin; public HubProtection(ZanderHubMain plugin) { + if (plugin == null) { + throw new IllegalArgumentException("plugin cannot be null"); + } this.plugin = plugin; }zander-hub/src/main/java/org/modularsoft/zander/hub/configs/MessageConfig.java (2)
15-18: Use private fields to enforce encapsulation.Marking these text component fields as private helps ensure that they are only accessed through the class's methods, improving encapsulation.
- private TextComponent textCompJoinTemplate; - private TextComponent textCompLeaveTemplate; + private TextComponent textCompJoinTemplate; + private TextComponent textCompLeaveTemplate;
83-98: Handle potential concurrency or disk write performance issues.
setPremiumVanishloads and savesmessages.ymlon each plugin startup or message configuration change, which can be I/O intensive in some environments. Consider lazy-loading or caching if performance becomes an issue.zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (1)
126-159: Validate firework detonation state to avoid errors.If the firework is destroyed (e.g., chunk unload, plugin-based removal) before the scheduled detonation, calling
firework.detonate()might trigger unwanted exceptions or logs. You may wish to check if the entity is still valid to safely handle edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
zander-hub/pom.xml(2 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/ConfigurationManager.java(3 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/ZanderHubMain.java(4 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/configs/MessageConfig.java(1 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java(1 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerLeave.java(1 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/protection/HubProtection.java(1 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/utils/Misc.java(1 hunks)zander-hub/src/main/java/org/modularsoft/zander/hub/utils/WelcomeSounds.java(1 hunks)zander-hub/src/main/resources/plugin.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- zander-hub/src/main/java/org/modularsoft/zander/hub/utils/WelcomeSounds.java
- zander-hub/src/main/resources/plugin.yml
🔇 Additional comments (11)
zander-hub/pom.xml (2)
30-39: LGTM! Repository configurations are secure and well-structured.The new repositories for PremiumVanishAPI and ProtocolLib are properly configured using secure HTTPS URLs.
50-63: Verify dependency versions and compatibility.The dependency configuration looks good with appropriate 'provided' scopes. However, let's verify the versions:
zander-hub/src/main/java/org/modularsoft/zander/hub/utils/Misc.java (1)
8-10: Good job preventing instantiation of the utility class.
Declaring a private constructor in a utility class and throwing an exception is a solid practice to avoid accidental instantiation.zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerLeave.java (1)
19-26: Confirm the event API method usage for quit messages.
Some server versions useevent.setQuitMessage(...)instead ofevent.quitMessage(...). Verify compatibility and ensure the vanish check is correct if the vanish plugin is absent.✅ Verification successful
Let me gather more information about the server API version and dependencies.
Let me check the vanish plugin implementation to ensure it's properly handled.
The code is using the correct API and dependencies
The implementation is correct for the following reasons:
- The project uses Paper API 1.20.2-R0.1-SNAPSHOT which supports the
quitMessage(Component)method- PremiumVanish is properly declared as a required dependency in both pom.xml and plugin.yml
- The vanish check is correctly implemented using PremiumVanish's API (
VanishAPI.isInvisible())- The quit message handling is consistent with Paper API's Component-based message system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking references to PlayerQuitEvent usage across the codebase # to ensure consistency with the version of the server API. rg -A 5 $'class PlayerQuitEvent' rg -A 3 $'quitMessage'Length of output: 396
Script:
#!/bin/bash # Check pom.xml for server API version and dependencies cat **/pom.xml # Check for any other quit message handling patterns rg -A 3 "PlayerQuitEvent|setQuitMessage"Length of output: 8213
Script:
#!/bin/bash # Check the Misc.isVanish implementation and any vanish-related code rg -A 5 "isVanish" # Check for any plugin.yml to see if PremiumVanish is a dependency fd plugin.yml --exec cat {}Length of output: 2527
zander-hub/src/main/java/org/modularsoft/zander/hub/ZanderHubMain.java (2)
41-41: Event listener registration looks good.
RegisteringHubPlayerLeaveensures your custom quit messages and vanish checks execute as intended.
59-59: Configuration setup invoked properly.
CallingConfigurationManager.setupMessage()during plugin enable is a good approach to ensure message defaults are prepared.zander-hub/src/main/java/org/modularsoft/zander/hub/ConfigurationManager.java (2)
89-91: Accessor for message config is straightforward.
ThegetMessage()method neatly exposes the initializedMessageConfiginstance. Make sure all calls in the codebase consistently use this accessor to avoid referencing uninitialized objects.
65-87: Setup method ensures default messages exist.
This is a clean approach for initializing configuration messages if absent. However, verify that silently overwriting user changes is desired if they intentionally remove these config keys.✅ Verification successful
Let me check the configuration handling pattern more thoroughly.
Let me check the default configuration file content.
Default messages are safely added only when missing
Based on the verification:
- The
messages.joinandmessages.leavekeys are only referenced inConfigurationManager.java- The default
config.ymldoesn't contain these message keys- The code only adds default messages when the keys are completely missing (
!config.contains())- If a user intentionally removes these keys, they will be restored with defaults on next startup
This is actually a good practice because:
- It ensures essential configuration always exists for core functionality
- It doesn't overwrite existing values, only provides defaults when missing
- The default messages are simple and neutral ("joined"/"left")
- Changes are saved only when modifications were necessary (
hasModifiedConfig)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to the messages.join/messages.leave keys # to confirm they're used consistently and won't be overwritten unexpectedly. rg 'messages.join|messages.leave'Length of output: 951
Script:
#!/bin/bash # Check for any default config.yml or similar files that might contain default values fd "config.yml" # Look for any other configuration-related code that might handle these messages ast-grep --pattern 'getConfig().set($$$)'Length of output: 185
Script:
#!/bin/bash # Check the content of the hub's default config.yml cat zander-hub/src/main/resources/config.yml # Look for any other message configuration handling rg -A 3 "messages" zander-hub/src/main/resources/Length of output: 190
zander-hub/src/main/java/org/modularsoft/zander/hub/configs/MessageConfig.java (2)
23-35: Validate PremiumVanish plugin presence more comprehensively.Currently,
setPremiumVanishis called only ifplugin.getServer().getPluginManager().getPlugin("PremiumVanish") != null. Consider also verifying that the plugin is enabled or that the version meets minimum requirements to avoid potential runtime anomalies.
59-81: Confirm placeholder usage aligns with server messages.The code enforces exactly one occurrence of
%p%. If your server messages require a scenario missing the placeholder (or multiple placeholders), you might need more flexible logic. Otherwise, this is a valid approach to ensure correct player name insertion.zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (1)
71-79: 🛠️ Refactor suggestionCheck if the player remains online before executing delayed tasks.
When the delayed task triggers (1.2s later), the player might have disconnected. Consider verifying
player.isOnline()to avoid potential NPE or unexpected states when sending messages or spawning fireworks.Bukkit.getScheduler().runTaskLater(plugin, () -> { + if (!player.isOnline()) { + return; + } if (!player.hasPlayedBefore() || TEST_ALWAYS_FIRST_JOIN) { chatWelcomeMessage(player); spawnWelcomeFirework(player); } playWelcomeSound(player); }, ROUTINE_PLAYER_JOINED_DELAY);Likely invalid or redundant comment.
…edge-cases and prevent multiple welcomes
|
The integration with |
… trying to .detonate()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (3)
29-29: Consider moving test flag to configurationThe
TEST_ALWAYS_FIRST_JOINflag should ideally be in a configuration file rather than hardcoded, allowing for easier testing without code changes.
102-108: Consider using per-player scoreboardsUsing the main scoreboard for collision teams could conflict with other plugins. Consider creating per-player scoreboards or a dedicated plugin scoreboard.
119-124: Move message formatting to ConfigurationManagerConsider moving the color code translation logic to ConfigurationManager to maintain consistent message formatting across the plugin.
- List<String> message = ConfigurationManager.getWelcome().getStringList("newplayerwelcome"); - player.sendMessage(""); - for (String row : message) { - player.sendMessage(ChatColor.translateAlternateColorCodes('&', row)); - } - player.sendMessage(""); + List<String> message = ConfigurationManager.getWelcome().getFormattedWelcomeMessage(); + message.forEach(player::sendMessage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java(1 hunks)
🔇 Additional comments (3)
zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java (3)
51-162: Overall implementation looks solid! 👍The code is well-structured with clear separation of concerns, good documentation, and proper error handling. The refactoring has improved maintainability while adding new features like customized fireworks and welcome sounds.
158-162: 🛠️ Refactor suggestionAdd player connection check for firework detonation
Similar to the join event handler, add a player connection check before detonating the firework to prevent unnecessary world modifications.
Bukkit.getScheduler().runTaskLater(plugin, () -> { + if (!player.isConnected()) return; if (firework.isValid()) { firework.detonate(); } }, FIREWORK_DETONATE_DELAY);Likely invalid or redundant comment.
64-66: Potential race condition with vanish checkThe comment "runs before checking vanish" suggests a potential race condition. Consider moving the
setInitialStatecall after the vanish check to ensure consistent behavior.- setInitialState(player); // * just be aware, runs before checking vanish - if (Misc.isVanish(player)) - return; + if (Misc.isVanish(player)) + return; + setInitialState(player);
Very cool and additionally personally I would have left the PV messages alone done the vanish check and have a messages config option in the hub rather than depending on 2 configs. |
|
Looked through Codacy and made one change I found appropriate. |
…'setup' counterpart to run first
…les, added robustness checks on a few places, updated pom.xml to specify java 21
|
Now there is a nice and robust way to have integration with 'config.yml' |
|
Oh should mention that the internal/embedded 'config.yml' is copied over to the external/server 'config.yml' (any missing fields are filled, existing are respected). |
…'always_first_join', reverted to old 'welcome.yml' and refined some existing code.
benrobson
changed the title
There was a problem hiding this comment.
I make the welcome message and all others generic so that eventually other networks could use this.
There was a problem hiding this comment.
Could make a Boolean in the config to use the preset or to use random as I would still like the option to use all
There was a problem hiding this comment.
Could make the compass slot and such a configuration option in the config yml instead of setting it static.
| setInitialState(player); | ||
| Bukkit.getScheduler().runTaskLater(plugin, () -> { | ||
| // * bukkit uses 'world/playerdata' dir for tracking | ||
| if (!player.hasPlayedBefore() || TEST_ALWAYS_FIRST_JOIN) { |
There was a problem hiding this comment.
player.hasPlayedBefore should always return a Boolean, not sure why the secondary test is present?
|
Now in testing on the CFC Hub. |
I made it so it's easier to tweak certain parameters.
Structured the code for better maintainability (easier to test).
Separated stages for player connection (slight optimization).
Made the firework unique and detonates ~2 blocks above the player when they first connect.
The welcome sound now rely on a collection of appropriate sounds (before it was any sound, which would be creepy cave noises, warden spawn, enderdragon defeat etc.)
Previous logic when it comes to '/fly' and player vanish was essentially untouched.
(* tested /fly)
Summary by CodeRabbit
New Features
Bug Fixes
Chores